bug#33468: A bug with yes and --help

2019-02-24 Thread Bernhard Voelker 
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#33468: A bug with yes and --help

2019-02-22 Thread Bernhard Voelker 
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

bug#33468: A bug with yes and --help

2019-02-22 Thread Eric Blake 
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

bug#33468: A bug with yes and --help

2019-02-21 Thread Bernhard Voelker 
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

bug#33468: A bug with yes and --help

2019-02-21 Thread Bernhard Voelker 
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
>From 9d87004e60b20c74524e9ed83f2253101b94a986 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
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 and fail for any other
unknown long or short option. See
https://gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html .
* lib/long-options.h (parse_gnu_standard_options_only): Declare it.
* modules/long-options (depends-on): Add stdbool, exitfail.
* top/maint.mk (sc_prohibit_long_options_without_use): Update
syntax-check rule, add new function name.
---
 lib/long-options.c   | 52 +++-
 lib/long-options.h   | 16 ++
 modules/long-options |  2 ++
 top/maint.mk |  2 +-
 4 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/lib/long-options.c b/lib/long-options.c
index 037f74b3a..cbdc6a98b 100644
--- a/lib/long-options.c
+++ b/lib/long-options.c
@@ -29,6 +29,7 @@
 #include 
 
 #include "version-etc.h"
+#include "exitfail.h"
 
 static struct option const long_options[] =
 {
@@ -71,7 +72,7 @@ parse_long_options (int argc,
 va_list authors;
 va_start (authors, usage_func);
 version_etc_va (stdout, command_name, package, version, authors);
-exit (0);
+exit (EXIT_SUCCESS);
   }
 
 default:
@@ -87,3 +88,52 @@ 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).  */
+void
+parse_gnu_standard_options_only (int argc,
+ char **argv,
+ const char *command_name,
+ const char *package,
+ const char *version,
+ bool scan_all,
+ void (*usage_func) (int),
+ /* const char *author1, ...*/ ...)
+{
+  int c;
+  int saved_opterr = opterr;
+
+  /* Print an error message for unrecognized options.  */
+  opterr = 1;
+
+  const char *optstring = scan_all ? "" : "+";
+
+  if ((c = getopt_long (argc, argv, optstring, long_options, NULL)) != -1)
+{
+  switch (c)
+{
+case 'h':
+  (*usage_func) (EXIT_SUCCESS);
+  break;
+
+case 'v':
+  {
+va_list authors;
+va_start (authors, usage_func);
+version_etc_va (stdout, command_name, package, version, authors);
+exit (EXIT_SUCCESS);
+  }
+
+default:
+  (*usage_func) (exit_failure);
+  break;
+}
+}
+
+  /* Restore previous value.  */
+  opterr = saved_opterr;
+}
diff --git a/lib/long-options.h b/lib/long-options.h
index f4d6a83d5..13f17eccc 100644
--- a/lib/long-options.h
+++ b/lib/long-options.h
@@ -17,6 +17,11 @@
 
 /* Written by Jim Meyering.  */
 
