Merge branch 'dm-blob-binaryness-change' into 'master'

Detect if blob binaryness changes after loading all data

See merge request !11981

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