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

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

[sbaugh]

1. emacs -Q
2. Put the following content in a diff-mode buffer:
diff --git a/foo b/foo
new file mode 100644
--- /dev/null
+++ b/foo
@@ -0,0 +1,1 @@
+content
3. C-c C-a

Expected behavior: A file called "foo" with content "content" is
created.

Observed behavior: diff-mode prompts for the location of "b/foo", and
doesn't allow specifying the location as a non-existent file, meaning
the file can't actaully be created.



In GNU Emacs 29.0.60 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.16.0, Xaw3d scroll bars)
Repository revision: 4b6f2a7028b91128934a19f83572f24106782225
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: NixOS 21.11 (Porcupine)

Configured using:
 'configure
 --prefix=/nix/store/6d12l6xgg6bdqbv2l0k1nkpbixh93ib7-emacs-git-20220225.0
 --disable-build-details --with-modules --with-x-toolkit=lucid
 --with-xft --with-cairo'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XAW3D XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Diff

Minor modes in effect:
  whitespace-mode: t
  envrc-global-mode: t
  envrc-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  windmove-mode: t
  tracking-mode: t
  savehist-mode: t
  save-place-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/sbaugh/.emacs.d/elpa/transient-0.3.7/transient hides /home/sbaugh/.local/src/emacs29/lisp/transient

Features:
(emacs-news-mode shadow emacsbug vc-annotate vc-dir vc-filewise
mode-local pcvs pcvs-defs pcvs-parse pcvs-info conf-mode magit-bundle
magit-gitignore magit-subtree ibuf-ext ibuf-macs mule-diag dos-w32
find-cmd apropos finder autoinsert pcmpl-unix pcmpl-gnu make-mode ido
benchmark term ehelp eshell esh-cmd esh-ext esh-opt esh-proc esh-io
esh-arg esh-module esh-groups esh-util ibuffer ibuffer-loaddefs
package-x eat term/xterm xterm eat-autoloads tramp-adb tramp-container
tramp-ftp loadhist timezone rect ediff-vers debbugs-browse time
flow-fill qp sort smiley gnus-cite mail-extr gnus-async gnus-bcklg
gnus-agent gnus-srvr gnus-score score-mode nnvirtual nntp gnus-ml
gnus-msg disp-table nndoc gnus-cache gnus-dup debbugs-gnu debbugs-compat
debbugs soap-client rng-xsd xsd-regexp debbugs-autoloads tar-mode
arc-mode archive-mode forge-list forge-commands forge-semi
forge-bitbucket buck forge-gogs gogs forge-gitea gtea forge-gitlab glab
forge-github ghub-graphql treepy gsexp ghub forge-notify forge-revnote
forge-pullreq forge-issue forge-topic yaml forge-post let-alist
markdown-mode forge-repo forge forge-core forge-db closql emacsql-sqlite
emacsql emacsql-compiler emoji-labels emoji multisession sqlite
org-attach tramp-cmds tramp-cache time-stamp tramp-sh shr-color textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check
skeleton mhtml-mode css-mode js c-ts-common sgml-mode facemenu nix-mode
nix-repl nix-shell nix-store nix-log nix-instantiate nix-shebang
nix-format nix ediff-ptch magit-patch tramp-archive tramp-gvfs tramp
tramp-loaddefs trampver tramp-integration tramp-compat ls-lisp
cursor-sensor magit-bookmark bookmark man find-dired grep org-capture
ob-ditaa ob-plantuml org-clock org-colview org-crypt org-ctags org-habit
org-mouse org-plot org-protocol novice pulse color git-rebase canlock
view image-file image-converter rmail goto-addr whitespace eglot
external-completion array jsonrpc ert flymake-proc flymake warnings
diary-lib diary-loaddefs cal-iso cus-dep loaddefs-gen cus-theme oc-basic
ol-eww eww url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus
nnselect gnus-art mm-uu mml2015 mm-view mml-smime smime dig gnus-sum shr
pixel-fill kinsoku url-file svg dom gnus-group gnus-undo gnus-start
gnus-dbus dbus xml gnus-cloud nnimap nnmail mail-source utf7 nnoo
parse-time gnus-spec gnus-int gnus-range gnus-win gnus nnheader range
ol-docview doc-view jka-compr image-mode exif ol-bibtex bibtex iso8601
ol-bbdb ol-w3m ol-doi org-link-doi dired-aux sh-script smie executable
files-x tabify ggtags etags fileloop xref compile ewoc cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
debug backtrace cus-edit cus-start cus-load wid-edit lisp-mnt dabbrev pp
cl-print shortdoc completion help-fns radix-tree mm-archive
network-stream url-cache url-http url-auth url-gw nsm
display-line-numbers misc tmm mule-util misearch multi-isearch vc-hg
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs log-view vc bug-reference
magit-ediff ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help
ediff-init ediff-util vc-git vc-dispatcher face-remap ob-python python
pcase treesit agda2 envrc inheritenv page-ext dired-x magit-extras
project magit-submodule magit-obsolete magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
which-func imenu magit-diff smerge-mode diff git-commit log-edit message
sendmail yank-media dired dired-loaddefs rfc822 mml mml-sec epa derived
epg rfc6068 epg-config gnus-util text-property-search mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor shell server
magit-mode transient edmacro kmacro magit-git magit-section magit-utils
crm dash cl-extra windmove lui-autopaste circe advice diff-mode
lui-irc-colors irc gnutls puny lcs lui-logging lui-format lui tracking
shorten help-mode flyspell ispell circe-compat ox-odt rng-loc rng-uri
rng-parse rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns
nxml-enc xmltok nxml-util ox-latex ox-icalendar org-agenda ox-html table
ox-ascii ox-publish ox org-element org-persist xdg org-id org-refile
avl-tree generator org ob ob-tangle ob-ref ob-lob ob-table ob-exp
org-macro org-src ob-comint org-pcomplete pcomplete org-list
org-footnote org-faces org-entities time-date noutline outline icons
ob-emacs-lisp ob-core ob-eval org-cycle org-table ol rx org-fold
org-fold-core org-keys oc org-loaddefs find-func cal-menu calendar
cal-loaddefs org-version org-compat org-macs format-spec gdb-mi bindat
gud easy-mmode comint ansi-osc ansi-color ring ffap thingatpt
cyberpunk-theme savehist saveplace finder-inf envrc-autoloads
nix-mode-autoloads forge-autoloads htmlize-autoloads
slime-volleyball-autoloads graphviz-dot-mode-autoloads yaml-autoloads
auctex-autoloads tex-site notmuch-autoloads csv-mode-autoloads
ghub-autoloads treepy-autoloads circe-autoloads inheritenv-autoloads
mentor-autoloads url-scgi-autoloads xml-rpc-autoloads async-autoloads
ggtags-autoloads closql-autoloads emacsql-sqlite-autoloads
emacsql-autoloads magit-autoloads magit-section-autoloads
git-commit-autoloads with-editor-autoloads transient-autoloads
cyberpunk-theme-autoloads info dash-autoloads markdown-mode-autoloads
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie generate-lisp-file url-domsuf url-util mailcap
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
password-cache json subr-x map byte-opt gv bytecomp byte-compile
url-vars cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 5976716 623558)
 (symbols 48 79995 41)
 (strings 32 570566 317212)
 (string-bytes 1 39667351)
 (vectors 16 185988)
 (vector-slots 8 3509152 1022275)
 (floats 8 10130 8492)
 (intervals 56 578779 15775)
 (buffers 976 366))

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

[Dmitry Gutov]

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

Hi!

On 09/04/2023 04:14, sbaugh@catern.com wrote:
> 1. emacs -Q
> 2. Put the following content in a diff-mode buffer:
> diff --git a/foo b/foo
> new file mode 100644
> --- /dev/null
> +++ b/foo
> @@ -0,0 +1,1 @@
> +content
> 3. C-c C-a
> 
> Expected behavior: A file called "foo" with content "content" is
> created.
> 
> Observed behavior: diff-mode prompts for the location of "b/foo", and
> doesn't allow specifying the location as a non-existent file, meaning
> the file can't actaully be created.

This is annoying indeed.

The attached patch should handle this:

* When OLD equals to /dev/null, allow reading non-existing file name.
* When NEW starts with b/ or /a, slice that off if such dir does not exist.
* Bonus: when the diff is applied in reverse, the checked file names are 
switched. That helps undo deletions as well. Or renames.

It makes some assumptions, though, (such as that default-directory fits 
the file names in the diff, which is normal for vc diffs but maybe not 
others), so some testing would be welcome, especially from people who 
deal with diffs produced otherwise.

[-- Attachment #2: diff-find-file-name-new.diff --]
[-- Type: text/x-patch, Size: 2083 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 25c6238765d..98f77f1a1d7 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1055,13 +1055,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 (string-match-p "\\`[ab]/" file)
+                        (not (file-directory-p (substring file 0 1))))
+               ;; Strip the common prefix a/ or /b if no such dir exists.
+               (setq file (substring file 2)))
+             (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)))))))
 
 
@@ -1921,7 +1932,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)

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

[Eli Zaretskii]

> Date: Wed, 2 Oct 2024 03:41:05 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > Observed behavior: diff-mode prompts for the location of "b/foo", and
> > doesn't allow specifying the location as a non-existent file, meaning
> > the file can't actaully be created.
> 
> This is annoying indeed.
> 
> The attached patch should handle this:
> 
> * When OLD equals to /dev/null, allow reading non-existing file name.
> * When NEW starts with b/ or /a, slice that off if such dir does not exist.
> * Bonus: when the diff is applied in reverse, the checked file names are 
> switched. That helps undo deletions as well. Or renames.

I think Git uses other prefixes as well (something like i/ and r/,
perhaps?)  And relying on b/ being an existing directory can cause
false positives.  How about relying on the "--git" part in the
"diff --git" header instead, and in the Git case _always_ removing one
leading directory?  (And if this also happens with Hg, include that in
the test as well.)

Also, what about the opposite case, when NEW is /dev/null? does that
work correctly?

Thanks.

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

[Dmitry Gutov]

On 02/10/2024 09:58, Eli Zaretskii wrote:
>> Date: Wed, 2 Oct 2024 03:41:05 +0300
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>>> Observed behavior: diff-mode prompts for the location of "b/foo", and
>>> doesn't allow specifying the location as a non-existent file, meaning
>>> the file can't actaully be created.
>>
>> This is annoying indeed.
>>
>> The attached patch should handle this:
>>
>> * When OLD equals to /dev/null, allow reading non-existing file name.
>> * When NEW starts with b/ or /a, slice that off if such dir does not exist.
>> * Bonus: when the diff is applied in reverse, the checked file names are
>> switched. That helps undo deletions as well. Or renames.
> 
> I think Git uses other prefixes as well (something like i/ and r/,
> perhaps?)

Interesting, I don't remember seeing 'i/' or 'r/' in use.

OT2H, apparently the prefixes are configurable both through command line 
and Git config: 
https://stackoverflow.com/questions/6764953/what-is-the-reason-for-the-a-b-prefixes-of-git-diff

I think that can mean two things: a/ and b/ could be different, and also 
- if a/ and b/ conflict with some dir, the repository could have 
settings to change the diff prefixes. Although not every user will know 
that, perhaps.

> And relying on b/ being an existing directory can cause
> false positives.  How about relying on the "--git" part in the
> "diff --git" header instead, and in the Git case _always_ removing one
> leading directory?

I guess that's an option too.

> (And if this also happens with Hg, include that in
> the test as well.)

With Hg, the format look like this:

   diff -r df0ef194120b -r 2039b18843da accessible/aom/AccessibleNode.cpp

No mention of 'Hg', that is. Could we match "\`diff -r" and

And I suppose this is theoretically a problem for most VC backends, 
although of we only support Git and Hg, it might be fine. That piece of 
code only affect the default file name input anyway.

-r is a real 'diff' option, by the way. When I try it, here's how the 
header looks:

diff '--color=auto' -u -r emacs/aclocal.m4 emacs-master/aclocal.m4
--- emacs/aclocal.m4    2024-08-19 04:06:06.237502671 +0300
+++ emacs-master/aclocal.m4     2024-09-14 05:42:16.907845058 +0300

> Also, what about the opposite case, when NEW is /dev/null? does that
> work correctly?