+#ifndef LONG_OPTIONS_H_
+# define LONG_OPTIONS_H_ 1
+
+# include 
+
 void parse_long_options (int _argc,
  char **_argv,
  const char *_command_name,
@@ -24,3 +29,14 @@ void parse_long_options (int _argc,
  const char *_version,
  void (*_usage) (int),
  /* const char *author1, ...*/ ...);
+
+void parse_gnu_standard_options_only (int argc,
+  char **argv,
+  const char *command_name,
+  const char *package,
+  const char *version,
+  bool scan_all,
+  void (*usage_func) (int),
+  /* const char *author1, ...*/ ...);
+
+#endif /* LONG_OPTIONS_H_ */
diff --git a/modules/long-options b/modules/long-options
index f1106c3cc..b845d4293 100644
--- a/modules/long-options
+++ b/modules/long-options
@@

bug#33468: A bug with yes and --help

2019-02-19 Thread Eric Blake 
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

bug#33468: A bug with yes and --help

2019-02-19 Thread Assaf Gordon 

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

bug#33468: A bug with yes and --help

2019-02-19 Thread Bernhard Voelker 
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
>From 0703d2520f79b499385d2f0617aac15203377415 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
Date: Mon, 26 Nov 2018 09:05:37 +0100
Subject: [PATCH] all: detect --help and --version more consistently

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

Before:

  $ dd bs=1 --help
  dd: unrecognized option '--help'
  Try 'dd --help' for more information.

  $ yes me --help
  me --help
  me --help
  ...

After:
Any occurrence of '--help' in the arguments (prior to '--') will
show the help screen.

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  inclusion;
Remove empty 'struct long_options' variable;
* tests/misc/help-version-getopt.sh: New test.
* tests/local.mk (all_tests): Add new test.
---
 NEWS  |  8 +
 src/cksum.c   | 12 ++-
 src/dd.c  | 13 ++-
 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 ++-
 tests/local.mk|  1 +
 tests/misc/help-version-getopt.sh | 57 +++
 17 files changed, 106 insertions(+), 141 deletions(-)
 create mode 100755 tests/misc/help-version-getopt.sh

diff --git a/NEWS b/NEWS
index fdde47593..5757602d6 100644
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,14 @@ GNU coreutils NEWS-*- outline -*-
 
 ** Changes in behavior
 
+  cksum, dd, hostid, hostname, link, logname, sleep, tsort, unlink,
+  uptime, users, whoami, yes: now always process --help and --version options,
+  regardless of any other arguments present before any optional '--'
+  end-of-options marker.
+
+  nohup: now process --help and --version as first options even if other
+  parameters follow.
+
   echo now always processes backslash escapes when the POSIXLY_CORRECT
   environment variable is set.
 
diff --git a/src/cksum.c b/src/cksum.c
index 6974724ed..c60510b4f 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -102,16 +102,10 @@ main (void)
 
 #else /* !CRCTAB */
 
-# include 
 # include "long-options.h"
 # include "die.h"
 # include "error.h"
 
-static struct option const long_options[] =
-{
-  {NULL, 0, NULL, 0}
-};
-
 /* Number of bytes to read at once.  */
 # define BUFLEN (1 << 16)
 
@@ -294,10 +288,8 @@ main (int argc, char **argv)
  so that processes running in parallel do not intersperse their output.  */
   setvbuf (stdout, NULL, _IOLBF, 0);
 
-  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);
 
   have_read_stdin = false;
 
diff --git a/src/dd.c b/src/dd.c
index 28611b3e6..2888b8e33 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -22,7 +22,6 @@
 
 #include 
 #include 
-#include 
 
 #include "system.h"
 #include "close-stream.h"
