GNU bug report logs - #33468
A bug with yes and --help

Previous Next

Package: coreutils;

Reported by: Uko Kokņevičs <perkontevs <at> gmail.com>

Date: Thu, 22 Nov 2018 18:30:02 UTC

Severity: normal

Done: Bernhard Voelker <mail <at> bernhard-voelker.de>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33468 in the body.
You can then email your comments to 33468 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Thu, 22 Nov 2018 18:30:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Uko Kokņevičs <perkontevs <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-coreutils <at> gnu.org. (Thu, 22 Nov 2018 18:30:02 GMT) Full text and rfc822 format available.

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

From: Uko Kokņevičs <perkontevs <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: A bug with yes and --help
Date: Thu, 22 Nov 2018 20:09:35 +0200
So I was messing around with `yes`, and after running `yes --help me` it
output this:

> yes: unrecognized option '--help'
> Try 'yes --help' for more information.

After a bit of more testing of this, I found the same reaction from
`whoami`.  I believe this might be because both `yes` and `whoami` only
ever accept one option -- that being `--help` or `--version`, and it
says that it doesn't know what `--version` is when run with an extra
operand as well.  However, `true` or `false` doesn't give a textual
error, but they completely ignore the option:

> $ /usr/bin/true --version
> true (GNU coreutils) 8.30
> Copyright (C) 2018 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
<https://gnu.org/licenses/gpl.html>.
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> Written by Jim Meyering.
> $ /usr/bin/true --version asd
> $ echo $?
> 0

Suggested possible fixes:
1.  A more general error message, e.g., `yes` only accepts one option or
none.
2.  Ignore the stuff that follows the option, making `yes --help me` act
the same as `yes --help`, which kind of matches with other shell
commands in that they print help, ignore the rest of arguments and exit.
3.  Ignore the option: `yes --help me` would use "--help me" as the
string to repeat.

The third one isn't really a good one, but it exists as an idea so I'm
marking it down.

----
Uko Kokņevičs (Uko Koknevics)





Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 24 Nov 2018 00:53:01 GMT) Full text and rfc822 format available.

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 23 Nov 2018 16:52:10 -0800
On 22/11/18 10:09, Uko Kokņevičs wrote:
> So I was messing around with `yes`, and after running `yes --help me` it
> output this:
> 
>> yes: unrecognized option '--help'
>> Try 'yes --help' for more information.
> 
> After a bit of more testing of this, I found the same reaction from
> `whoami`.  I believe this might be because both `yes` and `whoami` only
> ever accept one option -- that being `--help` or `--version`, and it
> says that it doesn't know what `--version` is when run with an extra
> operand as well.  However, `true` or `false` doesn't give a textual
> error, but they completely ignore the option:
> 
>> $ /usr/bin/true --version
>> true (GNU coreutils) 8.30
>> Copyright (C) 2018 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later
> <https://gnu.org/licenses/gpl.html>.
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.
>>
>> Written by Jim Meyering.
>> $ /usr/bin/true --version asd
>> $ echo $?
>> 0
> 
> Suggested possible fixes:
> 1.  A more general error message, e.g., `yes` only accepts one option or
> none.
> 2.  Ignore the stuff that follows the option, making `yes --help me` act
> the same as `yes --help`, which kind of matches with other shell
> commands in that they print help, ignore the rest of arguments and exit.
> 3.  Ignore the option: `yes --help me` would use "--help me" as the
> string to repeat.
> 
> The third one isn't really a good one, but it exists as an idea so I'm
> marking it down.

Yes this isn't ideal
yes uses parse_long_options from gnulib which only processes
options if there is a single option specified.

I see the message got a little more confusing since:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.28-42-g5782a36
changing from unrecognized option '-', to unrecognized option '--help'
Commands that use parse_long_options() are:

  cksum dd echo expr getlimits hostid hostname link logname nohup printf
  sleep test tsort unlink uptime users whoami yes

thanks
Pádraig




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 24 Nov 2018 17:18:01 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Sat, 24 Nov 2018 18:17:41 +0100

On 11/24/18 1:52 AM, Pádraig Brady wrote:
> On 22/11/18 10:09, Uko Kokņevičs wrote:
>> So I was messing around with `yes`, and after running `yes --help me` it
>> output this:
>>
>>> yes: unrecognized option '--help'
>>> Try 'yes --help' for more information.
>>
>> After a bit of more testing of this, I found the same reaction from
>> `whoami`.  I believe this might be because both `yes` and `whoami` only
>> ever accept one option -- that being `--help` or `--version`, and it
>> says that it doesn't know what `--version` is when run with an extra
>> operand as well.  However, `true` or `false` doesn't give a textual
>> error, but they completely ignore the option:
>>
>>> $ /usr/bin/true --version
>>> true (GNU coreutils) 8.30
>>> Copyright (C) 2018 Free Software Foundation, Inc.
>>> License GPLv3+: GNU GPL version 3 or later
>> <https://gnu.org/licenses/gpl.html>.
>>> This is free software: you are free to change and redistribute it.
>>> There is NO WARRANTY, to the extent permitted by law.
>>>
>>> Written by Jim Meyering.
>>> $ /usr/bin/true --version asd
>>> $ echo $?
>>> 0
>>
>> Suggested possible fixes:
>> 1.  A more general error message, e.g., `yes` only accepts one option or
>> none.
>> 2.  Ignore the stuff that follows the option, making `yes --help me` act
>> the same as `yes --help`, which kind of matches with other shell
>> commands in that they print help, ignore the rest of arguments and exit.
>> 3.  Ignore the option: `yes --help me` would use "--help me" as the
>> string to repeat.
>>
>> The third one isn't really a good one, but it exists as an idea so I'm
>> marking it down.
> 
> Yes this isn't ideal
> yes uses parse_long_options from gnulib which only processes
> options if there is a single option specified.
> 
> I see the message got a little more confusing since:
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.28-42-g5782a36
> changing from unrecognized option '-', to unrecognized option '--help'
> Commands that use parse_long_options() are:
> 
>   cksum dd echo expr getlimits hostid hostname link logname nohup printf
>   sleep test tsort unlink uptime users whoami yes
> 
> thanks
> Pádraig

