Merge branch '57793-fix-line-age' into 'master'

Support note position tracing on an image

See merge request gitlab-org/gitlab-ce!30158

app/services/discussions/update_diff_position_service.rb

@@ -3,7 +3,8 @@
 module Discussions
   class UpdateDiffPositionService < BaseService
     def execute(discussion)
-      result = tracer.trace(discussion.position)
+      old_position = discussion.position
+      result = tracer.trace(old_position)
       return unless result
 
       position = result[:position]

app/services/system_note_service.rb

@@ -268,11 +268,13 @@ module SystemNoteService
     merge_request = discussion.noteable
     diff_refs = change_position.diff_refs
     version_index = merge_request.merge_request_diffs.viewable.count
+    position_on_text = change_position.on_text?
+    text_parts = ["changed this #{position_on_text ? 'line' : 'file'} in"]
 
-    text_parts = ["changed this line in"]
     if version_params = merge_request.version_params_for(diff_refs)
-      line_code = change_position.line_code(project.repository)
+      repository = project.repository
-      url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: line_code))
+      anchor = position_on_text ? change_position.line_code(repository) : change_position.file_hash
+      url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: anchor))
 
       text_parts << "[version #{version_index} of the diff](#{url})"
     else

changelogs/unreleased/57793-fix-line-age.yml

@@ -0,0 +1,5 @@
+---
+title: Support note position tracing on an image
+merge_request: 30158
+author:
+type: fixed

lib/gitlab/diff/position.rb

@@ -134,6 +134,10 @@ module Gitlab
         @line_code ||= diff_file(repository)&.line_code_for_position(self)
       end
 
+      def file_hash
+        @file_hash ||= Digest::SHA1.hexdigest(file_path)
+      end
+
       def on_image?
         position_type == 'image'
       end

lib/gitlab/diff/position_tracer.rb

@@ -17,187 +17,13 @@ module Gitlab
         @paths = paths
       end
 
-      def trace(ab_position)
+      def trace(old_position)
         return unless old_diff_refs&.complete? && new_diff_refs&.complete?
-        return unless ab_position.diff_refs == old_diff_refs
+        return unless old_position.diff_refs == old_diff_refs
 
-        # Suppose we have an MR with source branch `feature` and target branch `master`.
+        strategy = old_position.on_text? ? LineStrategy : ImageStrategy
-        # When the MR was created, the head of `master` was commit A, and the
-        # head of `feature` was commit B, resulting in the original diff A->B.
-        # Since creation, `master` was updated to C.
-        # Now `feature` is being updated to D, and the newly generated MR diff is C->D.
-        # It is possible that C and D are direct descendants of A and B respectively,
-        # but this isn't necessarily the case as rebases and merges come into play.
-        #
-        # Suppose we have a diff note on the original diff A->B. Now that the MR
-        # is updated, we need to find out what line in C->D corresponds to the
-        # line the note was originally created on, so that we can update the diff note's
-        # records and continue to display it in the right place in the diffs.
-        # If we cannot find this line in the new diff, this means the diff note is now
-        # outdated, and we will display that fact to the user.
-        #
-        # In the new diff, the file the diff note was originally created on may
-        # have been renamed, deleted or even created, if the file existed in A and B,
-        # but was removed in C, and restored in D.
-        #
-        # Every diff note stores a Position object that defines a specific location,
-        # identified by paths and line numbers, within a specific diff, identified
-        # by start, head and base commit ids.
-        #
-        # For diff notes for diff A->B, the position looks like this:
-        # Position
-        #   start_sha - ID of commit A
-        #   head_sha - ID of commit B
-        #   base_sha - ID of base commit of A and B
-        #   old_path - path as of A (nil if file was newly created)
-        #   new_path - path as of B (nil if file was deleted)
-        #   old_line - line number as of A (nil if file was newly created)
-        #   new_line - line number as of B (nil if file was deleted)
-        #
-        # We can easily update `start_sha` and `head_sha` to hold the IDs of
-        # commits C and D, and can trivially determine `base_sha` based on those,
-        # but need to find the paths and line numbers as of C and D.
-        #
-        # If the file was unchanged or newly created in A->B, the path as of D can be found
-        # by generating diff B->D ("head to head"), finding the diff file with
-        # `diff_file.old_path == position.new_path`, and taking `diff_file.new_path`.
-        # The path as of C can be found by taking diff C->D, finding the diff file
-        # with that same `new_path` and taking `diff_file.old_path`.
-        # The line number as of D can be found by using the LineMapper on diff B->D
-        # and providing the line number as of B.
-        # The line number as of C can be found by using the LineMapper on diff C->D
-        # and providing the line number as of D.
-        #
-        # If the file was deleted in A->B, the path as of C can be found
-        # by generating diff A->C ("base to base"), finding the diff file with
-        # `diff_file.old_path == position.old_path`, and taking `diff_file.new_path`.
-        # The path as of D can be found by taking diff C->D, finding the diff file
-        # with `old_path` set to that `diff_file.new_path` and taking `diff_file.new_path`.
-        # The line number as of C can be found by using the LineMapper on diff A->C
-        # and providing the line number as of A.
-        # The line number as of D can be found by using the LineMapper on diff C->D
-        # and providing the line number as of C.
 
