Merge branch 'dm-fix-position-tracer-for-hidden-lines' into 'master'

Fix commenting on an existing discussion on an unchanged line that is no longer in the diff

Closes #31339

See merge request !10888

3 files changed, 15 insertions, 8 deletions

diff --git a/changelogs/unreleased/dm-fix-position-tracer-for-hidden-lines.yml b/changelogs/unreleased/dm-fix-position-tracer-for-hidden-lines.yml
new file mode 100644
index 00000000000..d9ba26a0657
--- /dev/null
+++ b/changelogs/unreleased/dm-fix-position-tracer-for-hidden-lines.yml
@@ -0,0 +1,5 @@
+---
+title: Fix commenting on an existing discussion on an unchanged line that is no longer
+  in the diff
+merge_request:
+author:
diff --git a/lib/gitlab/diff/position_tracer.rb b/lib/gitlab/diff/position_tracer.rb
index 4d04f867268..c7542a8fabc 100644
--- a/lib/gitlab/diff/position_tracer.rb
+++ b/lib/gitlab/diff/position_tracer.rb
@@ -82,7 +82,7 @@ module Gitlab
 
         file_diff, old_line, new_line = results
 
-        Position.new(
+        new_position = Position.new(
           old_path: file_diff.old_path,
           new_path: file_diff.new_path,
           head_sha: new_diff_refs.head_sha,
@@ -91,6 +91,13 @@ module Gitlab
           old_line: old_line,
           new_line: new_line
         )
+
+        # If a position is found, but is not actually contained in the diff, for example
+        # because it was an unchanged line in the context of a change that was undone,
+        # we cannot return this as a successful trace.
+        return unless new_position.diff_line(repository)
+
+        new_position
       end
 
       private
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index c166f83664a..a10a251dc4a 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -569,13 +569,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do
               # 1 1   BB
               # 2 2   A
 
-              it "returns the new position" do
-                expect_new_position(
-                  old_path: file_name,
-                  new_path: new_file_name,
-                  old_line: old_position.new_line,
-                  new_line: old_position.new_line
-                )
+              it "returns nil since the line doesn't exist in the new diffs anymore" do
+                expect(subject).to be_nil
               end
             end