Ouch.  At least for 'yes', one could argument that the program should only
care about the (GNU) long options --help and --version if they are the
only argument, and take everything else as the string(s) to be output:

  $ src/yes --help | head -n2
  Usage: src/yes [STRING]...
    or:  src/yes OPTION

  $ src/yes --version | head -n2
  yes (GNU coreutils) 8.30.27-806b0
  Copyright (C) 2018 Free Software Foundation, Inc.

  $ src/yes --help me | head -n2
  src/yes --help me
  src/yes --help me

  $ src/yes | head -n 2
  src/yes
  src/yes

Patch below.
WDYT?

Have a nice day,
Berny

 diff --git a/src/yes.c b/src/yes.c
index 3dd5d2f9d..b86b439a4 100644
--- a/src/yes.c
+++ b/src/yes.c
@@ -32,11 +32,6 @@

 #define AUTHORS proper_name ("David MacKenzie")

-static struct option const long_options[] =
-{
-  {NULL, 0, NULL, 0}
-};
-
 void
 usage (int status)
 {
@@ -74,8 +69,6 @@ main (int argc, char **argv)

   parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE_NAME, Version,
                       usage, AUTHORS, (char const *) NULL);
-  if (getopt_long (argc, argv, "+", long_options, NULL) != -1)
-    usage (EXIT_FAILURE);

   char **operands = argv + optind;
   char **operand_lim = argv + argc;




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 24 Nov 2018 18:46:01 GMT) Full text and rfc822 format available.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Sat, 24 Nov 2018 10:45:19 -0800
Bernhard Voelker wrote:
> At least for 'yes', one could argument that the program should only
> care about the (GNU) long options --help and --version if they are the
> only argument, and take everything else as the string(s) to be output

Why is the situation different for 'yes' than for other commands? The GNU coding 
standards are clear that 'yes --help foo' should act like 'yes --help'.

https://www.gnu.org/prep/standards/html_node/_002d_002dhelp.html




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Mon, 26 Nov 2018 08:25:02 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Mon, 26 Nov 2018 09:24:32 +0100
[Message part 1 (text/plain, inline)]
On 11/24/18 7:45 PM, Paul Eggert wrote:
> Why is the situation different for 'yes' than for other commands? The GNU coding 
> standards are clear that 'yes --help foo' should act like 'yes --help'.
> 
> https://www.gnu.org/prep/standards/html_node/_002d_002dhelp.html

Thanks for the link.  Indeed, 'yes' is not special in this regard - I just wanted
to get the discussion going about how the behavior should be.
And: there are special programs - from Padraig's list:

>>>>  cksum dd echo expr getlimits hostid hostname link logname nohup printf
>>>>  sleep test tsort unlink uptime users whoami yes

- dd has argument-style options,
- echo is special per se,
- expr does its own argument handling,
- getlimits is an internal program,
- nohup invokes another program.

I suggest the attached:
- defining a central 'emit_help_or_version_info' in 'system.h',
- with no change for echo, expr and getlimits,
- nohup: only scan until the 1st non-option,
- and scanning over all args for all other of the above programs.

E.g. for 'yes', all of the following successfully detecting
the --version option (and likewise for --help):

  $ src/yes       --version
  $ src/yes hello --version
  $ src/yes hello --version world
  $ src/yes       --version hello
  $ src/yes hello --v       hello -- a b

WDYT?

Have a nice day,
Berny
[0001-all-detect-help-and-version-more-consistently-FIXME.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Mon, 26 Nov 2018 17:54:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Mon, 26 Nov 2018 11:53:48 -0600
On 11/26/18 2:24 AM, Bernhard Voelker wrote:

> E.g. for 'yes', all of the following successfully detecting
> the --version option (and likewise for --help):
> 
>    $ src/yes       --version
>    $ src/yes hello --version
>    $ src/yes hello --version world
>    $ src/yes       --version hello
>    $ src/yes hello --v       hello -- a b

As long as it is still possible to output the string verbatim, which it 
should be with:

$ src/yes -- --version | head -1
--version
$ src/yes -- hello --version | head -1
hello --version

etc.

> 
> WDYT?

GCS is clear that 'yes --version more' should output a help message 
right away, rather than trying to go on to process the 'more' argument, 
barring any strong reason that it should not ('echo' and 'test' being 
two utilities that have strong reasons for being exceptions, due to 
their standardized requirements).  Forwarding utilities, like 'nohup' or 
'env', should not reorder command line arguments, so there, it is harder 
to argue whether 'env --unknown --help' should error that '--unknown' is 
bad or succeed in giving help; but 'env utility --help' should NOT print 
the env-specific help.  Even so, with forwarding apps, GCS is clear that 
'env --help --unknown' should prefer to give help rather than 
complaining about --unknown.  When command-line option reordering is 
permitted (GCS recommends it for all non-forwarding apps), then 'yes 
more --version' should output help the same as 'yes --help more', rather 
than behave like non-reordering 'yes -- more --version'.

So it really boils down to an audit of which utilities are forwarding 
apps (and must not reorder arguments), which utilities have special 
behavior based on number of arguments (echo, test, ...), and a question 
of whether unknown options should be diagnosed up front.  I think your 
proposal to change 'yes' makes sense, although I haven't closely thought 
about the ramifications of the other utilities you are touching.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Tue, 27 Nov 2018 21:05:01 GMT) Full text and rfc822 format available.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 27 Nov 2018 13:04:31 -0800
I'd rather see a function than a big macro like that. Can we do it as a 
function instead?

Is the idea to deprecate and/or stop using parse_long_options, since it 
doesn't work the way the Gnu Coding Standards says it should? If so, 
maybe it would be simpler to change parse_long_options instead. This 
could involve changing the API for parse_long_options.





Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Thu, 29 Nov 2018 08:50:02 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#33468: A bug with yes and --help
Date: Thu, 29 Nov 2018 09:48:34 +0100
[Message part 1 (text/plain, inline)]
On 11/27/18 10:04 PM, Paul Eggert wrote:
> I'd rather see a function than a big macro like that. Can we do it as a 
> function instead?
> 
> Is the idea to deprecate and/or stop using parse_long_options, since it 
> doesn't work the way the Gnu Coding Standards says it should? If so, 
> maybe it would be simpler to change parse_long_options instead. This 
> could involve changing the API for parse_long_options.

Thank you very much both for your comments.

