Merge branch 'use-file-identifier-hash-for-position-tracing' into 'master'

Use file_mode to guide position tracing

See merge request gitlab-org/gitlab!34375

lib/gitlab/diff/file_collection/base.rb

@@ -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

lib/gitlab/diff/position_tracer.rb

@@ -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)

lib/gitlab/diff/position_tracer/base_strategy.rb

@@ -8,6 +8,7 @@ module Gitlab
 
         delegate \
           :project,
+          :diff_file,
           :ac_diffs,
           :bd_diffs,
           :cd_diffs,

lib/gitlab/diff/position_tracer/image_strategy.rb

@@ -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
 

lib/gitlab/diff/position_tracer/line_strategy.rb

@@ -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.

spec/lib/gitlab/diff/position_tracer/image_strategy_spec.rb

@@ -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

spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb

@@ -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

spec/support/helpers/test_env.rb

@@ -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',