Fixing image lfs bug and also displaying text lfs

This commit, introduced in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23812,
fixes a problem creating a displaying image diff notes when the image
is stored in LFS. The main problem was that `Gitlab::Diff::File` was
returning an invalid valid in `text?` for this kind of files.

It also fixes a rendering problem with other LFS files, like text
ones. They LFS pointer shouldn't be shown when LFS is enabled
for the project, but they were.

15 files changed, 169 insertions, 85 deletions

diff --git a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
index d790bdc82ce..d19408ee87f 100644
--- a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
+++ b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb
@@ -90,9 +90,6 @@ describe 'Merge request > User creates image diff notes', :js do
 
   %w(inline parallel).each do |view|
     context "#{view} view" do
-      let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
-      let(:path)          { "files/images/ee_repo_logo.png" }
-
       let(:position) do
         Gitlab::Diff::Position.new(
           old_path: path,
@@ -108,9 +105,11 @@ describe 'Merge request > User creates image diff notes', :js do
 
       let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
 
-      describe 'creating a new diff note' do
+      shared_examples 'creates image diff note' do
         before do
           visit diffs_project_merge_request_path(project, merge_request, view: view)
+          wait_for_requests
+
           create_image_diff_note
         end
 
@@ -132,6 +131,32 @@ describe 'Merge request > User creates image diff notes', :js do
           expect(page).to have_content('image diff test comment')
         end
       end
+
+      context 'when images are not stored in LFS' do
+        let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
+        let(:path)          { 'files/images/ee_repo_logo.png' }
+
+        it_behaves_like 'creates image diff note'
+      end
+
+      context 'when images are stored in LFS' do
+        let(:merge_request) { create(:merge_request, source_project: project, target_project: project, source_branch: 'png-lfs', target_branch: 'master', author: user) }
+        let(:path)          { 'files/images/logo-black.png' }
+
+        before do
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+          project.update_attribute(:lfs_enabled, true)
+        end
+
+        it 'shows lfs badges' do
+          visit diffs_project_merge_request_path(project, merge_request, view: view)
+          wait_for_requests
+
+          expect(page.all('.diff-file span.label-lfs', visible: :all)).not_to be_empty
+        end
+
+        it_behaves_like 'creates image diff note'
+      end
     end
   end
 
diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb
index 0df9e4bbc1a..04b07525919 100644
--- a/spec/features/merge_request/user_sees_diff_spec.rb
+++ b/spec/features/merge_request/user_sees_diff_spec.rb
@@ -87,20 +87,6 @@ describe 'Merge request > User sees diff', :js do
       let(:current_user) { project.owner }
       let(:branch_name) {"test_branch"}
 
-      def create_file(branch_name, file_name, content)
-        Files::CreateService.new(
-          project,
-          current_user,
-          start_branch: branch_name,
-          branch_name: branch_name,
-          commit_message: "Create file",
-          file_path: file_name,
-          file_content: content
-        ).execute
-
-        project.commit(branch_name)
-      end
-
       it 'escapes any HTML special characters in the diff chunk header' do
         file_content =
           <<~CONTENT
@@ -136,5 +122,61 @@ describe 'Merge request > User sees diff', :js do
         expect(page).to have_css(".line[lang='rust'] .k")
       end
     end
+
+    context 'when file is stored in LFS' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+      let(:current_user) { project.owner }
+
+      context 'when LFS is enabled on the project' do
+        before do
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+          project.update_attribute(:lfs_enabled, true)
+
+          create_file('master', file_name, project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data)
+
+          visit diffs_project_merge_request_path(project, merge_request)
+        end
+
+        context 'when file is an image', :js do
+          let(:file_name) { 'files/lfs/image.png' }
+
+          it 'shows an error message' do
+            expect(page).not_to have_content('could not be displayed because it is stored in LFS')
+          end
+        end
+
+        context 'when file is not an image' do
+          let(:file_name) { 'files/lfs/ruby.rb' }
+
+          it 'shows an error message' do
+            expect(page).to have_content('This source diff could not be displayed because it is stored in LFS')
+          end
+        end
+      end
+
+      context 'when LFS is not enabled' do
+        before do
+          visit diffs_project_merge_request_path(project, merge_request)
+        end
+
+        it 'displays the diff' do
+          expect(page).to have_content('size 1575078')
+        end
+      end
+    end
+
+    def create_file(branch_name, file_name, content)
+      Files::CreateService.new(
+        project,
+        current_user,
+        start_branch: branch_name,
+        branch_name: branch_name,
+        commit_message: "Create file",
+        file_path: file_name,
+        file_content: content
+      ).execute
+
+      project.commit(branch_name)
+    end
   end
 end
diff --git a/spec/features/projects/commits/user_browses_commits_spec.rb b/spec/features/projects/commits/user_browses_commits_spec.rb
index 2159adf49fc..574a8aefd63 100644
--- a/spec/features/projects/commits/user_browses_commits_spec.rb
+++ b/spec/features/projects/commits/user_browses_commits_spec.rb
@@ -93,7 +93,7 @@ describe 'User browses commits' do
 
     it 'shows a blank label' do
       allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(nil)
-      allow_any_instance_of(Gitlab::Diff::File).to receive(:raw_binary?).and_return(true)
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:binary?).and_return(true)
 
       visit(project_commit_path(project, commit))
 
