Merge branch 'diff-file-lookup-with-file-identifier-hash' into 'master'

Find diff_files using file_identifer_hash if available

See merge request gitlab-org/gitlab!33579

app/models/diff_note.rb

@@ -136,7 +136,7 @@ class DiffNote < Note
       # As an extra benefit, the returned `diff_file` already
       # has `highlighted_diff_lines` data set from Redis on
       # `Diff::FileCollection::MergeRequestDiff`.
-      file = noteable.diffs(original_position.diff_options).diff_files.first
+      file = original_position.find_diff_file_from(noteable)
       # if line is not found in persisted diffs, fallback and retrieve file from repository using gitaly
       # This is required because of https://gitlab.com/gitlab-org/gitlab/issues/42676
       file = nil if file&.line_for_position(original_position).nil? && importing?

lib/gitlab/diff/position.rb

@@ -157,13 +157,23 @@ module Gitlab
         position_type == 'text'
       end
 
+      def find_diff_file_from(diffable)
+        diff_files = diffable.diffs(diff_options).diff_files
+
+        if Feature.enabled?(:file_identifier_hash) && file_identifier_hash.present?
+          diff_files.find { |df| df.file_identifier_hash == file_identifier_hash }
+        else
+          diff_files.first
+        end
+      end
+
       private
 
       def find_diff_file(repository)
         return unless diff_refs.complete?
         return unless comparison = diff_refs.compare_in(repository.project)
 
-        comparison.diffs(diff_options).diff_files.first
+        find_diff_file_from(comparison)
       end
 
       def get_formatter_class(type)

spec/lib/gitlab/diff/position_spec.rb

@@ -574,6 +574,86 @@ describe Gitlab::Diff::Position do
     end
   end
 
+  describe '#find_diff_file_from' do
+    context "position for a diff file that has changed from symlink to regular file" do
+      let(:commit) { project.commit("81e6355ce4e1544a3524b230952c12455de0777b") }
+
+      let(:old_symlink_file_identifier_hash) { "bfa430463f33619872d52a6b85ced59c973e42dc" }
+      let(:new_regular_file_identifier_hash) { "e25b60c2e5ffb977d2b1431b96c6f7800c3c3529" }
+      let(:file_identifier_hash) { new_regular_file_identifier_hash }
+
+      let(:args) do
+        {
+          file_identifier_hash: file_identifier_hash,
+          old_path: "symlink",
+          new_path: "symlink",
+          old_line: nil,
+          new_line: 1,
+          diff_refs: commit.diff_refs
+        }
+      end
+
+      let(:diffable) { commit.diff_refs.compare_in(project) }
+
+      subject(:diff_file) { described_class.new(args).find_diff_file_from(diffable) }
+
+      context 'when file_identifier_hash is disabled' do
+        before do
+          stub_feature_flags(file_identifier_hash: false)
+        end
+
+        it "returns the first diff file" do
+          expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash)
+        end
+      end
+
+      context 'when file_identifier_hash is enabled' do
+        before do
+          stub_feature_flags(file_identifier_hash: true)
+        end
+
+        context 'for new regular file' do
+          it "returns the correct diff file" do
+            expect(diff_file.file_identifier_hash).to eq(new_regular_file_identifier_hash)
+          end
+        end
+
+        context 'for old symlink file' do
+          let(:args) do
+            {
+              file_identifier_hash: old_symlink_file_identifier_hash,
+              old_path: "symlink",
+              new_path: "symlink",
+              old_line: 1,
+              new_line: nil,
+              diff_refs: commit.diff_refs
+            }
+          end
+
+          it "returns the correct diff file" do
+            expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash)
+          end
+        end
+
+        context 'when file_identifier_hash is missing' do
+          let(:file_identifier_hash) { nil }
+
+          it "returns the first diff file" do
+            expect(diff_file.file_identifier_hash).to eq(old_symlink_file_identifier_hash)
+          end
+        end
+
+        context 'when file_identifier_hash cannot be found' do
+          let(:file_identifier_hash) { "missingidentifier" }
+
+          it "returns nil" do
+            expect(diff_file).to be_nil
+          end
+        end
+      end
+    end
+  end
+
   describe '#==' do
     let(:commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
 

spec/models/diff_note_spec.rb

@@ -238,12 +238,32 @@ describe DiffNote do
     end
 
     context 'when the discussion was created in the diff' do
-      it 'returns correct diff file' do
-        diff_file = subject.diff_file
+      context 'when file_identifier_hash is disabled' do
+        before do
+          stub_feature_flags(file_identifier_hash: false)
+        end
 
-        expect(diff_file.old_path).to eq(position.old_path)
-        expect(diff_file.new_path).to eq(position.new_path)
-        expect(diff_file.diff_refs).to eq(position.diff_refs)
+        it 'returns correct diff file' do
+          diff_file = subject.diff_file
+
+          expect(diff_file.old_path).to eq(position.old_path)
+          expect(diff_file.new_path).to eq(position.new_path)
+          expect(diff_file.diff_refs).to eq(position.diff_refs)
+        end
+      end
+
+      context 'when file_identifier_hash is enabled' do
+        before do
+          stub_feature_flags(file_identifier_hash: true)
+        end
+
+        it 'returns correct diff file' do
+          diff_file = subject.diff_file
+
+          expect(diff_file.old_path).to eq(position.old_path)
+          expect(diff_file.new_path).to eq(position.new_path)
+          expect(diff_file.diff_refs).to eq(position.diff_refs)
+        end
       end
     end