Re: xstrtol.h

[Jim Meyering]

Hi Bruno,

Bruno Haible <address@hidden> wrote:
...
> ./src/ls.c:       human_output_opts = human_options (optarg, true, 
> &output_block_size);
> --block-size argument
>
> I can prepare a patch for this; just let me know whether for
>    $ od -j -1
> you would see the error message
>    od: invalid --skip-bytes argument `-1'
> or
>    od: invalid `-j' argument `-1'

You've discovered why the current diagnostics do not mention
explicit option strings: hard-coding an option string like --skip-bytes
or -j will inevitably be misleading to those who specify the offending
argument with the alternative option string.

E.g., od -j -1 complaining about the --skip-bytes argument is
not accurate, since it should speak of the "-j" argument that was
actually used.  The user may not even be aware of the long-named
equivalent.  Likewise, the user of a long-named option may not even
realize that there is a short-named equivalent.
Similarly for pr +1:9999999999 vs. --pages=1:9999999999
and the others.  Worse still, at one point, you could get a diagnostic
about a --block-size style argument coming from an environment variable,
so talking about *any* specific command-line argument was just plain wrong.
Now, at least in coreutils, if an envvar value is invalid, it is
silently ignored, so that's not an issue.

Finally, I didn't want to limit xstrtol.h to values specified via
command line arguments.  However, since that appears to be the only
type of use, I'm willing to compromise on that front.

I prepared a patch to make od, pr, and sort mention the actual
option name (long or short) in the diagnostic: [the only compromise
is that "pr +-1" mentions "--pages", since there is no short option name]

  $ src/sort --buffer=-1
  src/sort: invalid --buffer-size argument `-1'
  [Exit 2]
  $ src/sort -S -1
  src/sort: invalid -S argument `-1'
  [Exit 2]

If, in the next few days, no one tells me of a use of an xstrtol.h macro
applied to a non-option-argument, I'll check it in.  With a few tests,
too, of course...

However, I'm not completely pleased with this patch: I have substituted
" argument" into each of the xstrtol.h diagnostics.  Doing that suggests that
every user of e.g., STRTOL_FATAL_ERROR should adapt.  Unfortunately, adapting
human.c (human_options) may not be as easy.  It seems like it would require
an API addition, since there is currently no way to inform that function of
whether it's being invoked due to a "-B invalid" or "--block-size=invalid"
or something else.  On the other hand, I wouldn't be surprised to learn that
most translations of _("block size") inserted into each of xstrtol.h's three
messages -- now, with the " argument" addition -- are good enough that we
don't need to worry about it.

If, on the other hand, I had left the diagnostics in xstrtol.h as they were,
I'd then have to format the variable "option_name" into a translatable
string like _("%s argument"), and pass that result to STRTOL_FATAL_ERROR.
That's ugly, inefficient, and would probably produce bad translations, too.

Here's the xstrtol.h change, then the ones for coreutils:

Index: lib/xstrtol.h
===================================================================
RCS file: /sources/gnulib/gnulib/lib/xstrtol.h,v
retrieving revision 1.22
diff -u -p -r1.22 xstrtol.h
--- lib/xstrtol.h       19 Oct 2006 07:51:14 -0000      1.22
+++ lib/xstrtol.h       22 Jul 2007 16:12:54 -0000
@@ -1,6 +1,6 @@
 /* A more useful interface to strtol.

-   Copyright (C) 1995, 1996, 1998, 1999, 2001, 2002, 2003, 2004, 2006
+   Copyright (C) 1995, 1996, 1998, 1999, 2001-2004, 2006-2007
    Free Software Foundation, Inc.

    This program is free software; you can redistribute it and/or modify
@@ -59,19 +59,19 @@ _DECLARE_XSTRTOL (xstrtoumax, uintmax_t)
          abort ();                                                     \
                                                                        \
        case LONGINT_INVALID:                                           \
-         error ((Exit_code), 0, gettext ("invalid %s `%s'"),           \
+         error ((Exit_code), 0, gettext ("invalid %s argument `%s'"),  \
                 (Argument_type_string), (Str));                        \
          break;                                                        \
                                                                        \
        case LONGINT_INVALID_SUFFIX_CHAR:                               \
        case LONGINT_INVALID_SUFFIX_CHAR | LONGINT_OVERFLOW:            \
          error ((Exit_code), 0,                                        \
-                gettext ("invalid character following %s in `%s'"),    \
+                gettext ("invalid character following %s argument in `%s'"), \
                 (Argument_type_string), (Str));                        \
          break;                                                        \
                                                                        \
        case LONGINT_OVERFLOW:                                          \
-         error ((Exit_code), 0, gettext ("%s `%s' too large"),         \
+         error ((Exit_code), 0, gettext ("%s argument `%s' too large"), \
                 (Argument_type_string), (Str));                        \
          break;                                                        \
        }                                                               \

diff --git a/src/od.c b/src/od.c
index c2d419b..79bd4f1 100644
--- a/src/od.c
+++ b/src/od.c
@@ -1,5 +1,5 @@
 /* od -- dump files in octal and other formats
-   Copyright (C) 92, 1995-2006 Free Software Foundation, Inc.
+   Copyright (C) 92, 1995-2007 Free Software Foundation, Inc.

    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1564,6 +1564,7 @@ main (int argc, char **argv)
   bool modern = false;
   bool width_specified = false;
   bool ok = true;
+  int oi;
   static char const multipliers[] = "bEGKkMmPTYZ0";

   /* The old-style `pseudo starting address' to be printed in parentheses
@@ -1610,7 +1611,7 @@ main (int argc, char **argv)
   address_pad_len = 7;
   flag_dump_strings = false;

-  while ((c = getopt_long (argc, argv, short_options, long_options, NULL))
+  while ((c = getopt_long (argc, argv, short_options, long_options, &oi))
         != -1)
     {
       uintmax_t tmp;
@@ -1654,7 +1655,8 @@ it must be one character from [doxn]"),
          modern = true;
          s_err = xstrtoumax (optarg, NULL, 0, &n_bytes_to_skip, multipliers);
          if (s_err != LONGINT_OK)
-           STRTOL_FATAL_ERROR (optarg, _("skip argument"), s_err);
+           STRTOL_FATAL_ERROR (optarg,
+                               OPT_STR (oi, c, long_options), s_err);
          break;

        case 'N':
@@ -1664,7 +1666,7 @@ it must be one character from [doxn]"),
          s_err = xstrtoumax (optarg, NULL, 0, &max_bytes_to_format,
                              multipliers);
          if (s_err != LONGINT_OK)
-           STRTOL_FATAL_ERROR (optarg, _("limit argument"), s_err);
+           STRTOL_FATAL_ERROR (optarg, OPT_STR (oi, c, long_options), s_err);
          break;

        case 'S':
@@ -1675,7 +1677,8 @@ it must be one character from [doxn]"),
            {
              s_err = xstrtoumax (optarg, NULL, 0, &tmp, multipliers);
              if (s_err != LONGINT_OK)
-               STRTOL_FATAL_ERROR (optarg, _("minimum string length"), s_err);
+               STRTOL_FATAL_ERROR (optarg,
+                                   OPT_STR (oi, c, long_options), s_err);

              /* The minimum string length may be no larger than SIZE_MAX,
                 since we may allocate a buffer of this size.  */
