GNU bug report logs - #62731
29.0.60; diff-apply-hunk doesn't work for creating new files

Previous Next

Full log

Message #43 received at 62731 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>
Cc: sbaugh <at> catern.com, 62731 <at> debbugs.gnu.org
Subject: Re: bug#62731: 29.0.60; diff-apply-hunk doesn't work for creating new
 files
Date: Sat, 19 Oct 2024 02:29:51 +0100

On 17/10/2024 20:56, Juri Linkov wrote:
>>>>> All right, the attached seems to support both creation and deletion,
>>>>> including applying hunks in reverse direction.
>>>>> Things got trickier but not by a lot.
>>>>
>>>> Now pushed to master, seems useful enough. Let's see if some unforeseen
>>>> problems are reported.
>>>
>>> This change broke diff of files:
>>>
>>> @@ -1957,7 +1970,7 @@ diff-find-source-location
>>>                                    diff-context-mid-hunk-header-re nil t)
>>>    			 (error "Can't find the hunk separator"))
>>>    		       (match-string 1)))))
>>> -	   (file (or (diff-find-file-name other noprompt)
>>> +	   (file (or (diff-find-file-name (xor other reverse) noprompt)
>>>                         (error "Can't find the file")))
>>>    	   (revision (and other diff-vc-backend
>>>                              (if reverse (nth 1 diff-vc-revisions)
>>>
>>> So after 'dired-backup-diff', typing 'C-c C-c' visits wrong file:
>>> visits the backup when point is on the file line, and vice versa:
>>> visits the file when point is on backup file line.
>>
>> Thanks for reporting.
>>
>> Do you think we can fix it this way?
>>
>> diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
>> index cfa90d380ad..374df3ee2cb 100644
>> --- a/lisp/vc/diff-mode.el
>> +++ b/lisp/vc/diff-mode.el
>> @@ -2196,7 +2196,7 @@ diff-goto-source
>>      ;; This is a convenient detail when using smerge-diff.
>>      (if event (posn-set-point (event-end event)))
>>      (let ((buffer (when event (current-buffer)))
>> -        (reverse (not (save-excursion (beginning-of-line) (looking-at "[-<]")))))
>> +        (reverse (not (save-excursion (beginning-of-line) (looking-at "[+<]")))))
>>        (pcase-let ((`(,buf ,_line-offset ,pos ,src ,_dst ,_switched)
>>                     (diff-find-source-location other-file reverse)))
>>          (pop-to-buffer buf)
> 
> I don't understand this change.

It seemed logical that if the diff is between two different files, then 
reversing the direction would make the "old" file the current one.

I suppose that might be true only in some cases (e.g. creation or 
deletion diffs, or files of equal importance), but not when the 
comparison base is the backup file.

>> Otherwise, I don't quite understand the intent behind that line. But it
>> might have been masking the problem which I (hopefully) fixed in the
>> hunk you quoted/
> 
> There is already one 'xor' here.  Maybe the second 'xor' not needed:
> 
> (defun diff-find-source-location (&optional other-file reverse noprompt)
>      ...
>      (let* ((other (xor other-file diff-jump-to-old-file))
>      ...
> 	   (file (or (diff-find-file-name (xor other reverse) noprompt)

Yes ok, let's try moving that to the caller. Pushed as commit 
1374f20491b. A few other callers use non-nil 'reverse', but I suppose 
diff-test-hunk is not very useful for file creation/deletion.

Note that the new (same as previous) behavior is to always visit the 
source buffer, newer the backup file's buffer. I hope that is the goal.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.