About differences with GNU patch(1)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

About differences with GNU patch(1)

Vadim Zhukov
Hi,

The Ansible "patch" module fails to work on OpenBSD because it tries
to use "--dry-run" command-line option on patch(1), while ours
supports -C/--check instead. For now, I have an obvious, hm, patch :)
for patch.py that replaces --dry-run with --check. But what way do
people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
some logic to patch.py that'll detect correct command-line option to
use?

There's also an issue with --binary: we don't support this option on
OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
Would it be okay to add --binary as a no-op option to our patch?

--
  WBR,
  Vadim Zhukov

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Vadim Zhukov
ср, 20 июн. 2018 г. в 21:17, Vadim Zhukov <[hidden email]>:

>
> Hi,
>
> The Ansible "patch" module fails to work on OpenBSD because it tries
> to use "--dry-run" command-line option on patch(1), while ours
> supports -C/--check instead. For now, I have an obvious, hm, patch :)
> for patch.py that replaces --dry-run with --check. But what way do
> people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> some logic to patch.py that'll detect correct command-line option to
> use?
>
> There's also an issue with --binary: we don't support this option on
> OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> Would it be okay to add --binary as a no-op option to our patch?

BTW, the FreeBSD patch got --dry-run 4 years ago:
https://svnweb.freebsd.org/base/head/usr.bin/patch/patch.c?r1=267490&r2=267512

NetBSD got it in 2005:
http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/patch/patch.c.diff?r1=1.22&r2=1.23&only_with_tag=MAIN

Neither has --binary.

--
  WBR,
  Vadim Zhukov

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Chris Cappuccio
Vadim Zhukov [[hidden email]] wrote:

> ????, 20 ??????. 2018 ??. ?? 21:17, Vadim Zhukov <[hidden email]>:
> >
> > Hi,
> >
> > The Ansible "patch" module fails to work on OpenBSD because it tries
> > to use "--dry-run" command-line option on patch(1), while ours
> > supports -C/--check instead. For now, I have an obvious, hm, patch :)
> > for patch.py that replaces --dry-run with --check. But what way do
> > people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> > some logic to patch.py that'll detect correct command-line option to
> > use?
> >
> > There's also an issue with --binary: we don't support this option on
> > OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> > Would it be okay to add --binary as a no-op option to our patch?
>
> BTW, the FreeBSD patch got --dry-run 4 years ago:
> https://svnweb.freebsd.org/base/head/usr.bin/patch/patch.c?r1=267490&r2=267512
>
> NetBSD got it in 2005:
> http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/patch/patch.c.diff?r1=1.22&r2=1.23&only_with_tag=MAIN
>
> Neither has --binary.
>

Marc Espie did -C in 1998. See:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/patch/patch.c.diff?r1=1.10&r2=1.11

I think OpenBSD generally shies away from --gnu-style-options because they're
rather ugly compared to the originals. Unfortunately GNU is apparently setting
the standard for FreeBSD and NetBSD now.

Chris

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Vadim Zhukov
ср, 20 июн. 2018 г. в 21:59, Chris Cappuccio <[hidden email]>:

>
> Vadim Zhukov [[hidden email]] wrote:
> > ????, 20 ??????. 2018 ??. ?? 21:17, Vadim Zhukov <[hidden email]>:
> > >
> > > Hi,
> > >
> > > The Ansible "patch" module fails to work on OpenBSD because it tries
> > > to use "--dry-run" command-line option on patch(1), while ours
> > > supports -C/--check instead. For now, I have an obvious, hm, patch :)
> > > for patch.py that replaces --dry-run with --check. But what way do
> > > people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> > > some logic to patch.py that'll detect correct command-line option to
> > > use?
> > >
> > > There's also an issue with --binary: we don't support this option on
> > > OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> > > Would it be okay to add --binary as a no-op option to our patch?
> >
> > BTW, the FreeBSD patch got --dry-run 4 years ago:
> > https://svnweb.freebsd.org/base/head/usr.bin/patch/patch.c?r1=267490&r2=267512
> >
> > NetBSD got it in 2005:
> > http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/patch/patch.c.diff?r1=1.22&r2=1.23&only_with_tag=MAIN
> >
> > Neither has --binary.
> >
>
> Marc Espie did -C in 1998. See:
>
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/patch/patch.c.diff?r1=1.10&r2=1.11
>
> I think OpenBSD generally shies away from --gnu-style-options because they're
> rather ugly compared to the originals. Unfortunately GNU is apparently setting
> the standard for FreeBSD and NetBSD now.

But to be honest, GNU implemented --dry-run in 1997, so they were earlier.

--
  WBR,
  Vadim Zhukov

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Stuart Henderson
In reply to this post by Vadim Zhukov
On 2018/06/20 21:17, Vadim Zhukov wrote:
> Hi,
>
> The Ansible "patch" module fails to work on OpenBSD because it tries
> to use "--dry-run" command-line option on patch(1), while ours
> supports -C/--check instead. For now, I have an obvious, hm, patch :)
> for patch.py that replaces --dry-run with --check. But what way do
> people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> some logic to patch.py that'll detect correct command-line option to
> use?

