Commit 8fe28e12 authored by David Kim's avatar David Kim

Find diff_files using file_identifer_hash if available

This change allows us to use file_identifier_hash to find the correct
diff_file when there are more than one with the same file_path
parent e999b496
......@@ -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?
......
......@@ -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)
......
......@@ -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") }
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment