Detect if file that appears to be text in the first 1024 bytes is actually...

Detect if file that appears to be text in the first 1024 bytes is actually binary afer loading all data

app/models/blob.rb

@@ -191,9 +191,12 @@ class Blob < SimpleDelegator
     rendered_as_text? && rich_viewer
   end
 
+  def expanded?
+    !!@expanded
+  end
+
   def expand!
-    simple_viewer&.expanded = true
-    rich_viewer&.expanded = true
+    @expanded = true
   end
 
   private

app/models/blob_viewer/base.rb

@@ -6,15 +6,15 @@ module BlobViewer
 
     self.loading_partial_name = 'loading'
 
-    delegate :partial_path, :loading_partial_path, :rich?, :simple?, :text?, :binary?, to: :class
+    delegate :partial_path, :loading_partial_path, :rich?, :simple?, :load_async?, :text?, :binary?, to: :class
 
     attr_reader :blob
-    attr_accessor :expanded
 
     delegate :project, to: :blob
 
     def initialize(blob)
       @blob = blob
+      @initially_binary = blob.binary?
     end
 
     def self.partial_path
@@ -57,14 +57,10 @@ module BlobViewer
       false
     end
 
-    def load_async?
-      self.class.load_async? && render_error.nil?
-    end
-
     def collapsed?
       return @collapsed if defined?(@collapsed)
 
-      @collapsed = !expanded && collapse_limit && blob.raw_size > collapse_limit
+      @collapsed = !blob.expanded? && collapse_limit && blob.raw_size > collapse_limit
     end
 
     def too_large?
@@ -73,6 +69,10 @@ module BlobViewer
       @too_large = size_limit && blob.raw_size > size_limit
     end
 
+    def binary_detected_after_load?
+      !@initially_binary && blob.binary?
+    end
+
     # This method is used on the server side to check whether we can attempt to
     # render the blob at all. Human-readable error messages are found in the
     # `BlobHelper#blob_render_error_reason` helper.

app/views/projects/blob/_viewer.html.haml

 - hidden = local_assigns.fetch(:hidden, false)
 - render_error = viewer.render_error
-- load_async = local_assigns.fetch(:load_async, viewer.load_async?)
+- load_async = local_assigns.fetch(:load_async, viewer.load_async? && render_error.nil?)
 
 - viewer_url = local_assigns.fetch(:viewer_url) { url_for(params.merge(viewer: viewer.type, format: :json)) } if load_async
 .blob-viewer{ data: { type: viewer.type, url: viewer_url }, class: ('hidden' if hidden) }
-  - if load_async
-    = render viewer.loading_partial_path, viewer: viewer
-  - elsif render_error
+  - if render_error
     = render 'projects/blob/render_error', viewer: viewer
+  - elsif load_async
+    = render viewer.loading_partial_path, viewer: viewer
   - else
     - viewer.prepare!
+
+    -# In the rare case where the first kilobyte of the file looks like text,
+    -# but the file turns out to actually be binary after loading all data,
+    -# we fall back on the binary Download viewer.
+    - viewer = BlobViewer::Download.new(viewer.blob) if viewer.binary_detected_after_load?
+
     = render viewer.partial_path, viewer: viewer

changelogs/unreleased/dm-blob-binaryness-change.yml

+---
+title: Detect if file that appears to be text in the first 1024 bytes is actually
+  binary afer loading all data
+merge_request:
+author:

lib/gitlab/git/blob.rb

@@ -123,6 +123,7 @@ module Gitlab
         @loaded_all_data = true
         @data = repository.lookup(id).content
         @loaded_size = @data.bytesize
+        @binary = nil
       end
 
       def name

spec/features/projects/blobs/blob_show_spec.rb

@@ -3,8 +3,8 @@ require 'spec_helper'
 feature 'File blob', :js, feature: true do
   let(:project) { create(:project, :public) }
 
-  def visit_blob(path, fragment = nil)
-    visit namespace_project_blob_path(project.namespace, project, File.join('master', path), anchor: fragment)
+  def visit_blob(path, anchor: nil, ref: 'master')
+    visit namespace_project_blob_path(project.namespace, project, File.join(ref, path), anchor: anchor)
 
     wait_for_requests
   end
@@ -101,7 +101,7 @@ feature 'File blob', :js, feature: true do
 
     context 'visiting with a line number anchor' do
       before do
-        visit_blob('files/markdown/ruby-style-guide.md', 'L1')
+        visit_blob('files/markdown/ruby-style-guide.md', anchor: 'L1')
       end
 
       it 'displays the blob using the simple viewer' do
@@ -352,6 +352,37 @@ feature 'File blob', :js, feature: true do
     end
   end
 
+  context 'binary file that appears to be text in the first 1024 bytes' do
+    before do
+      visit_blob('encoding/binary-1.bin', ref: 'binary-encoding')
+    end
+
+    it 'displays the blob' do
+      aggregate_failures do
+        # shows a download link
+        expect(page).to have_link('Download (23.8 KB)')
+
+        # does not show a viewer switcher
+        expect(page).not_to have_selector('.js-blob-viewer-switcher')
+
+        # The specs below verify an arguably incorrect result, but since we only
+        # learn that the file is not actually text once the text viewer content
+        # is loaded asynchronously, there is no straightforward way to get these
+        # synchronously loaded elements to display correctly.
+        #
+        # Clicking the copy button will result in nothing being copied.
+        # Clicking the raw button will result in the binary file being downloaded,
+        # as expected.
+
+        # shows an enabled copy button, incorrectly
+        expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)')
+
+        # shows a raw button, incorrectly
+        expect(page).to have_link('Open raw')
+      end
+    end
+  end
+
   context '.gitlab-ci.yml' do
     before do
       project.add_master(project.creator)

spec/models/blob_viewer/base_spec.rb

@@ -106,9 +106,9 @@ describe BlobViewer::Base, model: true do
   end
 
   describe '#render_error' do
-    context 'when expanded' do
+    context 'when the blob is expanded' do
       before do
-        viewer.expanded = true
+        blob.expand!
       end
 
       context 'when the blob size is larger than the size limit' do