diff --git a/spec/fixtures/api/schemas/entities/diff_viewer.json b/spec/fixtures/api/schemas/entities/diff_viewer.json
index 19780f49a88..81325cd86c6 100644
--- a/spec/fixtures/api/schemas/entities/diff_viewer.json
+++ b/spec/fixtures/api/schemas/entities/diff_viewer.json
@@ -1,8 +1,20 @@
 {
   "type": "object",
-  "required": ["name"],
+  "required": [
+    "name"
+  ],
   "properties": {
-    "name": { "type": ["string"] }
+    "name": {
+      "type": [
+        "string"
+      ]
+    },
+    "error": {
+      "type": [
+        "string",
+        "null"
+      ]
+    }
   },
   "additionalProperties": false
 }
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 53c010fa0db..5396243f44d 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -256,43 +256,6 @@ describe DiffHelper do
     end
   end
 
-  context 'viewer related' do
-    let(:viewer) { diff_file.simple_viewer }
-
-    before do
-      assign(:project, project)
-    end
-
-    describe '#diff_render_error_reason' do
-      context 'for error :too_large' do
-        before do
-          expect(viewer).to receive(:render_error).and_return(:too_large)
-        end
-
-        it 'returns an error message' do
-          expect(helper.diff_render_error_reason(viewer)).to eq('it is too large')
-        end
-      end
-
-      context 'for error :server_side_but_stored_externally' do
-        before do
-          expect(viewer).to receive(:render_error).and_return(:server_side_but_stored_externally)
-          expect(diff_file).to receive(:external_storage).and_return(:lfs)
-        end
-
-        it 'returns an error message' do
-          expect(helper.diff_render_error_reason(viewer)).to eq('it is stored in LFS')
-        end
-      end
-    end
-
-    describe '#diff_render_error_options' do
-      it 'includes a "view the blob" link' do
-        expect(helper.diff_render_error_options(viewer)).to include(/view the blob/)
-      end
-    end
-  end
-
   context '#diff_file_path_text' do
     it 'returns full path by default' do
       expect(diff_file_path_text(diff_file)).to eq(diff_file.new_path)
diff --git a/spec/lib/gitlab/blob_helper_spec.rb b/spec/lib/gitlab/blob_helper_spec.rb
index 0b56f8687c3..e057385b35f 100644
--- a/spec/lib/gitlab/blob_helper_spec.rb
+++ b/spec/lib/gitlab/blob_helper_spec.rb
@@ -53,11 +53,11 @@ describe Gitlab::BlobHelper do
 
   describe '#text?' do
     it 'returns true' do
-      expect(blob.text?).to be_truthy
+      expect(blob.text_in_repo?).to be_truthy
     end
 
     it 'returns false' do
-      expect(large_blob.text?).to be_falsey
+      expect(large_blob.text_in_repo?).to be_falsey
     end
   end
 
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index b15d22c634a..862590268ca 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -310,7 +310,7 @@ describe Gitlab::Diff::File do
     context 'when the content changed' do
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns a No Preview viewer' do
@@ -345,7 +345,7 @@ describe Gitlab::Diff::File do
 
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns an Added viewer' do
@@ -380,7 +380,7 @@ describe Gitlab::Diff::File do
 
       context 'when the file represented by the diff file is binary' do
         before do
-          allow(diff_file).to receive(:raw_binary?).and_return(true)
+          allow(diff_file).to receive(:binary?).and_return(true)
         end
 
         it 'returns a Deleted viewer' do
@@ -436,7 +436,7 @@ describe Gitlab::Diff::File do
         allow(diff_file).to receive(:deleted_file?).and_return(false)
         allow(diff_file).to receive(:renamed_file?).and_return(false)
         allow(diff_file).to receive(:mode_changed?).and_return(false)