@@ -46,11 +45,6 @@
   proper_name ("David MacKenzie"), \
   proper_name ("Stuart

bug#33468: A bug with yes and --help

2019-02-18 Thread Assaf Gordon 

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


>From 08d0505683cebed0fc10cff082255fd79da2d989 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
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 and fail for any other
unknown long or short option. See
https://gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html .
* lib/long-options.h (parse_gnu_standard_options_only): Declare it.
* modules/long-options (depends-on): Add stdbool, exitfail.
* top/maint.mk (sc_prohibit_long_options_without_use): Update
syntax-check rule, add new function name.
---
 lib/long-options.c   | 68 +++-
 lib/long-options.h   | 17 +
 modules/long-options |  2 ++
 top/maint.mk |  2 +-
 4 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/lib/long-options.c b/lib/long-options.c
index 037f74b3a..b7acdb040 100644
--- a/lib/long-options.c
+++ b/lib/long-options.c
@@ -29,6 +29,7 @@
 #include 
 
 #include "version-etc.h"
+#include "exitfail.h"
 
 static struct option const long_options[] =
 {
@@ -71,7 +72,7 @@ parse_long_options (int argc,
 va_list authors;
 va_start (authors, usage_func);
 version_etc_va (stdout, command_name, package, version, authors);
-exit (0);
+exit (EXIT_SUCCESS);
   }
 
 default:
@@ -87,3 +88,68 @@ 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,
+   preparing (and requiring) a follow-up gnu-compatible getopt() call
+   (non-gnu getopt functions use optreset=optind=1 instead of 0 for reset).
+
+   if RESET_OPTIND=false, optind is left as-is (suitable for programs
+   which do not process further option parameters (but could still
+   process parameters directly by examining argv[optind]).  */
+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, ...*/ ...)
+{
+  int c;
+  int saved_opterr;
+
+  saved_opterr = opterr;
+
+  /* Print an error message for unrecognized options.  */
+  opterr = 1;
+
+  const char *optstring = scan_all ? "" : "+";
+
+  if ((c = getopt_long (argc, argv, optstring, long_options, NULL)) != -1)
+{
+  switch (c)
+{
+case 'h':
+  (*usage_func) (EXIT_SUCCESS);
+  break;
+
+case 'v':
+  {
+va_list authors;
+va_start (authors, usage_func);
+version_etc_va (stdout, command_name, package, version, authors);
+exit (EXIT_SUCCESS);
+  }
+
+default:
+  (*usage_func) (exit_failure);
+  break;
+}
+}
+
+  /* Restore previous value.  */
+  opterr = saved_opterr;
+
+  /* Reset this to zero so

bug#33468: A bug with yes and --help

2019-02-17 Thread Pádraig Brady 
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

bug#33468: A bug with yes and --help

2019-02-15 Thread Eric Blake 
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 --  +   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

bug#33468: A bug with yes and --help

2019-02-15 Thread Assaf Gordon 

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
Description: application/gzip


0001-all-detect-help-and-version-more-consistently.patch.gz
Description: application/gzip

bug#33468: A bug with yes and --help

2019-02-13 Thread Eric Blake 
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 
> 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 
> 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  inclusion;
> Remove empty 'struct long_options' variable;
> ---
>  NEWS   |  4 
>  src/cksum.c

bug#33468: A bug with yes and --help

2019-02-13 Thread Assaf Gordon 

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
Description: application/gzip


gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch.gz
Description: application/gzip

bug#33468: A bug with yes and --help

2019-02-12 Thread Eric Blake 
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

bug#33468: A bug with yes and --help

2019-02-12 Thread Assaf Gordon 

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

>From eb4ed1a5417a2d50941181aa1d8e06b674c661a8 Mon Sep 17 00:00:00 2001
From: Assaf Gordon 
Date: Tue, 12 Feb 2019 17:58:47 -0700
Subject: [PATCH] all: parse_gnu_standard_options_only fixup

---
 src/cksum.c| 1 +
 src/dd.c   | 1 +
 src/hostid.c   | 1 +
 src/hostname.c | 1 +
 src/link.c | 1 +
 src/logname.c  | 1 +
 src/nohup.c| 1 +
 src/sleep.c| 1 +
 src/tsort.c| 1 +
 src/unlink.c   | 1 +
 src/uptime.c   | 1 +
 src/users.c| 1 +
 src/whoami.c   | 1 +
 src/yes.c  | 1 +
 14 files changed, 14 insertions(+)

diff --git a/src/cksum.c b/src/cksum.c
index cda61516a..b62249862 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -291,6 +291,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE, Version,
true, usage, AUTHORS, (char const *) NULL);
+  optind = 1;
 
   have_read_stdin = false;
 
diff --git a/src/dd.c b/src/dd.c
index b361e7d5a..f47e8a788 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -2393,6 +2393,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE, Version,
true, usage, AUTHORS, (char const *) NULL);
+  optind = 1;
   close_stdout_required = false;
 
   /* Initialize translation table to identity translation. */
diff --git a/src/hostid.c b/src/hostid.c
index d9ea8929b..f023a3da1 100644
--- a/src/hostid.c
+++ b/src/hostid.c
@@ -66,6 +66,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE_NAME, Version,
true, usage, AUTHORS, (char const *) NULL);
+  optind = 1;
 
   if (optind < argc)
 {
diff --git a/src/hostname.c b/src/hostname.c
index 761f775b4..3a9d1dd80 100644
--- a/src/hostname.c
+++ b/src/hostname.c
@@ -83,6 +83,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE_NAME, Version,
true, usage, AUTHORS, (char const *) NULL);
+  optind = 1;
 
   if (argc == optind + 1)
 {
diff --git a/src/link.c b/src/link.c
index d70d434d9..d21f36099 100644
--- a/src/link.c
+++ b/src/link.c
@@ -69,6 +69,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv, PROGRAM_NAME, PACKAGE_NAME, Version,
true, usage, AUTHORS, (char const *) NULL);
+  optind = 1;
 
   if (argc < optind + 2)
 {
diff --git a/src/logname.c b/src/logname.c
index cea43720f..fb9c2bbab 100644
--- a/src/logname.c
+++ b/src/logname.c
@@ -64,6 +64,7 @@ main (int argc, char **argv)
 
   parse_gnu_standard_options_only (argc, argv,

bug#33468: A bug with yes and --help

2019-01-12 Thread Assaf Gordon 

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
  000 02 e2 03 0a
  004

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

bug#33468: A bug with yes and --help

2019-01-12 Thread Eric Blake 
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

bug#33468: A bug with yes and --help

2019-01-11 Thread Assaf Gordon 

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.

bug#33468: A bug with yes and --help

2018-11-29 Thread Bernhard Voelker 
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
>From a882a0bdf4b2d19b25f42bb2d4194b526dde8589 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
Date: Thu, 29 Nov 2018 09:06:26 +0100
Subject: [PATCH] long-options: add parse_gnu_standard_options_only

* 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
---
 ChangeLog| 12 +
 lib/long-options.c   | 58 +++-
 lib/long-options.h   | 16 
 modules/long-options |  1 +
 4 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index af7a14fa9..3b25962b6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2018-11-29  Bernhard Voelker  
+
+	long-options: add parse_gnu_standard_options_only
+	* 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
+
 2018-11-29  Akim Demaille  
 
 	bitset: rename ebitset/expandable.* as tbitset/table.*
diff --git a/lib/long-options.c b/lib/long-options.c
index eef1f2f35..b3f1df404 100644
--- a/lib/long-options.c
+++ b/lib/long-options.c
@@ -71,7 +71,7 @@ parse_long_options (int argc,
 va_list authors;
 va_start (authors, usage_func);
 version_etc_va (stdout, command_name, package, version, authors);
-exit (0);
+exit (EXIT_SUCCESS);
   }
 
 default:
@@ -87,3 +87,59 @@ 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).  */
+
+void
+parse_gnu_standard_options_only (int argc,
+ char **argv,
+ const char *command_name,
+ const char *package,
+ const char *version,
+ bool scan_all,
+ void (*usage_func) (int),
+ /* const char *author1, ...*/ ...)
+{
+  int c;
+  int saved_opterr;
+
+  saved_opterr = opterr;
+
+  /* Print an error

bug#33468: A bug with yes and --help

2018-11-27 Thread Paul Eggert 
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.

bug#33468: A bug with yes and --help

2018-11-26 Thread Eric Blake 

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

bug#33468: A bug with yes and --help

2018-11-26 Thread Bernhard Voelker 
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
From c203a09e3b1bea5a15a30dae099183291aa43d8e Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
Date: Mon, 26 Nov 2018 09:05:37 +0100
Subject: [PATCH] all: detect --help and --version more consistently [FIXME]

FIXME: syntax-check, tests.
---
 src/cksum.c| 11 ++-
 src/dd.c   | 19 ++-
 src/hostid.c   | 11 ++-
 src/hostname.c | 11 ++-
 src/link.c | 11 ++-
 src/logname.c  | 11 ++-
 src/nohup.c| 11 ++-
 src/sleep.c| 11 ++-
 src/system.h   | 24 
 src/tsort.c| 11 ++-
 src/unlink.c   | 11 ++-
 src/uptime.c   | 11 ++-
 src/users.c| 11 ++-
 src/whoami.c   | 11 ++-
 src/yes.c  | 11 ++-
 15 files changed, 64 insertions(+), 122 deletions(-)

diff --git a/src/cksum.c b/src/cksum.c
index 372d6b601..d21f70a9a 100644
--- a/src/cksum.c
+++ b/src/cksum.c
@@ -107,11 +107,6 @@ main (void)
 # include "die.h"
 # include "error.h"
 
-static struct option const long_options[] =
-{
-  {NULL, 0, NULL, 0}
-};
-
 /* Number of bytes to read at once.  */
 # define BUFLEN (1 << 16)
 
@@ -294,10 +289,8 @@ main (int argc, char **argv)
  so that processes running in parallel do not intersperse their output.  */
   setvbuf (stdout, NULL, _IOLBF, 0);
 
-  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);
+  /* Scan over all args to look for '--help' or '--version'.  */
+  emit_help_or_version_info (true);
 
   have_read_stdin = false;
 
diff --git a/src/dd.c b/src/dd.c
index 6aeef520a..c8781dee2 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -48,6 +48,8 @@
 
 static struct option const long_options[] =
 {
+  {GETOPT_HELP_OPTION_DECL},
+  {GETOPT_VERSION_OPTION_DECL},
   {NULL, 0, NULL, 0}
 };
 
@@ -2396,12 +2398,19 @@ main (int argc, char **argv)
 
   page_size = getpagesize ();
 
-  parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE, Version,
-  usage, AUTHORS, (char const *) NULL);
-  close_stdout_required = false;
+  int c;
+  if ((c = getopt_long (argc, argv, "", long_options, NULL)) != -1)
+{
+  switch (c)
+{
+case_GETOPT_HELP_CHAR;
+case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
+default:
+  usage (EXIT_FAILURE);
+}
+}
 
-  if (getopt_long (argc, argv, "", long_options, NULL) != -1)
-usage (EXIT_FAILURE);
+  close_stdout_required = false;
 
   /* Initialize translation table to identity translation. */
   for (i = 0; i < 256; i++)
diff --git a/src/hostid.c b/src/hostid.c
index c45328aea..4050aee2f 100644
--- a/src/hostid.c
+++ b/src/hostid.c
@@ -32,11 +32,6 @@
 
 #define AUTHORS proper_name ("Jim Meyering")
 
-static struct option const long_options[] =
-{
-  {NULL, 0, NULL, 0}
-};
-
 void
 usage (int status)
 {
@@ -69,10 +64,8 @@ main (int argc, char **argv)
 
   atexit (close_stdout);
 
-  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);
+  /* Scan over all args to look for '--help' or '--version'.  */
+  emit_help_or_version_info (true);
 
   if (optind < argc)
 {
diff --git a/src/hostname.c b/src/hostname.c
index 292c148ec..c8d51a4c8 100644
--- a/src/hostname.c
+++ b/src/hostname.c
@@ -33,11 +33,6 @@
 
 #define AUTHORS proper_name ("Jim Meyering")
 
-static struct option const long_options[] =
-{
-  {NULL, 0, NULL, 0}
-};
-
 #if !defined

bug#33468: A bug with yes and --help

2018-11-24 Thread Paul Eggert 

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

bug#33468: A bug with yes and --help

2018-11-24 Thread Bernhard Voelker 



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

bug#33468: A bug with yes and --help

2018-11-23 Thread Pádraig Brady 
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
> .
>> 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

bug#33468: A bug with yes and --help

2018-11-22 Thread Uko Kokņevičs 
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
.
> 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)