Commit f7993c7c authored by David Kim's avatar David Kim Committed by Oswaldo Ferreira

Use file_mode to guide position tracing

Since we are dealing with different diff_refs during position tracing,
file_identifier_hash cannot be used to find the correct diff_file.
Using file_mode seems to be viable in these cases since we can
find the diff_file with file_identifier_hash from the position to access
the correct file_mode.
parent 081d1801
......@@ -60,12 +60,20 @@ module Gitlab
end
end
def diff_file_with_old_path(old_path)
diff_files.find { |diff_file| diff_file.old_path == old_path }
def diff_file_with_old_path(old_path, a_mode = nil)
if Feature.enabled?(:file_identifier_hash) && a_mode.present?
diff_files.find { |diff_file| diff_file.old_path == old_path && diff_file.a_mode == a_mode }
else
diff_files.find { |diff_file| diff_file.old_path == old_path }
end
end
def diff_file_with_new_path(new_path)
diff_files.find { |diff_file| diff_file.new_path == new_path }
def diff_file_with_new_path(new_path, b_mode = nil)
if Feature.enabled?(:file_identifier_hash) && b_mode.present?
diff_files.find { |diff_file| diff_file.new_path == new_path && diff_file.b_mode == b_mode }
else
diff_files.find { |diff_file| diff_file.new_path == new_path }
end
end
def clear_cache
......
......@@ -42,6 +42,10 @@ module Gitlab
@cd_diffs ||= compare(new_diff_refs.start_sha, new_diff_refs.head_sha)
end
def diff_file(position)
position.diff_file(project.repository)
end
private
def compare(start_sha, head_sha, straight: false)
......
......@@ -8,6 +8,7 @@ module Gitlab
delegate \
:project,
:diff_file,
:ac_diffs,
:bd_diffs,
:cd_diffs,
......
......@@ -5,22 +5,26 @@ module Gitlab
class PositionTracer
class ImageStrategy < BaseStrategy
def trace(position)
a_path = position.old_path
b_path = position.new_path
diff_file = diff_file(position)
a_mode = diff_file&.a_mode
b_mode = diff_file&.b_mode
# If file exists in B->D (e.g. updated, renamed, removed), let the
# note become outdated.
bd_diff = bd_diffs.diff_file_with_old_path(b_path)
bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode)
return { position: new_position(position, bd_diff), outdated: true } if bd_diff
# If file still exists in the new diff, update the position.
cd_diff = cd_diffs.diff_file_with_new_path(bd_diff&.new_path || b_path)
cd_diff = cd_diffs.diff_file_with_new_path(b_path, b_mode)
return { position: new_position(position, cd_diff), outdated: false } if cd_diff
# If file exists in A->C (e.g. rebased and same changes were present
# in target branch), let the note become outdated.
ac_diff = ac_diffs.diff_file_with_old_path(position.old_path)
ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode)
return { position: new_position(position, ac_diff), outdated: true } if ac_diff
......
......@@ -76,16 +76,20 @@ module Gitlab
def trace_added_line(position)
b_path = position.new_path
b_line = position.new_line
diff_file = diff_file(position)
b_mode = diff_file&.b_mode
bd_diff = bd_diffs.diff_file_with_old_path(b_path)
bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode)
d_path = bd_diff&.new_path || b_path
d_mode = bd_diff&.b_mode || b_mode
d_line = LineMapper.new(bd_diff).old_to_new(b_line)
if d_line
cd_diff = cd_diffs.diff_file_with_new_path(d_path)
cd_diff = cd_diffs.diff_file_with_new_path(d_path, d_mode)
c_path = cd_diff&.old_path || d_path
c_mode = cd_diff&.a_mode || d_mode
c_line = LineMapper.new(cd_diff).new_to_old(d_line)
if c_line
......@@ -98,7 +102,7 @@ module Gitlab
else
# If the line is no longer in the MR, we unfortunately cannot show
# the current state on the CD diff, so we treat it as outdated.
ac_diff = ac_diffs.diff_file_with_new_path(c_path)
ac_diff = ac_diffs.diff_file_with_new_path(c_path, c_mode)
{ position: new_position(ac_diff, nil, c_line), outdated: true }
end
......@@ -115,22 +119,26 @@ module Gitlab
def trace_removed_line(position)
a_path = position.old_path
a_line = position.old_line
diff_file = diff_file(position)
a_mode = diff_file&.a_mode
ac_diff = ac_diffs.diff_file_with_old_path(a_path)
ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode)
c_path = ac_diff&.new_path || a_path
c_mode = ac_diff&.b_mode || a_mode
c_line = LineMapper.new(ac_diff).old_to_new(a_line)
if c_line
cd_diff = cd_diffs.diff_file_with_old_path(c_path)
cd_diff = cd_diffs.diff_file_with_old_path(c_path, c_mode)
d_path = cd_diff&.new_path || c_path
d_mode = cd_diff&.b_mode || c_mode
d_line = LineMapper.new(cd_diff).old_to_new(c_line)
if d_line
# If the line is still in C but also in D, it has turned from a
# removed line into an unchanged one.
bd_diff = bd_diffs.diff_file_with_new_path(d_path)
bd_diff = bd_diffs.diff_file_with_new_path(d_path, d_mode)
{ position: new_position(bd_diff, nil, d_line), outdated: true }
else
......@@ -148,17 +156,21 @@ module Gitlab
a_line = position.old_line
b_path = position.new_path
b_line = position.new_line
diff_file = diff_file(position)
a_mode = diff_file&.a_mode
b_mode = diff_file&.b_mode
ac_diff = ac_diffs.diff_file_with_old_path(a_path)
ac_diff = ac_diffs.diff_file_with_old_path(a_path, a_mode)
c_path = ac_diff&.new_path || a_path
c_mode = ac_diff&.b_mode || a_mode
c_line = LineMapper.new(ac_diff).old_to_new(a_line)
bd_diff = bd_diffs.diff_file_with_old_path(b_path)
bd_diff = bd_diffs.diff_file_with_old_path(b_path, b_mode)
d_line = LineMapper.new(bd_diff).old_to_new(b_line)
cd_diff = cd_diffs.diff_file_with_old_path(c_path)
cd_diff = cd_diffs.diff_file_with_old_path(c_path, c_mode)
if c_line && d_line
# If the line is still in C and D, it is still unchanged.
......
......@@ -234,5 +234,118 @@ describe Gitlab::Diff::PositionTracer::ImageStrategy do
end
end
end
describe 'symlink scenarios' do
let(:new_file) { old_file_status == :new }
let(:deleted_file) { old_file_status == :deleted }
let(:renamed_file) { old_file_status == :renamed }
let(:file_identifier) { "#{file_name}-#{new_file}-#{deleted_file}-#{renamed_file}" }
let(:file_identifier_hash) { Digest::SHA1.hexdigest(file_identifier) }
let(:old_position) { position(old_path: file_name, new_path: file_name, position_type: 'image', file_identifier_hash: file_identifier_hash) }
let(:update_file_commit) do
initial_commit
update_file(
branch_name,
file_name,
Base64.encode64('morecontent')
)
end
let(:delete_file_commit) do
initial_commit
delete_file(branch_name, file_name)
end
let(:create_second_file_commit) do
initial_commit
create_file(
branch_name,
second_file_name,
Base64.encode64('morecontent')
)
end
before do
stub_feature_flags(file_identifier_hash: true)
end
describe 'from symlink to image' do
let(:initial_commit) { project.commit('a19c7f9a147e35e535c797cf148d29c24dac5544') }
let(:symlink_to_image_commit) { project.commit('8cfca8420812e5bd7479aa32cf33e0c95a3ca576') }
let(:branch_name) { 'diff-files-symlink-to-image' }
let(:file_name) { 'symlink-to-image.png' }
context "when the old position is on the new image file" do
let(:old_file_status) { :new }
context "when the image file's content was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, create_second_file_commit) }
it "returns the new position" do
expect_new_position(
old_path: file_name,
new_path: file_name
)
end
end
context "when the image file's content was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, update_file_commit) }
let(:change_diff_refs) { diff_refs(symlink_to_image_commit, update_file_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name
)
end
end
context "when the image file was removed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_image_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(symlink_to_image_commit, delete_file_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name
)
end
end
end
end
describe 'from image to symlink' do
let(:initial_commit) { project.commit('d10dcdfbbb2b59a959a5f5d66a4adf28f0ea4008') }
let(:image_to_symlink_commit) { project.commit('3e94fdaa60da8aed38401b91bc56be70d54ca424') }
let(:branch_name) { 'diff-files-image-to-symlink' }
let(:file_name) { 'image-to-symlink.png' }
context "when the old position is on the added image file" do
let(:old_file_status) { :new }
context "when the image file gets changed to a symlink between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit.parent, initial_commit) }
let(:new_diff_refs) { diff_refs(initial_commit.parent, image_to_symlink_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, image_to_symlink_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name
)
end
end
end
end
end
end
end
......@@ -1801,5 +1801,143 @@ describe Gitlab::Diff::PositionTracer::LineStrategy do
end
end
end
describe 'symlink scenarios' do
let(:new_file) { old_file_status == :new }
let(:deleted_file) { old_file_status == :deleted }
let(:renamed_file) { old_file_status == :renamed }
let(:file_identifier) { "#{file_name}-#{new_file}-#{deleted_file}-#{renamed_file}" }
let(:file_identifier_hash) { Digest::SHA1.hexdigest(file_identifier) }
let(:update_line_commit) do
update_file(
branch_name,
file_name,
<<-CONTENT.strip_heredoc
A
BB
C
CONTENT
)
end
let(:delete_file_commit) do
delete_file(branch_name, file_name)
end
let(:create_second_file_commit) do
create_file(
branch_name,
second_file_name,
<<-CONTENT.strip_heredoc
D
E
CONTENT
)
end
before do
stub_feature_flags(file_identifier_hash: true)
end
describe 'from symlink to text' do
let(:initial_commit) { project.commit('0e5b363105e9176a77bac94d7ff6d8c4fb35c3eb') }
let(:symlink_to_text_commit) { project.commit('689815e617abc6889f1fded4834d2dd7d942a58e') }
let(:branch_name) { 'diff-files-symlink-to-text' }
let(:file_name) { 'symlink-to-text.txt' }
let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 3, file_identifier_hash: file_identifier_hash) }
before do
create_branch('diff-files-symlink-to-text-test', branch_name)
end
context "when the old position is on the new text file" do
let(:old_file_status) { :new }
context "when the text file's content was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, create_second_file_commit) }
it "returns the new position" do
expect_new_position(
new_path: old_position.new_path,
new_line: old_position.new_line
)
end
end
context "when the text file's content has change, but the line was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) }
it "returns the new position" do
expect_new_position(
new_path: old_position.new_path,
new_line: old_position.new_line
)
end
end
context "when the text file's line was changed between the old and the new diff" do
let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 2, file_identifier_hash: file_identifier_hash) }
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:change_diff_refs) { diff_refs(symlink_to_text_commit, update_line_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when the text file was removed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, symlink_to_text_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(symlink_to_text_commit, delete_file_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
describe 'from text to symlink' do
let(:initial_commit) { project.commit('3db7bd90bab8ce8f02c9818590b84739a2e97230') }
let(:text_to_symlink_commit) { project.commit('5e2c2708c2e403dece5dd25759369150aac51644') }
let(:branch_name) { 'diff-files-text-to-symlink' }
let(:file_name) { 'text-to-symlink.txt' }
context "when the position is on the added text file" do
let(:old_file_status) { :new }
context "when the text file gets changed to a symlink between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit.parent, initial_commit) }
let(:new_diff_refs) { diff_refs(initial_commit.parent, text_to_symlink_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, text_to_symlink_commit) }
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
end
end
end
end
end
......@@ -29,6 +29,10 @@ module TestEnv
'gitattributes' => '5a62481',
'expand-collapse-diffs' => '4842455',
'symlink-expand-diff' => '81e6355',
'diff-files-symlink-to-image' => '8cfca84',
'diff-files-image-to-symlink' => '3e94fda',
'diff-files-symlink-to-text' => '689815e',
'diff-files-text-to-symlink' => '5e2c270',
'expand-collapse-files' => '025db92',
'expand-collapse-lines' => '238e82d',
'pages-deploy' => '7897d5b',
......
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