-        if ab_position.added?
+        strategy.new(self).trace(old_position)
-          trace_added_line(ab_position)
-        elsif ab_position.removed?
-          trace_removed_line(ab_position)
-        else # unchanged
-          trace_unchanged_line(ab_position)
-        end
-      end
-
-      private
-
-      def trace_added_line(ab_position)
-        b_path = ab_position.new_path
-        b_line = ab_position.new_line
-
-        bd_diff = bd_diffs.diff_file_with_old_path(b_path)
-
-        d_path = bd_diff&.new_path || b_path
-        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)
-
-          c_path = cd_diff&.old_path || d_path
-          c_line = LineMapper.new(cd_diff).new_to_old(d_line)
-
-          if c_line
-            # If the line is still in D but also in C, it has turned from an
-            # added line into an unchanged one.
-            new_position = position(cd_diff, c_line, d_line)
-            if valid_position?(new_position)
-              # If the line is still in the MR, we don't treat this as outdated.
-              { position: new_position, outdated: false }
-            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)
-
-              { position: position(ac_diff, nil, c_line), outdated: true }
-            end
-          else
-            # If the line is still in D and not in C, it is still added.
-            { position: position(cd_diff, nil, d_line), outdated: false }
-          end
-        else
-          # If the line is no longer in D, it has been removed from the MR.
-          { position: position(bd_diff, b_line, nil), outdated: true }
-        end
-      end
-
-      def trace_removed_line(ab_position)
-        a_path = ab_position.old_path
-        a_line = ab_position.old_line
-
-        ac_diff = ac_diffs.diff_file_with_old_path(a_path)
-
-        c_path = ac_diff&.new_path || a_path
-        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)
-
-          d_path = cd_diff&.new_path || c_path
-          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)
-
-            { position: position(bd_diff, nil, d_line), outdated: true }
-          else
-            # If the line is still in C and not in D, it is still removed.
-            { position: position(cd_diff, c_line, nil), outdated: false }
-          end
-        else
-          # If the line is no longer in C, it has been removed outside of the MR.
-          { position: position(ac_diff, a_line, nil), outdated: true }
-        end
-      end
-
-      def trace_unchanged_line(ab_position)
-        a_path = ab_position.old_path
-        a_line = ab_position.old_line
-        b_path = ab_position.new_path
-        b_line = ab_position.new_line
-
-        ac_diff = ac_diffs.diff_file_with_old_path(a_path)
-
-        c_path = ac_diff&.new_path || a_path
-        c_line = LineMapper.new(ac_diff).old_to_new(a_line)
-
-        bd_diff = bd_diffs.diff_file_with_old_path(b_path)
-
-        d_line = LineMapper.new(bd_diff).old_to_new(b_line)
-
-        cd_diff = cd_diffs.diff_file_with_old_path(c_path)
-
-        if c_line && d_line
-          # If the line is still in C and D, it is still unchanged.
-          new_position = position(cd_diff, c_line, d_line)
-          if valid_position?(new_position)
-            # If the line is still in the MR, we don't treat this as outdated.
-            { position: new_position, outdated: false }
-          else
-            # If the line is no longer in the MR, we unfortunately cannot show
-            # the current state on the CD diff or any change on the BD diff,
-            # so we treat it as outdated.
-            { position: nil, outdated: true }
-          end
-        elsif d_line # && !c_line
-          # If the line is still in D but no longer in C, it has turned from
-          # an unchanged line into an added one.
-          # We don't treat this as outdated since the line is still in the MR.
-          { position: position(cd_diff, nil, d_line), outdated: false }
-        else # !d_line && (c_line || !c_line)
-          # If the line is no longer in D, it has turned from an unchanged line
-          # into a removed one.
-          { position: position(bd_diff, b_line, nil), outdated: true }
-        end
       end
 
       def ac_diffs
@@ -216,18 +42,12 @@ module Gitlab
         @cd_diffs ||= compare(new_diff_refs.start_sha, new_diff_refs.head_sha)
       end
 
+      private
+
       def compare(start_sha, head_sha, straight: false)
         compare = CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
         compare.diffs(paths: paths, expanded: true)
       end
-
-      def position(diff_file, old_line, new_line)
-        Position.new(diff_file: diff_file, old_line: old_line, new_line: new_line)
-      end
-
-      def valid_position?(position)
-        !!position.diff_line(project.repository)
-      end
     end
   end
 end

lib/gitlab/diff/position_tracer/base_strategy.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Diff
+    class PositionTracer
+      class BaseStrategy
+        attr_reader :tracer
+
+        delegate \
+          :project,
+          :ac_diffs,
+          :bd_diffs,
+          :cd_diffs,
+          to: :tracer
+
+        def initialize(tracer)
+          @tracer = tracer
+        end
+
+        def trace(position)
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end

lib/gitlab/diff/position_tracer/image_strategy.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Diff
+    class PositionTracer
+      class ImageStrategy < BaseStrategy
+        def trace(position)
+          b_path = position.new_path
+
+          # 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)
+
+          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)
+
+          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)
+
+          return { position: new_position(position, ac_diff), outdated: true } if ac_diff
+
+          # If ever there's a case that the file no longer exists in any diff,
+          # don't set a change position and let the note become outdated.
+          #
+          # This should never happen given the file should exist in one of the
+          # diffs above.
+          { outdated: true }
+        end
+
+        private
+
+        def new_position(position, diff_file)
+          Position.new(
+            diff_file: diff_file,
+            x: position.x,
+            y: position.y,
+            width: position.width,
+            height: position.height,
+            position_type: position.position_type
+          )
+        end
+      end
+    end
+  end
+end

lib/gitlab/diff/position_tracer/line_strategy.rb