-        allow(diff_file).to receive(:raw_text?).and_return(false)
+        allow(diff_file).to receive(:text?).and_return(false)
       end
 
       it 'returns a No Preview viewer' do
diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
index 8e00c8e0e30..f22c2c90334 100644
--- a/spec/lib/gitlab/diff/lines_unfolder_spec.rb
+++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb
@@ -185,7 +185,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   let(:project) { create(:project) }
 
-  let(:old_blob) { Gitlab::Git::Blob.new(data: raw_old_blob) }
+  let(:old_blob) { Blob.decorate(Gitlab::Git::Blob.new(data: raw_old_blob, size: 10)) }
 
   let(:diff) do
     Gitlab::Git::Diff.new(diff: raw_diff,
diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb
index 80dd3dcc58e..1bcec04d28f 100644
--- a/spec/lib/gitlab/git/blob_spec.rb
+++ b/spec/lib/gitlab/git/blob_spec.rb
@@ -59,7 +59,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       it { expect(blob.data[0..10]).to eq("*.rbc\n*.sas") }
       it { expect(blob.size).to eq(241) }
       it { expect(blob.mode).to eq("100644") }
-      it { expect(blob).not_to be_binary }
+      it { expect(blob).not_to be_binary_in_repo }
     end
 
     context 'file in root with leading slash' do
@@ -92,7 +92,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       end
 
       it 'does not mark the blob as binary' do
-        expect(blob).not_to be_binary
+        expect(blob).not_to be_binary_in_repo
       end
     end
 
@@ -123,7 +123,7 @@ describe Gitlab::Git::Blob, :seed_helper do
           .with(hash_including(binary: true))
           .and_call_original
 
-        expect(blob).to be_binary
+        expect(blob).to be_binary_in_repo
       end
     end
   end
@@ -196,7 +196,7 @@ describe Gitlab::Git::Blob, :seed_helper do
       it { expect(blob.id).to eq('409f37c4f05865e4fb208c771485f211a22c4c2d') }
       it { expect(blob.data).to eq('') }
       it 'does not mark the blob as binary' do
-        expect(blob).not_to be_binary
+        expect(blob).not_to be_binary_in_repo
       end
     end
 
diff --git a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
index 9db710e759e..742b2872c40 100644
--- a/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/blobs_stitcher_spec.rb
@@ -21,7 +21,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
       expect(blobs[0].size).to eq(1642)
       expect(blobs[0].commit_id).to eq('f00ba7')
       expect(blobs[0].data).to eq("first-line\nsecond-line")
-      expect(blobs[0].binary?).to be false
+      expect(blobs[0].binary_in_repo?).to be false
 
       expect(blobs[1].id).to eq('abcdef2')
       expect(blobs[1].mode).to eq('100644')
@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::BlobsStitcher do
       expect(blobs[1].size).to eq(2461)
       expect(blobs[1].commit_id).to eq('f00ba8')
       expect(blobs[1].data).to eq("GIF87a\x90\x01".b)
-      expect(blobs[1].binary?).to be true
+      expect(blobs[1].binary_in_repo?).to be true
     end
   end
 end
diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb
index e8c03b587e2..05cf242e84d 100644
--- a/spec/models/blob_spec.rb
+++ b/spec/models/blob_spec.rb
@@ -122,14 +122,14 @@ describe Blob do
     end
   end
 
-  describe '#raw_binary?' do
+  describe '#binary?' do
     context 'if the blob is stored externally' do
       context 'if the extension has a rich viewer' do
         context 'if the viewer is binary' do
           it 'returns true' do
             blob = fake_blob(path: 'file.pdf', lfs: true)
 
-            expect(blob.raw_binary?).to be_truthy
+            expect(blob.binary?).to be_truthy
           end
         end
 
@@ -137,7 +137,7 @@ describe Blob do
           it 'return false' do
             blob = fake_blob(path: 'file.md', lfs: true)
 
-            expect(blob.raw_binary?).to be_falsey
+            expect(blob.binary?).to be_falsey
           end
         end
       end
@@ -148,7 +148,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.txt', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -156,7 +156,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.ics', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
         end
@@ -166,7 +166,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.rb', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -174,7 +174,7 @@ describe Blob do
             it 'returns true' do
               blob = fake_blob(path: 'file.exe', lfs: true)
 
-              expect(blob.raw_binary?).to be_truthy
+              expect(blob.binary?).to be_truthy
             end
           end
         end
@@ -184,7 +184,7 @@ describe Blob do
             it 'returns false' do
               blob = fake_blob(path: 'file.ini', lfs: true)
 
-              expect(blob.raw_binary?).to be_falsey
+              expect(blob.binary?).to be_falsey
             end
           end
 
@@ -192,7 +192,7 @@ describe Blob do
             it 'returns true' do
               blob = fake_blob(path: 'file.wtf', lfs: true)
 
-              expect(blob.raw_binary?).to be_truthy
+              expect(blob.binary?).to be_truthy
             end
           end
         end
@@ -204,7 +204,7 @@ describe Blob do
         it 'returns true' do
           blob = fake_blob(path: 'file.pdf', binary: true)
 
-          expect(blob.raw_binary?).to be_truthy
+          expect(blob.binary?).to be_truthy
         end
       end
 
@@ -212,7 +212,7 @@ describe Blob do
         it 'return false' do
           blob = fake_blob(path: 'file.md')
 
-          expect(blob.raw_binary?).to be_falsey
+          expect(blob.binary?).to be_falsey
         end
       end
     end
diff --git a/spec/models/diff_viewer/base_spec.rb b/spec/models/diff_viewer/base_spec.rb
index c90b32c5d77..f4efe5a7b3a 100644
--- a/spec/models/diff_viewer/base_spec.rb
+++ b/spec/models/diff_viewer/base_spec.rb
@@ -58,7 +58,7 @@ describe DiffViewer::Base do
 
       context 'when the binaryness does not match' do
         before do
-          allow_any_instance_of(Blob).to receive(:binary?).and_return(true)
+          allow_any_instance_of(Blob).to receive(:binary_in_repo?).and_return(true)
         end
 
         it 'returns false' do
@@ -141,4 +141,25 @@ describe DiffViewer::Base do
       end
     end
   end
+
+  describe '#render_error_message' do
+    it 'returns nothing when no render_error' do
+      expect(viewer.render_error).to be_nil
+      expect(viewer.render_error_message).to be_nil
+    end
+
+    context 'when render_error error' do
+      before do
+        allow(viewer).to receive(:render_error).and_return(:too_large)
+      end
+
+      it 'returns an error message' do
+        expect(viewer.render_error_message).to include('it is too large')
+      end
+
+      it 'includes a "view the blob" link' do
+        expect(viewer.render_error_message).to include('view the blob')
+      end
+    end
+  end
 end
diff --git a/spec/models/diff_viewer/server_side_spec.rb b/spec/models/diff_viewer/server_side_spec.rb
index 98a8f6d4cc9..86b14b6ebf3 100644
--- a/spec/models/diff_viewer/server_side_spec.rb
+++ b/spec/models/diff_viewer/server_side_spec.rb
@@ -32,4 +32,24 @@ describe DiffViewer::ServerSide do
       end
     end
   end
+
+  describe '#render_error_reason' do
+    context 'when the diff file is stored externally' do
+      before do
+        allow(diff_file).to receive(:stored_externally?).and_return(true)
+      end
+
+      it 'returns error message if stored in LFS' do
+        allow(diff_file).to receive(:external_storage).and_return(:lfs)
+
+        expect(subject.render_error_message).to include('it is stored in LFS')
+      end
+
+      it 'returns error message if stored externally' do
+        allow(diff_file).to receive(:external_storage).and_return(:foo)
+
+        expect(subject.render_error_message).to include('it is stored externally')
+      end
+    end
+  end
 end
diff --git a/spec/support/helpers/fake_blob_helpers.rb b/spec/support/helpers/fake_blob_helpers.rb
index bc9686ed9cf..801ca8b7412 100644
--- a/spec/support/helpers/fake_blob_helpers.rb
+++ b/spec/support/helpers/fake_blob_helpers.rb
@@ -23,7 +23,7 @@ module FakeBlobHelpers
       0
     end
 
-    def binary?
+    def binary_in_repo?
       @binary
     end
 
diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb
index d52c40ff4f1..d352a7cdf1a 100644
--- a/spec/support/helpers/test_env.rb
+++ b/spec/support/helpers/test_env.rb
@@ -62,7 +62,8 @@ module TestEnv
     'between-create-delete-modify-move'  => '3f5f443',
     'after-create-delete-modify-move'    => 'ba3faa7',
     'with-codeowners'                    => '219560e',
-    'submodule_inside_folder'            => 'b491b92'
+    'submodule_inside_folder'            => 'b491b92',
+    'png-lfs'                            => 'fe42f41'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily