bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Alfred M. Szmidt]

The C-x v v command in a *vc-diff* buffer does not work if you have a
lesser version of patch, e.g., the one on OpenBSD:

user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
             [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
             [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
             [--posix] [origfile [patchfile]]
       patch <patchfile



In GNU Emacs 30.0.50 (build 3, x86_64-unknown-openbsd7.3, GTK+ Version
 3.24.37, cairo version 1.17.8) of 2023-09-25 built on
 nitrogenium.mendeleev
Repository revision: df5a9a78b51f2f42d2dbb010e811a239fc014732
Repository branch: master
System Description: OpenBSD nitrogenium.mendeleev 7.3 GENERIC.MP#3 amd64

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GNUTLS GSETTINGS HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBXML2 M17N_FLT MODULES NOTIFY KQUEUE PDUMPER PNG RSVG THREADS
TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  locale-coding-system: nil

Major mode: Messages

Minor modes in effect:
  global-company-mode: t
  company-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  buffer-read-only: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
~/vc-backup hides /home/ams/.emacs.d/elpa/vc-backup-1.1.0/vc-backup
~/loaddefs hides /home/ams/emacs/lisp/loaddefs

Features:
(shadow emacsbug log-edit message yank-media puny dired dired-loaddefs
rfc822 mml mml-sec epa epg rfc6068 epg-config gnus-util mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
add-log vc-backup vc-hg vc-git vc-bzr vc-src vc-sccs vc-svn vc-cvs
vc-rcs log-view pcvs-util smerge-mode diff diff-mode vc-fossil vc
vc-dispatcher term/screen term/xterm xterm rcirc parse-time iso8601
time-date mail-queue sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils slime-asdf grep slime-quicklisp slime-fancy
slime-indentation slime-cl-indent cl-indent slime-trace-dialog
slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree slime-scratch slime-presentations advice
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl elp slime-parse slime
derived cl-extra help-mode easy-mmode gud apropos compile
text-property-search arc-mode archive-mode noutline outline icons pp
comint ansi-osc ansi-color hyperspec thingatpt slime-autoloads
company-oddmuse company-keywords company-etags etags fileloop
generator xref project ring company-gtags company-dabbrev-code
company-dabbrev company-files company-capf company-cmake company-xcode
company-clang company-semantic company-eclim company-template
company-bbdb company edmacro kmacro pcase cal-menu calendar
cal-loaddefs autoload loaddefs-gen radix-tree lisp-mnt finder-inf
disp-table vc-backup-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 touch-screen 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 kqueue lcms2 dynamic-setting system-font-setting
font-render-setting cairo gtk x-toolkit xinput2 x multi-tty
move-toolbar make-network-process emacs)

Memory information:
((conses 16 229382 19817) (symbols 48 17997 0) (strings 32 59615 1393)
 (string-bytes 1 1639976) (vectors 16 24705)
 (vector-slots 8 286056 14977) (floats 8 68 481) (intervals 56 356 0)
 (buffers 984 18))

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 26/09/2023 15:17, Alfred M. Szmidt wrote:
> The C-x v v command in a*vc-diff*  buffer does not work if you have a
> lesser version of patch, e.g., the one on OpenBSD:
> 
> user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
> usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
>               [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
>               [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
>               [--posix] [origfile [patchfile]]
>         patch <patchfile

Thanks for the report.

Any idea how to implement the same thing using your version of 'patch'?

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Alfred M. Szmidt]

   > The C-x v v command in a*vc-diff*  buffer does not work if you have a
   > lesser version of patch, e.g., the one on OpenBSD:
   > 
   > user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
   > usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
   >               [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
   >               [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
   >               [--posix] [origfile [patchfile]]
   >         patch <patchfile

   Thanks for the report.

   Any idea how to implement the same thing using your version of 'patch'?

Maybe just pass --posix for both GNU patch and other patch?


From the GNU patch manual:

  The --no-backup-if-mismatch option causes patch to not create backup
  files, even for mismatched patches; this is the default when
  conforming to POSIX.

Scoundering the patch(1) man page:

     -b, --backup
             Save a backup copy of the file before it is modified.  By default
             the original file is saved with a backup extension of ".orig"
             unless the file already has a numbered backup, in which case a
             numbered backup is made.  This is equivalent to specifying "-V
             existing".  This option is currently the default, unless --posix
             is specified.

And looking further, --posix:

     --posix
             Enables strict IEEE Std 1003.1-2008 ("POSIX.1") conformance,
             specifically:

             1.   Backup files are not created unless the -b option is
                  specified.

             2.   If unspecified, the file name used is the first of the old,
                  new and index files that exists.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 26/09/2023 15:43, Alfred M. Szmidt wrote:
>     > The C-x v v command in a*vc-diff*  buffer does not work if you have a
>     > lesser version of patch, e.g., the one on OpenBSD:
>     >
>     > user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
>     > usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
>     >               [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
>     >               [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
>     >               [--posix] [origfile [patchfile]]
>     >         patch <patchfile
> 
>     Thanks for the report.
> 
>     Any idea how to implement the same thing using your version of 'patch'?
> 
> Maybe just pass --posix for both GNU patch and other patch?

All right. Does this work for you?

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 2f4b028bb4a..356da5ddfac 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1724,7 +1724,8 @@ vc-default-checkin-patch
                                               nil
                                               "-p1"
                                               "-r" null-device
-                                             "--no-backup-if-mismatch"
+                                             "--posix"
+                                             "--remove-empty-files"
                                               "-i" "-"))
                (user-error "Patch failed: %s" (buffer-string))))
            (vc-call-backend backend 'checkin files comment))

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Alfred M. Szmidt]

   diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
   index 2f4b028bb4a..356da5ddfac 100644
   --- a/lisp/vc/vc.el
   +++ b/lisp/vc/vc.el
   @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
						  nil
						  "-p1"
						  "-r" null-device
   -                                             "--no-backup-if-mismatch"
   +                                             "--posix"
   +                                             "--remove-empty-files"


Patch on BSD doe not have --remote-empty-files.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 26/09/2023 18:08, Alfred M. Szmidt wrote:
>     diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>     index 2f4b028bb4a..356da5ddfac 100644
>     --- a/lisp/vc/vc.el
>     +++ b/lisp/vc/vc.el
>     @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
> 						  nil
> 						  "-p1"
> 						  "-r" null-device
>     -                                             "--no-backup-if-mismatch"
>     +                                             "--posix"
>     +                                             "--remove-empty-files"
> 
> 
> Patch on BSD doe not have --remote-empty-files.

Just to make sure: did you try the correct spelling? "remove", not "remote".

Both FreeBSD and OpenBSD seem to support it:

https://man.freebsd.org/cgi/man.cgi?patch(1)
https://man.openbsd.org/patch.1

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Alfred M. Szmidt]

   On 26/09/2023 18:08, Alfred M. Szmidt wrote:
   >     diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
   >     index 2f4b028bb4a..356da5ddfac 100644
   >     --- a/lisp/vc/vc.el
   >     +++ b/lisp/vc/vc.el
   >     @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
   > 						  nil
   > 						  "-p1"
   > 						  "-r" null-device
   >     -                                             "--no-backup-if-mismatch"
   >     +                                             "--posix"
   >     +                                             "--remove-empty-files"
   > 
   > 
   > Patch on BSD doe not have --remote-empty-files.

   Just to make sure: did you try the correct spelling? "remove", not "remote".

I stand corrected, the short form output when you pass a wrong
argument didn't show the switch.  I'll try tomorrow.

   Both FreeBSD and OpenBSD seem to support it:

   https://man.freebsd.org/cgi/man.cgi?patch(1)
   https://man.openbsd.org/patch.1

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Date: Tue, 26 Sep 2023 08:17:27 -0400
> 
> 
> The C-x v v command in a *vc-diff* buffer does not work if you have a
> lesser version of patch, e.g., the one on OpenBSD:
> 
> user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
> usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
>              [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
>              [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
>              [--posix] [origfile [patchfile]]
>        patch <patchfile

I don't understand: does "git apply" invoke Patch?  AFAICT, "C-x v v"
in a *vc-diff* buffer runs "git apply FILE", where FILE is a temporary
patch file created from the diffs to be applied.  What am I missing?

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 29/09/2023 10:05, Eli Zaretskii wrote:
>> From: "Alfred M. Szmidt"<ams@gnu.org>
>> Date: Tue, 26 Sep 2023 08:17:27 -0400
>>
>>
>> The C-x v v command in a*vc-diff*  buffer does not work if you have a
>> lesser version of patch, e.g., the one on OpenBSD:
>>
>> user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
>> usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
>>               [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
>>               [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
>>               [--posix] [origfile [patchfile]]
>>         patch <patchfile
> I don't understand: does "git apply" invoke Patch?  AFAICT, "C-x v v"
> in a*vc-diff*  buffer runs "git apply FILE", where FILE is a temporary
> patch file created from the diffs to be applied.  What am I missing?

vc-default-checkin-patch calls 'patch'. It's for other backends.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Eli Zaretskii]

> Date: Fri, 29 Sep 2023 13:22:39 +0300
> Cc: 66211@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 29/09/2023 10:05, Eli Zaretskii wrote:
> >> From: "Alfred M. Szmidt"<ams@gnu.org>
> >> Date: Tue, 26 Sep 2023 08:17:27 -0400
> >>
> >>
> >> The C-x v v command in a*vc-diff*  buffer does not work if you have a
> >> lesser version of patch, e.g., the one on OpenBSD:
> >>
> >> user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
> >> usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
> >>               [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
> >>               [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
> >>               [--posix] [origfile [patchfile]]
> >>         patch <patchfile
> > I don't understand: does "git apply" invoke Patch?  AFAICT, "C-x v v"
> > in a*vc-diff*  buffer runs "git apply FILE", where FILE is a temporary
> > patch file created from the diffs to be applied.  What am I missing?
> 
> vc-default-checkin-patch calls 'patch'. It's for other backends.

Why do we call Patch if we have a way to apply hunks in Lisp?  Wasn't
that just discussed in the other thread?

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 29/09/2023 18:22, Eli Zaretskii wrote:
>> Date: Fri, 29 Sep 2023 13:22:39 +0300
>> Cc: 66211@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> On 29/09/2023 10:05, Eli Zaretskii wrote:
>>>> From: "Alfred M. Szmidt"<ams@gnu.org>
>>>> Date: Tue, 26 Sep 2023 08:17:27 -0400
>>>>
>>>>
>>>> The C-x v v command in a*vc-diff*  buffer does not work if you have a
>>>> lesser version of patch, e.g., the one on OpenBSD:
>>>>
>>>> user-error: Patch failed: patch: unknown option -- no-backup-if-mismatch
>>>> usage: patch [-bCcEeflNnRstuv] [-B backup-prefix] [-D symbol] [-d directory]
>>>>                [-F max-fuzz] [-i patchfile] [-o out-file] [-p strip-count]
>>>>                [-r rej-name] [-V t | nil | never] [-x number] [-z backup-ext]
>>>>                [--posix] [origfile [patchfile]]
>>>>          patch <patchfile
>>> I don't understand: does "git apply" invoke Patch?  AFAICT, "C-x v v"
>>> in a*vc-diff*  buffer runs "git apply FILE", where FILE is a temporary
>>> patch file created from the diffs to be applied.  What am I missing?
>>
>> vc-default-checkin-patch calls 'patch'. It's for other backends.
> 
> Why do we call Patch if we have a way to apply hunks in Lisp?  Wasn't
> that just discussed in the other thread?

https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01353.html

Anyway, it doesn't seem like 'patch' itself is the problem in this case: 
the alternative flags should work for BSD Patch as well. Though I'm 
still waiting for confirmation.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Eli Zaretskii]

> Date: Fri, 29 Sep 2023 20:27:21 +0300
> Cc: ams@gnu.org, 66211@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 29/09/2023 18:22, Eli Zaretskii wrote:
> >> vc-default-checkin-patch calls 'patch'. It's for other backends.
> > 
> > Why do we call Patch if we have a way to apply hunks in Lisp?  Wasn't
> > that just discussed in the other thread?
> 
> https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01353.html

That doesn't have a lot in the way of answering my question above.

> Anyway, it doesn't seem like 'patch' itself is the problem in this case: 

When Patch is installed, sure.  But using our own code is better
because it doesn't need Patch and doesn't have to rely on specific
options it may or may not support.

I'm not insisting, mind you, just asking.  It sounds inconsistent with
what we do in similar situations in other places, that's all.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 29/09/2023 20:40, Eli Zaretskii wrote:
>> Date: Fri, 29 Sep 2023 20:27:21 +0300
>> Cc: ams@gnu.org, 66211@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> On 29/09/2023 18:22, Eli Zaretskii wrote:
>>>> vc-default-checkin-patch calls 'patch'. It's for other backends.
>>>
>>> Why do we call Patch if we have a way to apply hunks in Lisp?  Wasn't
>>> that just discussed in the other thread?
>>
>> https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01353.html
> 
> That doesn't have a lot in the way of answering my question above.

The discussion regarding that choice is inside the quotes' ladder in 
that message.

>> Anyway, it doesn't seem like 'patch' itself is the problem in this case:
> 
> When Patch is installed, sure.  But using our own code is better
> because it doesn't need Patch and doesn't have to rely on specific
> options it may or may not support.
> 
> I'm not insisting, mind you, just asking.  It sounds inconsistent with
> what we do in similar situations in other places, that's all.

My preference was to limit the number of points of unreliability. And/or 
minor differences (e.g. whether file removals are supported).

'diff-apply-buffer' has only been checked in, we can revisit this a 
little later, after more testing and stuff.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Alfred M. Szmidt]

Sorry, I have no interest in verifying this at this point.  Eli's
behaviour is beyond my mood right now.  Maybe some other time.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 26/09/2023 18:08, Alfred M. Szmidt wrote:
>     diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>     index 2f4b028bb4a..356da5ddfac 100644
>     --- a/lisp/vc/vc.el
>     +++ b/lisp/vc/vc.el
>     @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
> 						  nil
> 						  "-p1"
> 						  "-r" null-device
>     -                                             "--no-backup-if-mismatch"
>     +                                             "--posix"
>     +                                             "--remove-empty-files"

Perhaps some other OpenBSD/FreeBSD user wants to verify this patch?

The scenario is making a commit from a vc-diff buffer (with 'C-x v v' 
there) managed by Bzr or SVN or CVS.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Eli Zaretskii]

> Cc: 66211@debbugs.gnu.org
> Date: Sun, 8 Oct 2023 01:33:37 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 26/09/2023 18:08, Alfred M. Szmidt wrote:
> >     diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> >     index 2f4b028bb4a..356da5ddfac 100644
> >     --- a/lisp/vc/vc.el
> >     +++ b/lisp/vc/vc.el
> >     @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
> > 						  nil
> > 						  "-p1"
> > 						  "-r" null-device
> >     -                                             "--no-backup-if-mismatch"
> >     +                                             "--posix"
> >     +                                             "--remove-empty-files"
> 
> Perhaps some other OpenBSD/FreeBSD user wants to verify this patch?

If no one steps forward in a week or so, I suggest that you install
this anyway.

bug#66211: 30.0.50; C-x v v in vc-diff does not work with lesser versionf of patch

[Dmitry Gutov]

On 08/10/2023 08:01, Eli Zaretskii wrote:
>> Cc:66211@debbugs.gnu.org
>> Date: Sun, 8 Oct 2023 01:33:37 +0300
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 26/09/2023 18:08, Alfred M. Szmidt wrote:
>>>      diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>>>      index 2f4b028bb4a..356da5ddfac 100644
>>>      --- a/lisp/vc/vc.el
>>>      +++ b/lisp/vc/vc.el
>>>      @@ -1724,7 +1724,8 @@ vc-default-checkin-patch
>>> 						  nil
>>> 						  "-p1"
>>> 						  "-r" null-device
>>>      -                                             "--no-backup-if-mismatch"
>>>      +                                             "--posix"
>>>      +                                             "--remove-empty-files"
>> Perhaps some other OpenBSD/FreeBSD user wants to verify this patch?
> If no one steps forward in a week or so, I suggest that you install
> this anyway.

Okay, done. Thanks.