Jim Meyering wrote: > Eric Blake wrote: >> On 12/09/2011 12:22 PM, Jim Meyering wrote: >>> jida...@jidanni.org 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 <meyer...@redhat.com> 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"))