Indeed, parse_long_options is not good enough for our purposes
in the following regards:

* it only runs if (argc == 2), i.e., it doesn't handle e.g.
    $ yes --help me

* it stops at the first non-option argument, i.e., it doesn't allow
to handle the GNU standard options after an argument, e.g.
    $ dd if=... --help
(The above is especially nice if one already typed a quite long dd command,
and needs to know one another option, so one could just append --help, and
then re-edit the command replacing --help by the then-known other option.)

* it does not allow to differentiate between the need to scan all
arguments vs. stopping at the first non-option argument;   this is
important for programs launching other programs.  E.g. nohup should
not gobble the --help option after COMMAND:
    $ nohup COMMAND --help

* if a command does not allow other options, then another getopt_long call
is needed.

The attached are quite raw attempts to address this - yes, as a function
instead of a macro. ;-)

The idea is to have a function which allows only the --help or --version
option, and complaining about any other option (well, programs allowing
other options besides the GNU standard options will have their own
getopt_long loop anyway).  And it allows by the SCAN_ALL parameter to
stop or not stop at the 1st non-option argument.

* [PATCH] long-options: add parse_gnu_standard_options_only
  gnulib patch!

* [PATCH] all: detect --help and --version more consistently [FIXME]
  FIXME: NEWS, syntax-check, tests.

Have a nice day,
Berny
[0001-long-options-add-parse_gnu_standard_options_only.patch (text/x-patch, attachment)]
[0001-all-detect-help-and-version-more-consistently-FIXME.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 12 Jan 2019 00:24:01 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 11 Jan 2019 17:23:04 -0700
Hello Berny and all,

On 2018-11-29 1:48 a.m., Bernhard Voelker wrote:
> 
> The attached are quite raw attempts to address this - yes, as a function
> instead of a macro. ;-)
> 
> * [PATCH] long-options: add parse_gnu_standard_options_only
>    gnulib patch!

For the gnulib patch, I believe the following is needed:
====
diff --git a/lib/long-options.c b/lib/long-options.c
index 52ef1f2f8..9567d5135 100644
--- a/lib/long-options.c
+++ b/lib/long-options.c
@@ -139,7 +139,7 @@ parse_gnu_standard_options_only (int argc,
   /* Restore previous value.  */
   opterr = saved_opterr;

-  /* Reset this to zero so that getopt internals get initialized from
+  /* Reset this to one so that getopt internals get initialized from
      the probably-new parameters when/if getopt is called later.  */
-  optind = 0;
+  optind = 1;
 }
====

Otherwise many things fail like so:

  $ ./src/dd
  ./src/dd: unrecognized operand ‘./src/dd’
  Try './src/dd --help' for more information.

The "1" value matches the instructions in the getopt_long(3) man page.

> * [PATCH] all: detect --help and --version more consistently [FIXME]
>    FIXME: NEWS, syntax-check, tests.

With the above 'optind=1' change, there are only two major differences:
---
  $ nohup-8.30 -/ ; echo $?
  nohup: invalid option -- '/'
  Try 'nohup --help' for more information.
  125

  $ ./src/nohup -/ ; echo $?
  src/nohup: invalid option -- '/'
  Try 'src/nohup --help' for more information.
  1


  $ dd-8.30 -- if=/dev/null
  0+0 records in
  0+0 records out
  0 bytes copied, 3.9014e-05 s, 0.0 kB/s

  $ ./src/dd -- if=/dev/null
  ./src/dd: unrecognized operand ‘--’
  Try './src/dd --help' for more information.
---

Which in turn cause "tests/misc/invalid-opt",
"tests/misc/usage_vs_getopt", and "tests/dd/misc" to fail.

All other test pass as before (tested only on Debian Stretch).

regards,
 - assaf

P.S.
https://bugs.gnu.org/29617  "seq: `seq 1 --help' doesn't give help"
will also likely be fixed by your patch.










Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 12 Jan 2019 15:43:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Assaf Gordon <assafgordon <at> gmail.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Sat, 12 Jan 2019 09:42:46 -0600
On 1/11/19 6:23 PM, Assaf Gordon wrote:

> For the gnulib patch, I believe the following is needed:
> ====
> diff --git a/lib/long-options.c b/lib/long-options.c
> index 52ef1f2f8..9567d5135 100644
> --- a/lib/long-options.c
> +++ b/lib/long-options.c
> @@ -139,7 +139,7 @@ parse_gnu_standard_options_only (int argc,
>    /* Restore previous value.  */
>    opterr = saved_opterr;
> 
> -  /* Reset this to zero so that getopt internals get initialized from
> +  /* Reset this to one so that getopt internals get initialized from
>       the probably-new parameters when/if getopt is called later.  */
> -  optind = 0;
> +  optind = 1;

Ouch. You're hitting the portability problem of the difference between
BSD and glibc.

>  }
> ====
> 
> Otherwise many things fail like so:
> 
>   $ ./src/dd
>   ./src/dd: unrecognized operand ‘./src/dd’
>   Try './src/dd --help' for more information.

That's the symptoms on BSD for optind = 0 (there, you HAVE to use
optreset=optind=1 for a complete reset; or plain optind=1 for a soft
reset where the man page is not clear if it will always work).  But on
glibc, optind=1 does a soft reset (works if the optstring does not start
with '-' or '+' and if you did not change POSIXLY_CORRECT), but MUST use
optind = 0 if you want a hard reset.

But shouldn't the real solution be using the gnulib module getopt-gnu
(instead of getopt-posix), so that you are guaranteed that optind=0
works and you don't have to worry about optreset?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Sat, 12 Jan 2019 18:31:02 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Sat, 12 Jan 2019 11:30:49 -0700
Hello Eric,

On 2019-01-12 8:42 a.m., Eric Blake wrote:
> On 1/11/19 6:23 PM, Assaf Gordon wrote:
> 
>> -  optind = 0;
>> +  optind = 1;
> 
> Ouch. You're hitting the portability problem of the difference between
> BSD and glibc.
> 
>>
>> Otherwise many things fail like so:
>>
>>    $ ./src/dd
>>    ./src/dd: unrecognized operand ‘./src/dd’
>>    Try './src/dd --help' for more information.
> 
> That's the symptoms on BSD for optind = 0 (there, you HAVE to use
> optreset=optind=1 for a complete reset; or plain optind=1 for a soft
> reset where the man page is not clear if it will always work).  But on
> glibc, optind=1 does a soft reset (works if the optstring does not start
> with '-' or '+' and if you did not change POSIXLY_CORRECT), but MUST use
> optind = 0 if you want a hard reset.

I only tested on Debian Stretch (with Debian GLIBC 2.24-11+deb9u3),
did not yet test on BSDs.

With "optind=1", I see the following:

===
  $ ./src/hostid
  ec68f06c

  $ ./src/sleep
  ./src/sleep: missing operand
  Try './src/sleep --help' for more information.

  $ ./src/uptime
   11:14:05  up 23 days 21:23,  4 users,  load average: 1.16, 1.05, 0.52

  $ ./src/users
  gordon gordon gordon gordon

  $ ./src/nohup
  ./src/nohup: missing operand
  Try './src/nohup --help' for more information.

  $ ./src/dd       ## waits for CTRL-C
  ^C
  0+0 records in
  0+0 records out
  0 bytes copied, 1.10243 s, 0.0 kB/s

  $ ./src/yes | head -n1
  y
===

With "optind=0" I see the following:

===
  $ ./src/hostid
  ./src/hostid: extra operand ‘./src/hostid’
  Try './src/hostid --help' for more information.

  $ ./src/sleep
  ./src/sleep: missing operand
  Try './src/sleep --help' for more information.

  $ ./src/users

  $ ./src/users | od -tx1
  0000000 02 e2 03 0a
  0000004

  $ ./src/users /var/log/wtmp
  ./src/users: extra operand ‘/var/log/wtmp’
  Try './src/users --help' for more information.

  $ ./src/nohup
  ./src/nohup: ignoring input and appending output to 'nohup.out'
  ^C

  $ ./src/dd
  ./src/dd: unrecognized operand ‘./src/dd’
  Try './src/dd --help' for more information.

  $ ./src/yes | head -n1
  ./src/yes
===

Perhaps "parse_gnu_standard_options_only" should use "_getopt_long_r"
and avoid the need to reset anything?

regards,
 - assaf







Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Wed, 13 Feb 2019 01:22:02 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 12 Feb 2019 18:21:36 -0700
[Message part 1 (text/plain, inline)]
Hello,

A follow-up and more details:

On 2019-01-12 11:30 a.m., Assaf Gordon wrote:
> On 2019-01-12 8:42 a.m., Eric Blake wrote:
>> On 1/11/19 6:23 PM, Assaf Gordon wrote:
>>
>>> -  optind = 0;
>>> +  optind = 1;
>>
>> Ouch. You're hitting the portability problem of the difference between
>> BSD and glibc.
>>
> I only tested on Debian Stretch (with Debian GLIBC 2.24-11+deb9u3),
> did not yet test on BSDs.
> 
> With "optind=1", I see the following:
> 
> ===
>    $ ./src/hostid
>    ec68f06c
[...]
> With "optind=0" I see the following:
> 
> ===
>    $ ./src/hostid
>    ./src/hostid: extra operand ‘./src/hostid’
>    Try './src/hostid --help' for more information.
> 
====

Eric's suggestion was not wrong, "optint=0"
was already used (and worked just fine) in parse_long_option.

But there's a catch: after calling "parse_long_options"
(which sets optind=0), every program called "getopt_long"
again! and that call set optind to non-zero value.

Bernhard's patch removed the (now unneeded) getopt_long call:
===
-  parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE, Version,
-                      usage, AUTHORS, (char const *) NULL);
-  if (getopt_long (argc, argv, "", long_options, NULL) != -1)
-    usage (EXIT_FAILURE);
+  parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE, 
Version,
+                                   true, usage, AUTHORS, (char const *) 
NULL);
===

And so all these programs were left with "optind=0" when the checked 
non-option arguments, e.g.:

===
  if (optind < argc) 

    { 

      error (0, 0, _("extra operand %s"), quote (argv[optind])); 

      usage (EXIT_FAILURE); 

    }
===

which resulted in all the parsing errors I reported previously.

> Perhaps "parse_gnu_standard_options_only" should use "_getopt_long_r"
> and avoid the need to reset anything?
> 

_getopt_long_r was ostensibly fine, but turned out to be messy:
when coreutils is built on glibc systems, all of gnulib's getopt
replacement modules are not used, and so _getopt_long_r is not
available.


As all the programs in this patch accept only --help and --yes
(and non-option arguments), the attached ugly hack seems to solve the
issue.
There's probably a prettier way.

With this patch, the only issues left are nohup's exit code (1 instead 
of 125) and "dd --", see https://bugs.gnu.org/33468#29

regards,
 - assaf	

[0001-all-parse_gnu_standard_options_only-fixup.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Wed, 13 Feb 2019 02:01:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Assaf Gordon <assafgordon <at> gmail.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 12 Feb 2019 20:00:20 -0600
On 2/12/19 7:21 PM, Assaf Gordon wrote:

> Eric's suggestion was not wrong, "optint=0"
> was already used (and worked just fine) in parse_long_option.
> 
> But there's a catch: after calling "parse_long_options"
> (which sets optind=0), every program called "getopt_long"
> again! and that call set optind to non-zero value.
> 
> Bernhard's patch removed the (now unneeded) getopt_long call:
> ===
> -  parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE, Version,
> -                      usage, AUTHORS, (char const *) NULL);
> -  if (getopt_long (argc, argv, "", long_options, NULL) != -1)
> -    usage (EXIT_FAILURE);
> +  parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE,
> Version,
> +                                   true, usage, AUTHORS, (char const *)
> NULL);
> ===
> 
> And so all these programs were left with "optind=0" when the checked
> non-option arguments, e.g.:
> 
> ===
>   if (optind < argc)
>     {
>       error (0, 0, _("extra operand %s"), quote (argv[optind]));
>       usage (EXIT_FAILURE);
>     }
> ===
> 
> which resulted in all the parsing errors I reported previously.

Aha. So the problem isn't resetting the parse, but the fact that
parse_long_options() left the parser in a different state than
parse_gnu_standard_options_only().

 >   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME,
PACKAGE_NAME, Version,
>                                     true, usage, AUTHORS, (char const *) NULL);
> +  optind = 1;

