Merge branch 'dm-diff-file-diffable' into 'master'

app/views/projects/diffs/_content.html.haml

@@ -6,7 +6,7 @@
   - elsif blob.truncated?
     .nothing-here-block The file could not be displayed because it is too large.
   - elsif blob.readable_text?
-    - if !diff_file.repository.diffable?(blob)
+    - if !diff_file.diffable?
       .nothing-here-block This diff was suppressed by a .gitattributes entry.
     - elsif diff_file.collapsed?
       - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier))

lib/gitlab/diff/file.rb

@@ -165,6 +165,18 @@ module Gitlab
       def file_identifier
         "#{file_path}-#{new_file?}-#{deleted_file?}-#{renamed_file?}"
       end
+
+      def diffable?
+        repository.attributes(file_path).fetch('diff') { true }
+      end
+
+      def binary?
+        old_blob&.binary? || new_blob&.binary?
+      end
+
+      def text?
+        !binary?
+      end
     end
   end
 end

lib/gitlab/diff/file_collection/merge_request_diff.rb

@@ -66,10 +66,7 @@ module Gitlab
         end
 
         def cacheable?(diff_file)
-          @merge_request_diff.present? &&
-            diff_file.blob &&
-            diff_file.blob.text? &&
-            @project.repository.diffable?(diff_file.blob)
+          @merge_request_diff.present? && diff_file.text? && diff_file.diffable?
         end
 
         def cache_key

lib/gitlab/git/repository.rb

@@ -962,11 +962,6 @@ module Gitlab
         end
       end
 
-      # Checks if the blob should be diffable according to its attributes
-      def diffable?(blob)
-        attributes(blob.path).fetch('diff') { blob.text? }
-      end
-
       # Returns the Git attributes for the given file path.
       #
       # See `Gitlab::Git::Attributes` for more information.

spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

@@ -5,15 +5,7 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
   let(:diff_files) { described_class.new(merge_request.merge_request_diff, diff_options: nil).diff_files }
 
   it 'does not highlight binary files' do
-    allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(double("text?" => false))
-
-    expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
-
-    diff_files
-  end
-
-  it 'does not highlight file if blob is not accessable' do
-    allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(nil)
+    allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(false)
 
     expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
 
@@ -21,7 +13,7 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
   end
 
   it 'does not files marked as undiffable in .gitattributes' do
-    allow_any_instance_of(Repository).to receive(:diffable?).and_return(false)
+    allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
 
     expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
 

spec/lib/gitlab/diff/file_spec.rb

@@ -6,7 +6,7 @@ describe Gitlab::Diff::File, lib: true do
   let(:project) { create(:project, :repository) }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff) { commit.raw_diffs.first }
-  let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
+  let(:diff_file) { described_class.new(diff, diff_refs: commit.diff_refs, repository: project.repository) }
 
   describe '#diff_lines' do
     let(:diff_lines) { diff_file.diff_lines }
@@ -63,11 +63,33 @@ describe Gitlab::Diff::File, lib: true do
     end
   end
 
-  describe '#blob' do
+  describe '#new_blob' do
     it 'returns blob of new commit' do
-      data = diff_file.blob.data
+      data = diff_file.new_blob.data
 
       expect(data).to include('raise RuntimeError, "System commands must be given as an array of strings"')
     end
   end
+
+  describe '#diffable?' do
+    let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+    let(:diffs) { commit.diffs }
+
+    before do
+      info_dir_path = File.join(project.repository.path_to_repo, 'info')
+
+      FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path)
+      File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n")
+    end
+
+    it "returns true for files that do not have attributes" do
+      diff_file = diffs.diff_file_with_new_path('LICENSE')
+      expect(diff_file.diffable?).to be_truthy
+    end
+
+    it "returns false for files that have been marked as not being diffable in attributes" do
+      diff_file = diffs.diff_file_with_new_path('README.md')
+      expect(diff_file.diffable?).to be_falsey
+    end
+  end
 end

spec/lib/gitlab/git/repository_spec.rb

@@ -1235,47 +1235,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe '#diffable' do
-    info_dir_path = attributes_path = File.join(SEED_STORAGE_PATH, TEST_REPO_PATH, 'info')
-    attributes_path = File.join(info_dir_path, 'attributes')
-
-    before(:all) do
-      FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path)
-      File.write(attributes_path, "*.md -diff\n")
-    end
-
-    it "should return true for files which are text and do not have attributes" do
-      blob = Gitlab::Git::Blob.find(
-        repository,
-        '33bcff41c232a11727ac6d660bd4b0c2ba86d63d',
-        'LICENSE'
-      )
-      expect(repository.diffable?(blob)).to be_truthy
-    end
-
-    it "should return false for binary files which do not have attributes" do
-      blob = Gitlab::Git::Blob.find(
-        repository,
-        '33bcff41c232a11727ac6d660bd4b0c2ba86d63d',
-        'files/images/logo-white.png'
-      )
-      expect(repository.diffable?(blob)).to be_falsey
-    end
-
-    it "should return false for text files which have been marked as not being diffable in attributes" do
-      blob = Gitlab::Git::Blob.find(
-        repository,
-        '33bcff41c232a11727ac6d660bd4b0c2ba86d63d',
-        'README.md'
-      )
-      expect(repository.diffable?(blob)).to be_falsey
-    end
-
-    after(:all) do
-      FileUtils.rm_rf(info_dir_path)
-    end
-  end
-
   describe '#tag_exists?' do
     it 'returns true for an existing tag' do
       tag = repository.tag_names.first

spec/services/merge_requests/merge_request_diff_cache_service_spec.rb

@@ -10,8 +10,8 @@ describe MergeRequests::MergeRequestDiffCacheService do
 
       expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
       expect(Rails.cache).to receive(:write).with(cache_key, anything)
-      allow_any_instance_of(Gitlab::Diff::File).to receive(:blob).and_return(double("text?" => true))
-      allow_any_instance_of(Repository).to receive(:diffable?).and_return(true)
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
+      allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
 
       subject.execute(merge_request)
     end