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

[Dmitry Gutov <dmitry@gutov.dev>]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

On 03/10/2024 08:47, Eli Zaretskii wrote:
>>> If Hg doesn't prepend fake leading directories, we don't need to be
>>> bothered by Hg.
>>
>> It does. A fuller example, with deletion:

This was file creation, btw. Just to keep the things clear.

>> diff -r d045d1125783 -r 9396bae6ff0d CLOBBER.new
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/CLOBBER.new	Fri Dec 15 20:37:14 2023 +0200
>> @@ -0,0 +1,56 @@
> 
> OK, then does the presence of two -r options indicate Hg?  Or is that
> not guaranteed, either?

No guarantee I suppose, but I'm not aware of any others, yet.

And apparently 'hg diff --git' can also output

   diff --git a/a b/b

But that seems fine for our check.

>> I'm concerned the user is going to wonder whether anything happened at
>> all, and checking is a non-trivial action. But if you think this is
>> fine, I guess it's something to try.
> 
> Not sure I understand the problem.  The user instructed us to apply
> diffs, one of which deletes a file.  Why should we hesitate about
> deleting that file?

It's a destructive operation, not always easy to undo. The current edits 
might be saved to disk but not checked in, for example.

I suppose using a prompt could be enough, though.

>>>> Deleting files is something that one can do manually, though, so solving
>>>> this seems lower priority.
>>>
>>> When you apply a large set of diffs in which one file is deleted,
>>> there's no easy way of knowing you should deleted that file.
>>
>> In the current version of code you will be asked midway through a file
>> (or right away, when using diff-apply-hunk) to specify a file name,
>> defaulting to /dev/null, and after you press C-g after seeing the odd
>> prompt the hunk won't be applied. So it's hard to miss, at least.
> 
> Yes, but this is buggy behavior: there's no need to ask for a file
> name in this case.  Emacs is just confused by the part of the diffs
> which delete a file because the code doesn't take that into account.

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.

[-- Attachment #2: diff-apply-hunk-create-or-delete.diff --]
[-- Type: text/x-patch, Size: 4154 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index df55ca2ad80..4e227501883 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1091,13 +1091,24 @@ diff-find-file-name
 	      (diff-find-file-name old noprompt (match-string 1)))
          ;; if all else fails, ask the user
          (unless noprompt
-           (let ((file (expand-file-name (or (car fs) ""))))
+           (let ((file (or (car fs) ""))
+                 (creation (equal null-device
+                                  (car (diff-hunk-file-names (not old))))))
+             (when (and (memq diff-buffer-type '(git hg))
+                        (string-match "/" file))
+               ;; Strip the dst prefix (like b/) if diff is from Git/Hg.
+               (setq file (substring file (match-end 0))))
+             (setq file (expand-file-name file))
 	     (setq file
 		   (read-file-name (format "Use file %s: " file)
-				   (file-name-directory file) file t
+				   (file-name-directory file) file
+                                   ;; Allow non-matching for creation.
+                                   (not creation)
 				   (file-name-nondirectory file)))
-             (setq-local diff-remembered-files-alist
-                         (cons (cons fs file) diff-remembered-files-alist))
+             (when (or (not creation) (file-exists-p file))
+               ;; Only remember files that exist. User might have mistyped.
+               (setq-local diff-remembered-files-alist
+                           (cons (cons fs file) diff-remembered-files-alist)))
              file)))))))
 
 
@@ -1647,7 +1658,9 @@ diff-setup-buffer-type
     (setq-local diff-buffer-type
                 (if (re-search-forward "^diff --git" nil t)
                     'git
-                  nil)))
+                  (if (re-search-forward "^diff -r.*-r" nil t)
+                      'hg
+                    nil))))
   (when (eq diff-buffer-type 'git)
     (setq diff-outline-regexp
           (concat "\\(^diff --git.*\\|" diff-hunk-header-re "\\)")))
@@ -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)
@@ -2020,7 +2033,11 @@ diff-apply-hunk
 With a prefix argument, REVERSE the hunk."
   (interactive "P")
   (diff-beginning-of-hunk t)
-  (pcase-let ((`(,buf ,line-offset ,pos ,old ,new ,switched)
+  (pcase-let* (;; Do not accept BUFFER.REV buffers as source location.
+               (diff-vc-backend nil)
+               ;; When we detect deletion, we will use the old file name.
+               (deletion (equal null-device (car (diff-hunk-file-names reverse))))
+               (`(,buf ,line-offset ,pos ,old ,new ,switched)
                ;; Sometimes we'd like to have the following behavior: if
                ;; REVERSE go to the new file, otherwise go to the old.
                ;; But that means that by default we use the old file, which is
@@ -2030,7 +2047,7 @@ diff-apply-hunk
                ;; TODO: make it possible to ask explicitly for this behavior.
                ;;
                ;; This is duplicated in diff-test-hunk.
-               (diff-find-source-location nil reverse)))
+               (diff-find-source-location deletion reverse)))
     (cond
      ((null line-offset)
       (user-error "Can't find the text to patch"))
@@ -2056,6 +2073,10 @@ diff-apply-hunk
 		       "Hunk hasn't been applied yet; apply it now? "
 		     "Hunk has already been applied; undo it? ")))))
       (message "(Nothing done)"))
+     ((and deletion (not switched))
+      (when (y-or-n-p (format-message "Delete file `%s'?" (buffer-file-name buf)))
+        (delete-file (buffer-file-name buf) delete-by-moving-to-trash)
+        (kill-buffer buf)))
      (t
       ;; Apply the hunk
       (with-current-buffer buf