I think it would be right to add some logic. By the time you're running
a command like this Ansible already knows what OS it's connecting to,
it should use that knowledge, and that way it will work on old OpenBSD
versions too.

Since patch already accepts long options, it might also be worth
accepting --dry-run as a synonym for --check, as long as it does the
same thing.

> There's also an issue with --binary: we don't support this option on
> OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> Would it be okay to add --binary as a no-op option to our patch?

It says "This option is needed on POSIX systems when applying patches
generated on non-POSIX systems to non-POSIX files", so I think it does
do something there? However our patch doesn't have heuristics for
handling line-endings like GNU patch does..

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Vadim Zhukov
ср, 20 июн. 2018 г. в 22:09, Stuart Henderson <[hidden email]>:

>
> On 2018/06/20 21:17, Vadim Zhukov wrote:
> > Hi,
> >
> > The Ansible "patch" module fails to work on OpenBSD because it tries
> > to use "--dry-run" command-line option on patch(1), while ours
> > supports -C/--check instead. For now, I have an obvious, hm, patch :)
> > for patch.py that replaces --dry-run with --check. But what way do
> > people prefer: to move OpenBSD's patch(1) closer to GNU one, or to add
> > some logic to patch.py that'll detect correct command-line option to
> > use?
>
> I think it would be right to add some logic. By the time you're running
> a command like this Ansible already knows what OS it's connecting to,
> it should use that knowledge, and that way it will work on old OpenBSD
> versions too.
>
> Since patch already accepts long options, it might also be worth
> accepting --dry-run as a synonym for --check, as long as it does the
> same thing.

It's absolutely the same, yes.

> > There's also an issue with --binary: we don't support this option on
> > OpenBSD, and  GNU patch manual page says it's a no-op on POSIX anyway.
> > Would it be okay to add --binary as a no-op option to our patch?
>
> It says "This option is needed on POSIX systems when applying patches
> generated on non-POSIX systems to non-POSIX files", so I think it does
> do something there? However our patch doesn't have heuristics for
> handling line-endings like GNU patch does..

Ah, I've misunderstood "binary mode". Then, yes, this is not a no-op.
This option isn't a real problem though, since if you're feeding our
patch() with invalid line endings, you're screwed already. And you can
just do not set "binary" flag in Ansible to avoid --binary there. :)

--
  WBR,
  Vadim Zhukov

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Vadim Zhukov
In reply to this post by Vadim Zhukov
So after a few discussions I propose to add --dry-run as synonim for -C
in our patch(1). Quick summary:

  * GNU got --dry-run earlier than us;
  * --dry-run way more popular name than --check for such functionality;
  * FreeBSD and NetBSD has the same for a long time already;
  * We don't care about long option names generally, anyway.

On the second option, --binary, since we have no functionality and
I see no demand for it, lets just keep things as is. Failing with
"unkonwn option" message is clear indicator that requested functionality
isn't supported.

Builds and runs on (almost) -CURRENT. Okay or not?

--
  WBR,
    Vadim Zhukov


Index: patch.c
===================================================================
RCS file: /cvs/src/usr.bin/patch/patch.c,v
retrieving revision 1.65
diff -u -p -r1.65 patch.c
--- patch.c 7 Apr 2018 14:55:13 -0000 1.65
+++ patch.c 22 Jun 2018 09:12:53 -0000
@@ -454,6 +454,7 @@ get_some_switches(void)
  {"context", no_argument, 0, 'c'},
  {"debug", required_argument, 0, 'x'},
  {"directory", required_argument, 0, 'd'},
+ {"dry-run", no_argument, 0, 'C'},
  {"ed", no_argument, 0, 'e'},
  {"force", no_argument, 0, 'f'},
  {"forward", no_argument, 0, 'N'},

Reply | Threaded
Open this post in threaded view
|

Re: About differences with GNU patch(1)

Jeremie Courreges-Anglas-2
On Fri, Jun 22 2018, Vadim Zhukov <[hidden email]> wrote:

> So after a few discussions I propose to add --dry-run as synonim for -C
> in our patch(1). Quick summary:
>
>   * GNU got --dry-run earlier than us;
>   * --dry-run way more popular name than --check for such functionality;
>   * FreeBSD and NetBSD has the same for a long time already;
>   * We don't care about long option names generally, anyway.
>
> On the second option, --binary, since we have no functionality and
> I see no demand for it, lets just keep things as is. Failing with
> "unkonwn option" message is clear indicator that requested functionality
> isn't supported.
>
> Builds and runs on (almost) -CURRENT. Okay or not?

ok jca@ but please update the manpage, something like that I guess...

Index: patch.1
===================================================================
RCS file: /d/cvs/src/usr.bin/patch/patch.1,v
retrieving revision 1.31
diff -u -p -p -u -r1.31 patch.1
--- patch.1 11 Apr 2018 10:06:50 -0000 1.31
+++ patch.1 22 Jun 2018 09:46:27 -0000
@@ -99,7 +99,7 @@ This is equivalent to specifying
 This option is currently the default, unless
 .Fl -posix
 is specified.
-.It Fl C , Fl Fl check
+.It Fl C , Fl Fl check , Fl Fl dry-run
 Checks that the patch would apply cleanly, but does not modify anything.
 .It Fl c , Fl Fl context
 Forces

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE