bug#10253: mention +FORMAT in ls time style reminder help blurb

[Jim Meyering]

Jim Meyering wrote:
> Eric Blake wrote:
>> On 12/09/2011 12:22 PM, Jim Meyering wrote:
>>> address@hidden wrote:
>>>> $ ls -t1 --time-style=%c -og
>>>> ls: invalid argument `%c' for `time style'
>>>> Valid arguments are:
>>>>   - `full-iso'
>>>>   - `long-iso'
>>>>   - `iso'
>>>>   - `locale'     <-------------you forgot to also mention "+FORMAT"
>>>> Try `ls --help' for more information.
>>>> $ ls -t1 --time-style=+%c -og
>>>
>>> Thanks for the report.
>>> However, that doesn't lend itself well to a clean fix, since +FORMAT
>>> is not a literal string option argument like the others, and currently
>>> that diagnostic is generated automatically based on the list of
>>> literal strings.
>>
>> I agree that listing `+FORMAT' doesn't fit the pattern.  But maybe we
>> can make it obvious that there is a non-literal possibility, as well as
>> also mentioning the posix- prefix already covered by 'ls --help':
>>
>> Valid arguments are:
>>   - `[posix-]full-iso'
>>   - `[posix-]long-iso'
>>   - `[posix-]iso'
>>   - `[posix-]locale'
>>   - `+' followed by formatting directives
>> Try `ls --help' for more information.
>
> Sure, that is possible, and adding the [posix-] prefixes would be a nice 
> bonus.
> The trouble is that the current code is not only nice and compact, but will
> safely and automatically reflect any addition to the list of time styles:
>
>         switch (XARGMATCH ("time style", style,
>                            time_style_args,
>                            time_style_types))
>
> That handles everything, including printing the offending diagnostic
> and exiting.  If you pick it apart in preparation for open-coding, you
> then have to find a way to ensure that the new diagnostic stays in sync
> with list of possible option arguments.
>
> All feasible, but is it worth it, just for an improved diagnostic that
> won't be seen unless you use ls's --time-style=V option with an invalid V ?
>
> I'm leaning away, but if someone comes up with a clean and
> maintainable way to do it, no problem.

I went ahead and started writing, to see just how bad it would be...
This looks worse than it is due to the indentation change.
Below is the much smaller white-space-ignoring diff.
It seems worthwhile after all.

With this patch, ls would print this:

    $ src/ls -l --time-style=%c
    src/ls: invalid argument `%c' for `time style'
    Valid arguments are:  - `[posix-]full-iso'
      - `[posix-]long-iso'
      - `[posix-]iso'
      - `[posix-]locale'
      - `+' followed by a date format string
    Try `src/ls --help' for more information.


>From 4114b1c0e1116e05d50ea3a2377edfb8bb090e78 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 11 Dec 2011 11:59:31 +0100
Subject: [PATCH] ls: give a more useful diagnostic for a bogus --time-style
 arg

* src/ls.c (decode_switches): Replace our use of XARGMATCH
with open-coded version so that we can give a better diagnostic.
Reported by Dan Jacobson in http://bugs.gnu.org/10253
---
 src/ls.c |   72 +++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/src/ls.c b/src/ls.c
index 0d64bab..ac01f3d 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -2039,33 +2039,57 @@ decode_switches (int argc, char **argv)
           long_time_format[1] = p1;
         }
       else
-        switch (XARGMATCH ("time style", style,
-                           time_style_args,
-                           time_style_types))
-          {
-          case full_iso_time_style:
-            long_time_format[0] = long_time_format[1] =
-              "%Y-%m-%d %H:%M:%S.%N %z";
-            break;
+        {
+          ptrdiff_t res = argmatch (style, time_style_args,
+                                    (char const *) time_style_types,
+                                    sizeof (*time_style_types));
+          if (res < 0)
+            {
+              /* This whole block used to be a simple use of XARGMATCH.
+                 but that didn't print the "posix-"-prefixed variants or
+                 the "+"-prefixed format string option upon failure.  */
+              argmatch_invalid ("time style", style, res);
+
+              /* The following is a manual expansion of argmatch_valid,
+                 but with the added "+ ..." description and the [posix-]
+                 prefixes prepended.  Note that this simplification works
+                 only because all four existing time_style_types values
+                 are distinct.  */
+              fprintf (stderr, _("Valid arguments are:"));
+              char const *const *p = time_style_args;
+              while (*p)
+                fprintf (stderr, "  - `[posix-]%s'\n", *p++);
+              fprintf (stderr,
+                       _("  - `+' followed by a date format string\n"));
+              usage (LS_FAILURE);
+            }
+          switch (res)
+            {
+            case full_iso_time_style:
+              long_time_format[0] = long_time_format[1] =
+                "%Y-%m-%d %H:%M:%S.%N %z";
+              break;

-          case long_iso_time_style:
-            long_time_format[0] = long_time_format[1] = "%Y-%m-%d %H:%M";
-            break;
+            case long_iso_time_style:
+              long_time_format[0] = long_time_format[1] = "%Y-%m-%d %H:%M";
+              break;

-          case iso_time_style:
-            long_time_format[0] = "%Y-%m-%d ";
-            long_time_format[1] = "%m-%d %H:%M";
-            break;
+            case iso_time_style:
+              long_time_format[0] = "%Y-%m-%d ";
+              long_time_format[1] = "%m-%d %H:%M";
+              break;
+
+            case locale_time_style:
+              if (hard_locale (LC_TIME))
+                {
+                  int i;
+                  for (i = 0; i < 2; i++)
+                    long_time_format[i] =
+                      dcgettext (NULL, long_time_format[i], LC_TIME);
+                }
+            }
+        }

-          case locale_time_style:
-            if (hard_locale (LC_TIME))
-              {
-                int i;
-                for (i = 0; i < 2; i++)
-                  long_time_format[i] =
-                    dcgettext (NULL, long_time_format[i], LC_TIME);
-              }
-          }
       /* Note we leave %5b etc. alone so user widths/flags are honored.  */
       if (strstr (long_time_format[0], "%b")
           || strstr (long_time_format[1], "%b"))
--
1.7.8.163.g9859a



Ignoring white space changes:

diff --git a/src/ls.c b/src/ls.c
index 0d64bab..ac01f3d 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -2039,9 +2039,31 @@ decode_switches (int argc, char **argv)
           long_time_format[1] = p1;
         }
       else
-        switch (XARGMATCH ("time style", style,
-                           time_style_args,
-                           time_style_types))
+        {
+          ptrdiff_t res = argmatch (style, time_style_args,
+                                    (char const *) time_style_types,
+                                    sizeof (*time_style_types));
+          if (res < 0)
+            {
+              /* This whole block used to be a simple use of XARGMATCH.
+                 but that didn't print the "posix-"-prefixed variants or
+                 the "+"-prefixed format string option upon failure.  */
+              argmatch_invalid ("time style", style, res);
+
+              /* The following is a manual expansion of argmatch_valid,
+                 but with the added "+ ..." description and the [posix-]
+                 prefixes prepended.  Note that this simplification works
+                 only because all four existing time_style_types values
+                 are distinct.  */
+              fprintf (stderr, _("Valid arguments are:"));
+              char const *const *p = time_style_args;
+              while (*p)
+                fprintf (stderr, "  - `[posix-]%s'\n", *p++);
+              fprintf (stderr,
+                       _("  - `+' followed by a date format string\n"));
+              usage (LS_FAILURE);
+            }
+          switch (res)
           {
           case full_iso_time_style:
             long_time_format[0] = long_time_format[1] =
@@ -2066,6 +2088,8 @@ decode_switches (int argc, char **argv)
                     dcgettext (NULL, long_time_format[i], LC_TIME);
               }
           }
+        }
+
       /* Note we leave %5b etc. alone so user widths/flags are honored.  */
       if (strstr (long_time_format[0], "%b")
           || strstr (long_time_format[1], "%b"))