Not currently or with the proposed patch. It could be fixed along 
similar lines, but I'm not clear on the ideal behavior here. Delete the 
"old" file and kill its buffer? And say that with 'message'? This would 
differ from the regular diff-apply-hunk behavior in that nothing else 
might appear on screen if the buffer was not displayed, and if it was 
displayed - that would kill a visible buffer. Unusual for Emacs behavior 
either way.

Deleting files is something that one can do manually, though, so solving 
this seems lower priority.

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

[Eli Zaretskii]

> Date: Wed, 2 Oct 2024 21:57:48 +0300
> Cc: sbaugh@catern.com, 62731@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > And relying on b/ being an existing directory can cause
> > false positives.  How about relying on the "--git" part in the
> > "diff --git" header instead, and in the Git case _always_ removing one
> > leading directory?
> 
> I guess that's an option too.
> 
> > (And if this also happens with Hg, include that in
> > the test as well.)
> 
> With Hg, the format look like this:
> 
>    diff -r df0ef194120b -r 2039b18843da accessible/aom/AccessibleNode.cpp
> 
> No mention of 'Hg', that is. Could we match "\`diff -r" and

If Hg doesn't prepend fake leading directories, we don't need to be
bothered by Hg.

> > Also, what about the opposite case, when NEW is /dev/null? does that
> > work correctly?
> 
> Not currently or with the proposed patch. It could be fixed along 
> similar lines, but I'm not clear on the ideal behavior here. Delete the 
> "old" file and kill its buffer? And say that with 'message'?

Something like that, yes.  We could also delete the file silently.

> 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.

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

[Dmitry Gutov]

On 02/10/2024 22:41, Eli Zaretskii wrote:

>> With Hg, the format look like this:
>>
>>     diff -r df0ef194120b -r 2039b18843da accessible/aom/AccessibleNode.cpp
>>
>> No mention of 'Hg', that is. Could we match "\`diff -r" and
> 
> If Hg doesn't prepend fake leading directories, we don't need to be
> bothered by Hg.

It does. A fuller example, with deletion:

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 @@

>>> Also, what about the opposite case, when NEW is /dev/null? does that
>>> work correctly?
>>
>> Not currently or with the proposed patch. It could be fixed along
>> similar lines, but I'm not clear on the ideal behavior here. Delete the
>> "old" file and kill its buffer? And say that with 'message'?
> 
> Something like that, yes.  We could also delete the file silently.

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.

>> 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.

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

[Eli Zaretskii]

> Date: Wed, 2 Oct 2024 22:48:27 +0300
> Cc: sbaugh@catern.com, 62731@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 02/10/2024 22:41, Eli Zaretskii wrote:
> 
> >> With Hg, the format look like this:
> >>
> >>     diff -r df0ef194120b -r 2039b18843da accessible/aom/AccessibleNode.cpp
> >>
> >> No mention of 'Hg', that is. Could we match "\`diff -r" and
> > 
> > If Hg doesn't prepend fake leading directories, we don't need to be
> > bothered by Hg.
> 
> It does. A fuller example, with deletion:
> 
> 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?

> >>> Also, what about the opposite case, when NEW is /dev/null? does that
> >>> work correctly?
> >>
> >> Not currently or with the proposed patch. It could be fixed along
> >> similar lines, but I'm not clear on the ideal behavior here. Delete the
> >> "old" file and kill its buffer? And say that with 'message'?
> > 
> > Something like that, yes.  We could also delete the file silently.
> 
> 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?

> >> 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.

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

[Dmitry Gutov]

[-- 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

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

[Dmitry Gutov]

On 04/10/2024 04:14, Dmitry Gutov 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.

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

[Juri Linkov]

>> 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.

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

[Dmitry Gutov]

Hi Juri,

On 15/10/2024 19:13, 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)


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/

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

[Juri Linkov]

>>>> 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.

> 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)

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

[Dmitry Gutov]

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.

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

[Juri Linkov]

>>> 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.

Thanks for fixing, I confirm that now 'C-c C-c' doesn't go to the
backup file, only 'C-u C-c C-c' does.

> 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.

I forgot that diff-goto-source is not context-dependent for file diffs,
unlike for vc-diff.  So for file diffs it depends only on the prefix arg.
I don't know why is such inconsistency and whether it should be improved.