@@ -0,0 +1,201 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Diff
+    class PositionTracer
+      class LineStrategy < BaseStrategy
+        def trace(position)
+          # Suppose we have an MR with source branch `feature` and target branch `master`.
+          # When the MR was created, the head of `master` was commit A, and the
+          # head of `feature` was commit B, resulting in the original diff A->B.
+          # Since creation, `master` was updated to C.
+          # Now `feature` is being updated to D, and the newly generated MR diff is C->D.
+          # It is possible that C and D are direct descendants of A and B respectively,
+          # but this isn't necessarily the case as rebases and merges come into play.
+          #
+          # Suppose we have a diff note on the original diff A->B. Now that the MR
+          # is updated, we need to find out what line in C->D corresponds to the
+          # line the note was originally created on, so that we can update the diff note's
+          # records and continue to display it in the right place in the diffs.
+          # If we cannot find this line in the new diff, this means the diff note is now
+          # outdated, and we will display that fact to the user.
+          #
+          # In the new diff, the file the diff note was originally created on may
+          # have been renamed, deleted or even created, if the file existed in A and B,
+          # but was removed in C, and restored in D.
+          #
+          # Every diff note stores a Position object that defines a specific location,
+          # identified by paths and line numbers, within a specific diff, identified
+          # by start, head and base commit ids.
+          #
+          # For diff notes for diff A->B, the position looks like this:
+          # Position
+          #   start_sha - ID of commit A
+          #   head_sha - ID of commit B
+          #   base_sha - ID of base commit of A and B
+          #   old_path - path as of A (nil if file was newly created)
+          #   new_path - path as of B (nil if file was deleted)
+          #   old_line - line number as of A (nil if file was newly created)
+          #   new_line - line number as of B (nil if file was deleted)
+          #
+          # We can easily update `start_sha` and `head_sha` to hold the IDs of
+          # commits C and D, and can trivially determine `base_sha` based on those,
+          # but need to find the paths and line numbers as of C and D.
+          #
+          # If the file was unchanged or newly created in A->B, the path as of D can be found
+          # by generating diff B->D ("head to head"), finding the diff file with
+          # `diff_file.old_path == position.new_path`, and taking `diff_file.new_path`.
+          # The path as of C can be found by taking diff C->D, finding the diff file
+          # with that same `new_path` and taking `diff_file.old_path`.
+          # The line number as of D can be found by using the LineMapper on diff B->D
+          # and providing the line number as of B.
+          # The line number as of C can be found by using the LineMapper on diff C->D
+          # and providing the line number as of D.
+          #
+          # If the file was deleted in A->B, the path as of C can be found
+          # by generating diff A->C ("base to base"), finding the diff file with
+          # `diff_file.old_path == position.old_path`, and taking `diff_file.new_path`.
+          # The path as of D can be found by taking diff C->D, finding the diff file
+          # with `old_path` set to that `diff_file.new_path` and taking `diff_file.new_path`.
+          # The line number as of C can be found by using the LineMapper on diff A->C
+          # and providing the line number as of A.
+          # The line number as of D can be found by using the LineMapper on diff C->D
+          # and providing the line number as of C.
+
+          if position.added?
+            trace_added_line(position)
+          elsif position.removed?
+            trace_removed_line(position)
+          else # unchanged
+            trace_unchanged_line(position)
+          end
+        end
+
+        private
+
+        def trace_added_line(position)
+          b_path = position.new_path
+          b_line = position.new_line
+
+          bd_diff = bd_diffs.diff_file_with_old_path(b_path)
+
+          d_path = bd_diff&.new_path || b_path
+          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)
+
+            c_path = cd_diff&.old_path || d_path
+            c_line = LineMapper.new(cd_diff).new_to_old(d_line)
+
+            if c_line
+              # If the line is still in D but also in C, it has turned from an
+              # added line into an unchanged one.
+              new_position = new_position(cd_diff, c_line, d_line)
+              if valid_position?(new_position)
+                # If the line is still in the MR, we don't treat this as outdated.
+                { position: new_position, outdated: false }
+              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)
+
+                { position: new_position(ac_diff, nil, c_line), outdated: true }
+              end
+            else
+              # If the line is still in D and not in C, it is still added.
+              { position: new_position(cd_diff, nil, d_line), outdated: false }
+            end
+          else
+            # If the line is no longer in D, it has been removed from the MR.
+            { position: new_position(bd_diff, b_line, nil), outdated: true }
+          end
+        end
+
+        def trace_removed_line(position)
+          a_path = position.old_path
+          a_line = position.old_line
+
+          ac_diff = ac_diffs.diff_file_with_old_path(a_path)
+
+          c_path = ac_diff&.new_path || a_path
+          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)
+
+            d_path = cd_diff&.new_path || c_path
+            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)
+
+              { position: new_position(bd_diff, nil, d_line), outdated: true }
+            else
+              # If the line is still in C and not in D, it is still removed.
+              { position: new_position(cd_diff, c_line, nil), outdated: false }
+            end
+          else
+            # If the line is no longer in C, it has been removed outside of the MR.
+            { position: new_position(ac_diff, a_line, nil), outdated: true }
+          end
+        end
+
+        def trace_unchanged_line(position)
+          a_path = position.old_path
+          a_line = position.old_line
+          b_path = position.new_path
+          b_line = position.new_line
+
+          ac_diff = ac_diffs.diff_file_with_old_path(a_path)
+
+          c_path = ac_diff&.new_path || a_path
+          c_line = LineMapper.new(ac_diff).old_to_new(a_line)
+
+          bd_diff = bd_diffs.diff_file_with_old_path(b_path)
+
+          d_line = LineMapper.new(bd_diff).old_to_new(b_line)
+
+          cd_diff = cd_diffs.diff_file_with_old_path(c_path)
+
+          if c_line && d_line
+            # If the line is still in C and D, it is still unchanged.
+            new_position = new_position(cd_diff, c_line, d_line)
+            if valid_position?(new_position)
+              # If the line is still in the MR, we don't treat this as outdated.
+              { position: new_position, outdated: false }
+            else
+              # If the line is no longer in the MR, we unfortunately cannot show
+              # the current state on the CD diff or any change on the BD diff,
+              # so we treat it as outdated.
+              { position: nil, outdated: true }
+            end
+          elsif d_line # && !c_line
+            # If the line is still in D but no longer in C, it has turned from
+            # an unchanged line into an added one.
+            # We don't treat this as outdated since the line is still in the MR.
+            { position: new_position(cd_diff, nil, d_line), outdated: false }
+          else # !d_line && (c_line || !c_line)
+            # If the line is no longer in D, it has turned from an unchanged line
+            # into a removed one.
+            { position: new_position(bd_diff, b_line, nil), outdated: true }
+          end
+        end
+
+        def new_position(diff_file, old_line, new_line)
+          Position.new(
+            diff_file: diff_file,
+            old_line: old_line,
+            new_line: new_line
+          )
+        end
+
+        def valid_position?(position)
+          !!position.diff_line(project.repository)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/diff/position_spec.rb

