Subject: Re: bug#72556: 29.1.90; vc-diff does not undo hunk in end of source file

From: Tomas Nordin <[email-masked]>
Date: Sat, 01 Mar 2025 14:28:04 +0000

Juri Linkov <[email-masked]> writes:

>> Juri and Dmitry, any comments or suggestions?
>
> I don't know why it fails at the end, so I tried to debug 'diff-apply-hunk',
> and found that the problem is in these lines of 'diff-find-source-location':
>
>     ;; FIXME: Check for case where both OLD and NEW are found.
>     (pos (or (diff-find-text (car old))
>
> When the hunk is at the beginning/end of the diff buffer,
> then it misses the top/bottom part of the context,
> so 'diff-find-text' wrongly matches it because it fails
> to see any changes in it due to insufficient context,
> and returns switched=nil.

Here is some notes from a bug-hunt (mabye a glorified variant of what
you are already saying Juri).  Perspective is from
'diff-find-source-location' in diff-mode.el

The call (diff-find-text (car old)):
    OLD is text derived from hunk representing text in the source file
    before the edits. => nil if not found in the source file

The call (diff-find-text (car new)):
    NEW is text derived from hunk representing text in the source file
    after the edits. => nil if not found in the source file

Assumptions made on result from applying 'diff-find-text' on

NEW: if non-nil, hunk is already applied
OLD: if non-nil, hunk is not applied

When the edited line(s) have context only above or below itself in the
hunk, the application of diff-find-text on OLD will always yield
non-nil, because OLD is then always found in the source file.

The application of diff-find-text on NEW shouldn't have similar
problems.

There is a comment requesting a fix for when both OLD and NEW are
found.  Maybe this was the case concerned.

The variable SWITCHED, then, should be non-nil when hunk is already
applied, and is a prerequisite for the DWIM behavior to work (the
bug).  It is not set to t when 'diff-find-text' is applied on OLD (the
first or:ed attempt), which returns non-nil.

The attached patch of diff-mode.el makes the assumption that hunk is
already applied when both OLD and NEW yields non-nil, and checks if
this is the case as the first or:ed attempt.  Optimistically assumes
that this was the requested FIXME and so removes the FIXME comment.

Dmitri had concerns about drawbacks when splitting a hunk.  I don't have
successful experience of that operation.  Can Dmitri maybe check so that
this patch don't produce such drawbacks?

>From b78bb6d6b9757dc937b35007c1592628de9cc8cf Mon Sep 17 00:00:00 2001
From: Tomas Nordin <[email-masked]>
Date: Sat, 1 Mar 2025 14:50:36 +0100
Subject: [PATCH] Fix detection of hunk already applied

* lisp/vc/diff-mode.el (diff-find-source-location):
Consider the case when there is no diff context above or below
edited lines.  (bug#72556)
---
 lisp/vc/diff-mode.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 16e696b6609..d11cd521a1e 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1996,9 +1996,11 @@ diff-find-source-location
         (goto-char (point-min)) (forward-line (1- (string-to-number line)))
 	(let* ((orig-pos (point))
 	       (switched nil)
-	       ;; FIXME: Check for case where both OLD and NEW are found.
-	       (pos (or (diff-find-text (car old))
-			(progn (setq switched t) (diff-find-text (car new)))
+	       (maybe-old (diff-find-text (car old)))
+	       (maybe-new (diff-find-text (car new)))
+	       (pos (or (and maybe-old maybe-new (setq switched t) maybe-new)
+                        maybe-old
+			(progn (setq switched t) maybe-new)
 			(progn (setq switched nil)
 			       (condition-case nil
 				   (diff-find-approx-text (car old))
-- 
2.39.5