@@ -1747,7 +1750,8 @@ it must be one character from [doxn]"),
              uintmax_t w_tmp;
              s_err = xstrtoumax (optarg, NULL, 10, &w_tmp, "");
              if (s_err != LONGINT_OK)
-               STRTOL_FATAL_ERROR (optarg, _("width specification"), s_err);
+               STRTOL_FATAL_ERROR (optarg,
+                                   OPT_STR (oi, c, long_options), s_err);
              if (SIZE_MAX < w_tmp)
                error (EXIT_FAILURE, 0, _("%s is too large"), optarg);
              desired_width = w_tmp;
diff --git a/src/pr.c b/src/pr.c
index 8a65ca0..ca75226 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -804,7 +804,7 @@ first_last_page (char const *pages)
   uintmax_t last = UINTMAX_MAX;
   strtol_error err = xstrtoumax (pages, &p, 10, &first, "");
   if (err != LONGINT_OK && err != LONGINT_INVALID_SUFFIX_CHAR)
-    _STRTOL_ERROR (EXIT_FAILURE, pages, _("page range"), err);
+    _STRTOL_ERROR (EXIT_FAILURE, pages, "--pages", err);

   if (p == pages || !first)
     return false;
@@ -814,7 +814,7 @@ first_last_page (char const *pages)
       char const *p1 = p + 1;
       err = xstrtoumax (p1, &p, 10, &last, "");
       if (err != LONGINT_OK)
-       _STRTOL_ERROR (EXIT_FAILURE, pages, _("page range"), err);
+       _STRTOL_ERROR (EXIT_FAILURE, pages, "--pages", err);
       if (p1 == p || last < first)
        return false;
     }
diff --git a/src/sort.c b/src/sort.c
index 824dd0d..874b188 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1033,7 +1033,7 @@ inittables (void)

 /* Specify the amount of main memory to use when sorting.  */
 static void
-specify_sort_size (char const *s)
+specify_sort_size (char const *s, char const *option_name)
 {
   uintmax_t n;
   char *suffix;
@@ -1089,7 +1089,7 @@ specify_sort_size (char const *s)
       e = LONGINT_OVERFLOW;
     }

-  STRTOL_FATAL_ERROR (s, _("sort size"), e);
+  STRTOL_FATAL_ERROR (s, option_name, e);
 }

 /* Return the default sort size.  */
@@ -2834,6 +2834,7 @@ main (int argc, char **argv)

   for (;;)
     {
+      int oi = -1;
       /* Parse an operand as a file after "--" was seen; or if
         pedantic and a file was seen, unless the POSIX version
         predates 1003.1-2001 and -c was not seen and the operand is
@@ -2847,7 +2848,7 @@ main (int argc, char **argv)
                    && argv[optind][0] == '-' && argv[optind][1] == 'o'
                    && (argv[optind][2] || optind + 1 != argc)))
          || ((c = getopt_long (argc, argv, short_options,
-                               long_options, NULL))
+                               long_options, &oi))
              == -1))
        {
          if (argc <= optind)
@@ -3007,7 +3008,7 @@ main (int argc, char **argv)
          break;

        case 'S':
-         specify_sort_size (optarg);
+         specify_sort_size (optarg, OPT_STR (oi, c, long_options));
          break;

        case 't':
diff --git a/src/system.h b/src/system.h
index 79b1d47..24df764 100644
--- a/src/system.h
+++ b/src/system.h
@@ -593,3 +593,25 @@ emit_bug_reporting_address (void)
      bugs (typically your translation team's web or email address).  */
   printf (_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
 }
+
+#define OPT_STR(opt_idx, c, long_options) \
+  opt_str (opt_idx, c, (long_options)[(opt_idx) < 0 ? 0 : (opt_idx)].name)
+
+static inline char *
+opt_str (int opt_idx, int c, char const *long_name)
+{
+  static char s[80];
+  s[0] = '-';
+  if (opt_idx < 0)
+    {
+      s[1] = c;
+      s[2] = '\0';
+    }
+  else
+    {
+      s[1] = '-';
+      strncpy (s + 2, long_name, sizeof s - 2);
+      s[sizeof s - 1] = '\0';
+    }
+  return s;
+}