@@ -610,4 +610,17 @@ describe Gitlab::Diff::Position do
       it_behaves_like "diff position json"
     end
   end
+
+  describe "#file_hash" do
+    subject do
+      described_class.new(
+        old_path: "image.jpg",
+        new_path: "image.jpg"
+      )
+    end
+
+    it "returns SHA1 representation of the file_path" do
+      expect(subject.file_hash).to eq(Digest::SHA1.hexdigest(subject.file_path))
+    end
+  end
 end

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

@@ -0,0 +1,238 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Diff::PositionTracer::ImageStrategy do
+  include PositionTracerHelpers
+
+  let(:project) { create(:project, :repository) }
+  let(:current_user) { project.owner }
+  let(:file_name) { 'test-file' }
+  let(:new_file_name) { "#{file_name}-new" }
+  let(:second_file_name) { "#{file_name}-2" }
+  let(:branch_name) { 'position-tracer-test' }
+  let(:old_position) { position(old_path: file_name, new_path: file_name, position_type: 'image') }
+
+  let(:tracer) do
+    Gitlab::Diff::PositionTracer.new(
+      project: project,
+      old_diff_refs: old_diff_refs,
+      new_diff_refs: new_diff_refs
+    )
+  end
+
+  let(:strategy) { described_class.new(tracer) }
+
+  subject { strategy.trace(old_position) }
+
+  let(:initial_commit) do
+    project.commit(create_branch(branch_name, 'master')[:branch].name)
+  end
+
+  describe '#trace' do
+    describe 'diff scenarios' do
+      let(:create_file_commit) do
+        initial_commit
+
+        create_file(
+          branch_name,
+          file_name,
+          Base64.encode64('content')
+        )
+      end
+
+      let(:update_file_commit) do
+        create_file_commit
+
+        update_file(
+          branch_name,
+          file_name,
+          Base64.encode64('updatedcontent')
+        )
+      end
+
+      let(:update_file_again_commit) do
+        update_file_commit
+
+        update_file(
+          branch_name,
+          file_name,
+          Base64.encode64('updatedcontentagain')
+        )
+      end
+
+      let(:delete_file_commit) do
+        create_file_commit
+        delete_file(branch_name, file_name)
+      end
+
+      let(:rename_file_commit) do
+        delete_file_commit
+
+        create_file(
+          branch_name,
+          new_file_name,
+          Base64.encode64('renamedcontent')
+        )
+      end
+
+      let(:create_second_file_commit) do
+        create_file_commit
+
+        create_file(
+          branch_name,
+          second_file_name,
+          Base64.encode64('morecontent')
+        )
+      end
+
+      let(:create_another_file_commit) do
+        create_file(
+          branch_name,
+          second_file_name,
+          Base64.encode64('morecontent')
+        )
+      end
+
+      let(:update_another_file_commit) do
+        update_file(
+          branch_name,
+          second_file_name,
+          Base64.encode64('updatedmorecontent')
+        )
+      end
+
+      context 'when the file was created in the old diff' do
+        context 'when the file is unchanged between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(initial_commit, create_file_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 file was updated between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+          let(:new_diff_refs) { diff_refs(initial_commit, update_file_commit) }
+          let(:change_diff_refs) { diff_refs(create_file_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 file was renamed in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+          let(:new_diff_refs) { diff_refs(initial_commit, rename_file_commit) }
+          let(:change_diff_refs) { diff_refs(create_file_commit, rename_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 file was removed in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+          let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) }
+          let(:change_diff_refs) { diff_refs(create_file_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
+
+        context 'when the file is unchanged in the new diff' do
+          let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_another_file_commit, update_another_file_commit) }
+          let(:change_diff_refs) { diff_refs(initial_commit, create_another_file_commit) }
+
+          it 'returns the position of the change' do
+            expect_change_position(
+              old_path: file_name,
+              new_path: file_name
+            )
+          end
+        end
+      end
+
+      context 'when the file was changed in the old diff' do
+        context 'when the file is unchanged in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_file_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 file was updated in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
+          let(:change_diff_refs) { diff_refs(update_file_commit, update_file_again_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 file was renamed in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_file_commit, rename_file_commit) }
+          let(:change_diff_refs) { diff_refs(update_file_commit, rename_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 file was removed in between the old and the new diff' do
+          let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_file_commit, delete_file_commit) }
+          let(:change_diff_refs) { diff_refs(update_file_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
+
+        context 'when the file is unchanged in the new diff' do
+          let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+          let(:new_diff_refs) { diff_refs(create_another_file_commit, update_another_file_commit) }
+          let(:change_diff_refs) { diff_refs(create_file_commit, create_another_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
+  end
+end

spec/services/system_note_service_spec.rb

@@ -1175,16 +1175,30 @@ describe SystemNoteService do
       end
 
       it 'links to the diff in the system note' do
-        expect(subject.note).to include('version 1')
-
         diff_id = merge_request.merge_request_diff.id
         line_code = change_position.line_code(project.repository)
-        expect(subject.note).to include(diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: line_code))
+        link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: line_code)
+
+        expect(subject.note).to eq("changed this line in [version 1 of the diff](#{link})")
+      end
+
+      context 'discussion is on an image' do
+        let(:discussion) { create(:image_diff_note_on_merge_request, project: project).to_discussion }
+
+        it 'links to the diff in the system note' do
+          diff_id = merge_request.merge_request_diff.id
+          file_hash = change_position.file_hash
+          link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: file_hash)
+
+          expect(subject.note).to eq("changed this file in [version 1 of the diff](#{link})")
+        end
       end
     end
 
-    context 'when the change_position is invalid for the discussion' do
+    context 'when the change_position does not point to a valid version' do
-      let(:change_position) { project.commit(sample_commit.id) }
+      before do
+        allow(merge_request).to receive(:version_params_for).and_return(nil)
+      end
 
       it 'creates a new note in the discussion' do
         # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.

spec/support/helpers/position_tracer_helpers.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true
+
+module PositionTracerHelpers
+  def diff_refs(base_commit, head_commit)
+    Gitlab::Diff::DiffRefs.new(base_sha: base_commit.id, head_sha: head_commit.id)
+  end
+
+  def position(attrs = {})
+    attrs.reverse_merge!(
+      diff_refs: old_diff_refs
+    )
+    Gitlab::Diff::Position.new(attrs)
+  end
+
+  def expect_new_position(attrs, result = subject)
+    aggregate_failures("expect new position #{attrs.inspect}") do
+      if attrs.nil?
+        expect(result[:outdated]).to be_truthy
+      else
+        new_position = result[:position]
+
+        expect(result[:outdated]).to be_falsey
+        expect(new_position).not_to be_nil
+        expect(new_position.diff_refs).to eq(new_diff_refs)
+
+        attrs.each do |attr, value|
+          expect(new_position.send(attr)).to eq(value)
+        end
+      end
+    end
+  end
+
+  def expect_change_position(attrs, result = subject)
+    aggregate_failures("expect change position #{attrs.inspect}") do
+      change_position = result[:position]
+
+      expect(result[:outdated]).to be_truthy
+
+      if attrs.nil? || attrs.empty?
+        expect(change_position).to be_nil
+      else
+        expect(change_position).not_to be_nil
+        expect(change_position.diff_refs).to eq(change_diff_refs)
+
+        attrs.each do |attr, value|
+          expect(change_position.send(attr)).to eq(value)
+        end
+      end
+    end
+  end
+
+  def create_branch(new_name, branch_name)
+    CreateBranchService.new(project, current_user).execute(new_name, branch_name)
+  end
+
+  def create_file(branch_name, file_name, content)
+    Files::CreateService.new(
+      project,
+      current_user,
+      start_branch: branch_name,
+      branch_name: branch_name,
+      commit_message: "Create file",
+      file_path: file_name,
+      file_content: content
+    ).execute
+    project.commit(branch_name)
+  end
+
+  def update_file(branch_name, file_name, content)
+    Files::UpdateService.new(
+      project,
+      current_user,
+      start_branch: branch_name,
+      branch_name: branch_name,
+      commit_message: "Update file",
+      file_path: file_name,
+      file_content: content
+    ).execute
+    project.commit(branch_name)
+  end
+
+  def delete_file(branch_name, file_name)
+    Files::DeleteService.new(
+      project,
+      current_user,
+      start_branch: branch_name,
+      branch_name: branch_name,
+      commit_message: "Delete file",
+      file_path: file_name
+    ).execute
+    project.commit(branch_name)
+  end
+end