Why are you doing this in every caller, instead of doing it just once
inside the body of parse_gnu_standard_options_only(), so that the state
is left unchanged at optind==1 if there were no options parsed?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Thu, 14 Feb 2019 00:57:01 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Wed, 13 Feb 2019 17:56:39 -0700
[Message part 1 (text/plain, inline)]
Hello,

On 2019-02-12 7:00 p.m., Eric Blake wrote:
> On 2/12/19 7:21 PM, Assaf Gordon wrote:
> 
>> +  optind = 1;
> 
> Why are you doing this in every caller, instead of doing it just once
> inside the body of parse_gnu_standard_options_only(), so that the state
> is left unchanged at optind==1 if there were no options parsed?

That was just an ugly hack.

Here are a more complete patches (both for gnulib and for coreutils).

All existing tests pass (including nohup's exit code)
but I did not yet write new tests for these improvements.

Comments welcomed.
 -assaf





[0001-all-detect-help-and-version-more-consistently-FIXME.patch.gz (application/gzip, attachment)]
[gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch.gz (application/gzip, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Thu, 14 Feb 2019 03:21:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Assaf Gordon <assafgordon <at> gmail.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Wed, 13 Feb 2019 21:20:03 -0600
On 2/13/19 6:56 PM, Assaf Gordon wrote:
> Hello,
> 
> On 2019-02-12 7:00 p.m., Eric Blake wrote:
>> On 2/12/19 7:21 PM, Assaf Gordon wrote:
>>
>>> +  optind = 1;
>>
>> Why are you doing this in every caller, instead of doing it just once
>> inside the body of parse_gnu_standard_options_only(), so that the state
>> is left unchanged at optind==1 if there were no options parsed?
> 
> That was just an ugly hack.
> 
> Here are a more complete patches (both for gnulib and for coreutils).
> 
> All existing tests pass (including nohup's exit code)
> but I did not yet write new tests for these improvements.
> 
> Comments welcomed.
>  -assaf
> 

> From 089e732b25f02c564e9606e8be7d2ab0b6caff98 Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
> Date: Thu, 29 Nov 2018 09:06:26 +0100
> Subject: [PATCH] long-options: add parse_gnu_standard_options_only
> 
> Discussed in https://bugs.gnu.org/33468 .
> 
> * lib/long-options.c (parse_long_options): Use EXIT_SUCCESS instead
> of 0.
> (parse_gnu_standard_options_only): Add function to
> process the GNU default options --help and --version (see also [1]),
> and fail for any other unknown long or short option.
> * lib/long-options.h (parse_gnu_standard_options_only): Declare it.
> [1]
> https://gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html

Putting a [1] footnote in the middle of the ChangeLog section looks odd;
did you mean to sink it lower...

> * modules/long-options (depends-on): Add stdbool, extifail.

exitfail

> * top/maint.mk (sc_prohibit_long_options_without_use): Update
> syntax-check rule, add new function name.
...to here?

> @@ -87,3 +88,65 @@ parse_long_options (int argc,
>       the probably-new parameters when/if getopt is called later.  */
>    optind = 0;
>  }
> +
> +/* Process the GNU default long options --help and --version (see also
> +   https://gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html),
> +   and fail for any other unknown long or short option.
> +   Use with SCAN_ALL=true to scan until "--", or with SCAN_ALL=false to stop
> +   at the first non-option argument (or "--", whichever comes first).
> +   if RESET_OPTIND==TRUE, the global optind variable will be reset to zero,

RESET_OPTIND=true, to match your style of SCAN_ALL=true above

> +   preparing (and requiring) a follow-up getopt_long(3) call.

Not just any getopt_long call, but the glibc-flavored getopt_gnu() [the
getopt-gnu module] (since FreeBSD native getopt_long() uses
optreset=optind=1 instead of optind=0 for a reset).

> +   if RESET_OPTIND==FALSE, optind is left as-is (suitible for programs

RESET_OPTIND=false, suitable

> +   which do not process further optional parameters (but could still
> +   process paramaters directly by examining argv[optind]).  */

parameters

> +void
> +parse_gnu_standard_options_only (int argc,
> +                                 char **argv,
> +                                 const char *command_name,
> +                                 const char *package,
> +                                 const char *version,
> +                                 bool scan_all,
> +                                 bool reset_optind,
> +                                 void (*usage_func) (int),
> +                                 /* const char *author1, ...*/ ...)
> +{

> +  /* Reset this to zero so that getopt internals get initialized from
> +     the probably-new parameters when/if getopt is called later.  */
> +  if (reset_optind)
> +    optind = 0;
> +}

Either this should depend on the getopt-gnu module, or you could do a
configure-check for optreset, and write:

#if HAVE_OPTRESET
  optind = optreset = 1;
#else
  optind = 0;
#endif

to work with both common flavors of getopt_long() adn the getopt-posix
module.

> From 46ff493a864449fae1ad5678f1c79ce4c0eea1ab Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
> Date: Mon, 26 Nov 2018 09:05:37 +0100
> Subject: [PATCH] all: detect --help and --version more consistently [FIXME]
> 
> FIXME: tests
> 
> For select programs which accept only --help and --version options
> (in addition to non-options arguments), process these options before
> any other options.
> 
> Before:
> 
>   $ yes --help me
>   yes: unrecognized option '--help'
>   Try 'yes --help' for more information.
> 
>   $ yes me --help
>   me --help
>   me --help
>   ...
> 
> After:
> Any occurence of '--help' in yes's arguments will show the help screen.

occurrence

Maybe:

show the help screen, unless -- is used to mark end of options.

> 
> Discussed in https://bugs.gnu.org/33468 .
> 
> * NEWS: Mention change.
> * src/cksum.c, src/dd.c, src/hostid.c, src/hostname.c, src/link.c,
> src/logname.c, src/nohup.c, src/sleep.c, src/tsort.c, src/unlink.c,
> src/uptime.c, src/users.c, src/whoami.c, src/yes.c (main): Replace
> parse_long_options() + getopt_long() calls with
> parse_gnu_standard_options_only(); Remove <getopt.h> inclusion;
> Remove empty 'struct long_options' variable;
> ---
>  NEWS           |  4 ++++
>  src/cksum.c    | 13 +++----------
>  src/dd.c       | 14 +++-----------
>  src/hostid.c   | 13 +++----------
>  src/hostname.c | 13 +++----------
>  src/link.c     | 13 +++----------
>  src/logname.c  | 13 +++----------
>  src/nohup.c    | 13 +++----------
>  src/sleep.c    | 13 +++----------
>  src/tsort.c    | 13 +++----------
>  src/unlink.c   | 13 +++----------
>  src/uptime.c   | 13 +++----------
>  src/users.c    | 13 +++----------
>  src/whoami.c   | 13 +++----------
>  src/yes.c      | 13 +++----------
>  15 files changed, 46 insertions(+), 141 deletions(-)

Nice diffstat.

> 
> diff --git a/NEWS b/NEWS
> index fdde47593..5975c2908 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -37,6 +37,10 @@ GNU coreutils NEWS                                    -*- outline -*-
>  
>  ** Changes in behavior
>  
> +  cksum,dd,hostid,hostname,link,logname,nohup,sleep,tsort,unlink,uptime,
> +  users,whoami,yes: these programs now always process --help and --version

Spaces between program names.

> +  before any other option.

Is "argument" better than "option" here?  Or, maybe:

now always process --help and --version options, regardless of any other
arguments present before any optinoal -- end-of-options marker.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Fri, 15 Feb 2019 18:33:01 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 15 Feb 2019 11:32:20 -0700
[Message part 1 (text/plain, inline)]
Hello Eric and all,


Thanks for the quick and detailed review.
I've amended all the issues you mentioned.

On 2019-02-13 8:20 p.m., Eric Blake wrote:
>>   15 files changed, 46 insertions(+), 141 deletions(-)
> 
> Nice diffstat.

These are of course Bernhard's improvements,
I just did the testing (and some minor things).

>> diff --git a/NEWS b/NEWS
> 
> Is "argument" better than "option" here?  Or, maybe:
> 
> now always process --help and --version options, regardless of any other
> arguments present before any optinoal -- end-of-options marker.

I've used your phrasing, and also separated "nohup" from the rest
of the programs, as it does not accept --help/--version anywhere,
just as first arguments.

Attached updated patches, with tests.

comments welcomed,
 - assaf

 P.S.
There is at least one change in behavior, not sure if this is
bad enough to be a regression or doesn't really matter:

  $ yes-OLD me -- --help | head -n1
  me -- --help

  $ yes-NEW me -- --help | head -n1
  me --help

[gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch.gz (application/gzip, attachment)]
[0001-all-detect-help-and-version-more-consistently.patch.gz (application/gzip, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Fri, 15 Feb 2019 20:21:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Assaf Gordon <assafgordon <at> gmail.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 15 Feb 2019 14:19:54 -0600
On 2/15/19 12:32 PM, Assaf Gordon wrote:

> Attached updated patches, with tests.

It's easier to review patches when they are attached inline without
compression (then I don't have to decompress and copy from my editor
back into my email), but I can cope with your .gz attachments.

> 
> comments welcomed,
>  - assaf
> 
>  P.S.
> There is at least one change in behavior, not sure if this is
> bad enough to be a regression or doesn't really matter:
> 
>   $ yes-OLD me -- --help | head -n1
>   me -- --help
> 
>   $ yes-NEW me -- --help | head -n1
>   me --help

I would argue bug-fix. My reference is any other getopt-based utility
where -- has observable effects when treated as an explicit option:

$ echo pattern > ./--
$ grep pattern -- </dev/null
$ POSIXLY_CORRECT=1 grep pattern -- </dev/null
pattern
$ grep -- pattern -- </dev/null
pattern

That is, the value of POSIXLY_CORRECT determines whether to permute the
'end-of-option' behavior of '--' prior to non-options (GNU behavior) or
to strictly treat the first non-option as end-of-options (POSIX
behavior), and the user can always add an explicit -- prior to
non-options to guarantee POSIX behavior regardless of POSIXLY_CORRECT.

So, I would suspect (although I have not yet tesed) that as patched, you
would get:

$ yes-NEW me -- --help | head -n1
me --help
$ POSIXLY_CORRECT=1 yes-NEW me -- --help | head -n1
me -- --help
$ yes-NEW -- me -- --help
me -- --help

In the gnulib patch:
> +   if RESET_OPTIND=false, optind is left as-is (suitable for programs
> +   which do not process further optional parameters (but could still

s/optional/option/

> +   process parameters directly by examining argv[optind]).  */

In the coreutils patch:

> For select programs which accept only --help and --version options
> (in addition to non-options arguments), process these options before

s/non-options/non-option/

> any other options.

Also, all coreutils callers pass reset_optind==false; does the gnulib
interface still need to provide a reset_optind parameter, given that
setting the parameter true forces reliance on the getopt-gnu module as
currently coded?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Mon, 18 Feb 2019 01:38:01 GMT) Full text and rfc822 format available.

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Assaf Gordon <assafgordon <at> gmail.com>, Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Uko Kokņevičs
 <perkontevs <at> gmail.com>, 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Sun, 17 Feb 2019 17:37:08 -0800
On 15/02/19 10:32, Assaf Gordon wrote:
> Hello Eric and all,
> 
> 
> Thanks for the quick and detailed review.
> I've amended all the issues you mentioned.
> 
> On 2019-02-13 8:20 p.m., Eric Blake wrote:
>>>   15 files changed, 46 insertions(+), 141 deletions(-)
>>
>> Nice diffstat.
> 
> These are of course Bernhard's improvements,
> I just did the testing (and some minor things).
> 
>>> diff --git a/NEWS b/NEWS
>>
>> Is "argument" better than "option" here?  Or, maybe:
>>
>> now always process --help and --version options, regardless of any other
>> arguments present before any optinoal -- end-of-options marker.
> 
> I've used your phrasing, and also separated "nohup" from the rest
> of the programs, as it does not accept --help/--version anywhere,
> just as first arguments.
> 
> Attached updated patches, with tests.
> 
> comments welcomed,
>   - assaf
> 
>   P.S.
> There is at least one change in behavior, not sure if this is
> bad enough to be a regression or doesn't really matter:
> 
>    $ yes-OLD me -- --help | head -n1
>    me -- --help
> 
>    $ yes-NEW me -- --help | head -n1
>    me --help

I wouldn't worry about that.
If it could be controlled by POSIXLY_CORRECT all the better,
but still not worth worrying about.
It would be appropriate for a test, at least for documentation.

this is looks ready to land.

thanks!
Pádraig




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Mon, 18 Feb 2019 10:21:01 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Eric Blake <eblake <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>, Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Mon, 18 Feb 2019 03:20:05 -0700
[Message part 1 (text/plain, inline)]
Hello,

On 2019-02-15 1:19 p.m., Eric Blake wrote:
> On 2/15/19 12:32 PM, Assaf Gordon wrote:
>> There is at least one change in behavior, not sure if this is
>> bad enough to be a regression or doesn't really matter:
>>
>>    $ yes-OLD me -- --help | head -n1
>>    me -- --help
>>
>>    $ yes-NEW me -- --help | head -n1
>>    me --help
> 
> I would argue bug-fix.
[...]
> So, I would suspect (although I have not yet tesed) that as patched, you
> would get:
> 
> $ yes-NEW me -- --help | head -n1
> me --help
> $ POSIXLY_CORRECT=1 yes-NEW me -- --help | head -n1
> me -- --help
> $ yes-NEW -- me -- --help
> me -- --help

Indeed - that's how it behaves with the patch.

Thanks for explaining.

> In the gnulib patch:
> s/optional/option/

> In the coreutils patch:
> s/non-options/non-option/

Attached updates with your suggested fixes.


> Also, all coreutils callers pass reset_optind==false; does the gnulib
> interface still need to provide a reset_optind parameter, given that
> setting the parameter true forces reliance on the getopt-gnu module as
> currently coded?

The "getopt-gnu" was already a dependency before this patch,
not sure if removing this parameter will save much hassle - what do you
think ?

-assaf


[gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch (text/x-patch, attachment)]
[0001-all-detect-help-and-version-more-consistently.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Tue, 19 Feb 2019 08:26:02 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Assaf Gordon <assafgordon <at> gmail.com>, Eric Blake <eblake <at> redhat.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 19 Feb 2019 09:24:30 +0100
[Message part 1 (text/plain, inline)]
On 2/18/19 11:20 AM, Assaf Gordon wrote:
> [...] what do you think ?

Thanks for driving this.

To Eric's suggestion, I'd remove the RESET_OPTIND function argument,
because it's never used.
Re. OPTIND: what about resetting the values of all involved externals
to their previous value?

+  int saved_opterr = opterr;
+  int saved_optind = optind;
+  int saved_optopt = optopt;
+  char *saved_optarg = optarg;

...

+  /* Restore previous values.  */
+  opterr = saved_opterr;
+  optind = saved_optind;
+  optopt = saved_optopt;
+  optarg = saved_optarg;


In the attached, I've also changed the test a bit:
- remove unused _cleanup().
- add 'print_ver_ yes' to satisfy sc_env_test_dependencies,
- use case/esac to avoid spawning expr for the matching,
- and fix a missing "|| fail=1" in the final 'yes' test.

*BUT*: for 'yes', we'd introduce a regression due to option reordering:

  $ /usr/bin/yes a -- --help | head -n1
  a -- --help

  $ src/yes a -- --help | head -n1
  -- a --help

Any idea?

Thanks & have a nice day,
Berny
[0001-all-detect-help-and-version-more-consistently.patch (text/x-patch, attachment)]
[gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Tue, 19 Feb 2019 09:54:02 GMT) Full text and rfc822 format available.

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

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Eric Blake <eblake <at> redhat.com>, Paul Eggert <eggert <at> cs.ucla.edu>,
 Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 19 Feb 2019 02:53:21 -0700
Hello,

On 2019-02-19 1:24 a.m., Bernhard Voelker wrote:
> On 2/18/19 11:20 AM, Assaf Gordon wrote:
>> [...] what do you think ?
> 
> To Eric's suggestion, I'd remove the RESET_OPTIND function argument,
> because it's never used.

+1

> Re. OPTIND: what about resetting the values of all involved externals
> to their previous value?
> 
> +  int saved_optind = optind;
> ...
> +  /* Restore previous values.  */
> +  optind = saved_optind;
> 

I believe restoring optind is incorrect here - did the tests pass
after this change ?

For example, I get the following:
---
$ ./src/dd --
./src/dd: unrecognized operand ‘--’
Try './src/dd --help' for more information.

$ ./src/nohup --
./src/nohup: ignoring input and appending output to 'nohup.out'
./src/nohup: failed to run command '--': No such file or directory

$ ./src/yes -- | head -n1
--

---

All these programs expect 'optind' to point to the first non-option
argument (because they all called "getopt_long" directly before your
patch, and parse_gnu_standard_options_only() now calls getopt_long()
for them, indirectly).

So restoring it to its initial value of 1 is going to confuse the
programs when they look into argv[optind] .

Unless I got confused (it's rather late here).
I'll double-check in the morning.

regards,
 - assaf









Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Tue, 19 Feb 2019 13:54:02 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Assaf Gordon <assafgordon <at> gmail.com>, Paul Eggert <eggert <at> cs.ucla.edu>,
 Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Tue, 19 Feb 2019 07:53:19 -0600
On 2/19/19 2:24 AM, Bernhard Voelker wrote:
> On 2/18/19 11:20 AM, Assaf Gordon wrote:
>> [...] what do you think ?
> 
> Thanks for driving this.
> 
> To Eric's suggestion, I'd remove the RESET_OPTIND function argument,
> because it's never used.
> Re. OPTIND: what about resetting the values of all involved externals
> to their previous value?
> 
> +  int saved_opterr = opterr;
> +  int saved_optind = optind;
> +  int saved_optopt = optopt;
> +  char *saved_optarg = optarg;
> 
> ...
> 
> +  /* Restore previous values.  */
> +  opterr = saved_opterr;
> +  optind = saved_optind;
> +  optopt = saved_optopt;
> +  optarg = saved_optarg;

Why? Most callers call this _instead_ of getopt_long(), so they don't
care what things are left at except that optind must point to the first
non-option.  Restoring optind (to 1) is wrong.  And for the rare caller
that DOES want to call getopt_long() themselves afterwards, they can
safe/restore as needed prior to calling this helper.  Restoring state
made sense as long as we had a RESET_OPTIND argument, but now I'm
thinking that it does not buy us any utility at all - just blindly set
opterr without worrying about restoring it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Thu, 21 Feb 2019 08:16:02 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Eric Blake <eblake <at> redhat.com>, Assaf Gordon <assafgordon <at> gmail.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Thu, 21 Feb 2019 09:15:04 +0100
[Message part 1 (text/plain, inline)]
On 2/19/19 2:53 PM, Eric Blake wrote:
> [...] - just blindly set
> opterr without worrying about restoring it.

You are both right, sorry for the confusion.
Adjusted patches attached.

I added some more test cases as well.
Finally, I documented the change in 'yes' in NEWS regarding:

  $ yes a -- b | head -n1
  a -- b
vs.
  $ src/yes a -- b | head -n1
  a b

Slowly coming to an end of this story ... (hopefully).

Thanks & have a nice day,
Berny
[gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch (text/x-patch, attachment)]
[0001-all-detect-help-and-version-more-consistently.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Fri, 22 Feb 2019 07:47:02 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Eric Blake <eblake <at> redhat.com>, Assaf Gordon <assafgordon <at> gmail.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 22 Feb 2019 08:46:01 +0100
On 2/21/19 9:15 AM, Bernhard Voelker wrote:
> On 2/19/19 2:53 PM, Eric Blake wrote:
>> [...] - just blindly set
>> opterr without worrying about restoring it.
> 
> You are both right, sorry for the confusion.
> Adjusted patches attached.
> 
> I added some more test cases as well.
> Finally, I documented the change in 'yes' in NEWS regarding:
> 
>   $ yes a -- b | head -n1
>   a -- b
> vs.
>   $ src/yes a -- b | head -n1
>   a b

What about putting 'yes' into the 'nohup' category?  I mean, we have
the SCAN_ALL flag in our new function, so why not simply use it?

diff --git a/src/yes.c b/src/yes.c
index 0864186f9..477872e92 100644
--- a/src/yes.c
+++ b/src/yes.c
@@ -67,7 +67,7 @@ main (int argc, char **argv)
   atexit (close_stdout);

   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE_NAME,
-                                   Version, true, usage, AUTHORS,
+                                   Version, false, usage, AUTHORS,
                                    (char const *) NULL);

   char **operands = argv + optind;


That makes:

  $ src/yes a -- b | sed 1q
  a -- b

and also recognizing --help when other arguments are following:

  $ src/yes --help a -- b | sed 1q
  Usage: src/yes [STRING]...

That is actually our intention, no?
WDYT?

Thanks & have a nice day,
Berny




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Fri, 22 Feb 2019 15:07:01 GMT) Full text and rfc822 format available.

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

From: Eric Blake <eblake <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Assaf Gordon <assafgordon <at> gmail.com>, Paul Eggert <eggert <at> cs.ucla.edu>,
 Pádraig Brady <P <at> draigBrady.com>,
 Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 22 Feb 2019 09:06:08 -0600
On 2/22/19 1:46 AM, Bernhard Voelker wrote:

>> Finally, I documented the change in 'yes' in NEWS regarding:
>>
>>   $ yes a -- b | head -n1
>>   a -- b
>> vs.
>>   $ src/yes a -- b | head -n1
>>   a b
> 
> What about putting 'yes' into the 'nohup' category?  I mean, we have
> the SCAN_ALL flag in our new function, so why not simply use it?

My opinion: SCAN_ALL should ONLY be used by forwarding apps (nohup, env,
nice, ...). yes is not a forwarding app. GNU Coding Standards demand
that unless you have a strong reason to forbid option reordering (and
being a forwarding app is such a strong reason), then you should default
to allowing argument reordering, which means this is a bug-fix for 'yes'
and not a regression.


> That makes:
> 
>   $ src/yes a -- b | sed 1q
>   a -- b
> 
> and also recognizing --help when other arguments are following:
> 
>   $ src/yes --help a -- b | sed 1q
>   Usage: src/yes [STRING]...

But breaks:

$ src/yes a --help

into outputting "a --help" infinitely instead of giving help, which was
the whole point of this exercise.

> 
> That is actually our intention, no?
> WDYT?

Bad idea, for the reasons given above. Leave it as-is, with the change
in yes behavior being an intentional bug fix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




Information forwarded to bug-coreutils <at> gnu.org:
bug#33468; Package coreutils. (Fri, 22 Feb 2019 19:51:01 GMT) Full text and rfc822 format available.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Eric Blake <eblake <at> redhat.com>, Assaf Gordon <assafgordon <at> gmail.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468 <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Fri, 22 Feb 2019 20:50:10 +0100
On 2/22/19 4:06 PM, Eric Blake wrote:
> Bad idea, for the reasons given above. Leave it as-is, with the change
> in yes behavior being an intentional bug fix.

Good point(s).

So the latest patch set [*] is good to push?
If yes: I don't have push perms on gnulib, so if someone of you would
be so kind (with adjusted ChangeLog) ...

[*] http://lists.gnu.org/archive/html/bug-coreutils/2019-02/msg00078.html

Thanks & have a nice day,
Berny




Reply sent to Bernhard Voelker <mail <at> bernhard-voelker.de>:
You have taken responsibility. (Mon, 25 Feb 2019 07:30:02 GMT) Full text and rfc822 format available.

Notification sent to Uko Kokņevičs <perkontevs <at> gmail.com>:
bug acknowledged by developer. (Mon, 25 Feb 2019 07:30:02 GMT) Full text and rfc822 format available.

Message #85 received at 33468-done <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Eric Blake <eblake <at> redhat.com>, Assaf Gordon <assafgordon <at> gmail.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>, Uko Kokņevičs <perkontevs <at> gmail.com>,
 33468-done <at> debbugs.gnu.org
Subject: Re: bug#33468: A bug with yes and --help
Date: Mon, 25 Feb 2019 08:29:08 +0100
On 2/22/19 8:50 PM, Bernhard Voelker wrote:
> So the latest patch set [*] is good to push?

Padraig pushed the gnulib change at:
  https://git.sv.gnu.org/cgit/gnulib.git/commit/?id=e3970fb989
and sync'd coreutils to that commit.

I pushed the CU change on top at:
https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=44af8426

Marking this as done.

Thanks for all the "--help"-ful discussion. ;-)

Have a nice day,
Berny




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 25 Mar 2019 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 5 years and 5 days ago.

Previous Next


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