Subject: bug#10253: closed (Re: bug#10253: mention +FORMAT in ls time style reminder help blurb)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 12 Dec 2011 11:23:01 +0000

Your bug report

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

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
10253: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10253
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#10253: mention +FORMAT in ls time style reminder help blurb
From: Jim Meyering <[email-masked]>
To: Paul Eggert <[email-masked]>
Cc: [email-masked], Eric Blake <[email-masked]>, [email-masked]
Date: Mon, 12 Dec 2011 12:20:51 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Jim Meyering wrote:

> Paul Eggert wrote:
>> I like the change, thanks.  A couple of nits:
>>
>> On 12/11/11 03:07, Jim Meyering wrote:
>>> +              fprintf (stderr,
>>> +                       _("  - `+' followed by a date format string\n"));
>>
>> I suggest supplying an example and quoting "date" so that it's clearer
>> that it's talking about the `date' command.  Something like this, perhaps?
>>
>>    _("  - +FORMAT (e.g., +%H:%M) for a `date'-style format\n")
>
> Thanks.  That is better.
>
>>> +                fprintf (stderr, "  - `[posix-]%s'\n", *p++);
>>
>> I suggest removing the ` and ' since they are locale-dependent
>> and aren't needed here (plus, that works better with the above
>> suggestion....).
>
> Good point.  Besides, I'd say that using quotes around syntax including
> the likes of `[posix-]...' is misleading in that it might encourage
> someone to use the []'s.
>
>>> +              fprintf (stderr, _("Valid arguments are:"));
>>
>> Isn't the usual style to use fputs when there's no directive
>> in the format?  There's one other example of this.
>
> Yes, that is my preference, too.
> Thanks for pointing it out.
>
> I copied both that format-less fprintf and the `' mark-up from argmatch.c.
> The fix there was easy: just use quote (...), since argmatch.c already
> includes quote.h, so I've just fixed that in gnulib.
>
> Here's a new version of the patch:
> [slightly risky for translators and fuzzy string matchers:
>  now there are two very similar strings:
>
>   "Valid arguments are:\n" (here in ls.c)
>   "Valid arguments are:"   (in gnulib's argmatch.c)
>
>  It'd be easy rework argmatch.c to include the \n.
>  ]
>
> Now it prints this:
>
>     $ src/ls -l --time-style=x
>     src/ls: invalid argument `x' for `time style'
>     Valid arguments are:
>       - [posix-]full-iso
>       - [posix-]long-iso
>       - [posix-]iso
>       - [posix-]locale
>       - +FORMAT (e.g., +%H:%M) for a `date'-style format
>     Try `src/ls --help' for more information.

I wrote a test, amended the preceding to include it
and pushed this result:

>From a3fee8b6afdbb70317d2124d5a3bb0d2887ab31b Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
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.
* tests/ls/time-style-diag: New file.
* tests/Makefile.am (TESTS): Add it.
Reported by Dan Jacobson in http://bugs.gnu.org/10253
with suggestions from Eric Blake and Paul Eggert.
---
 gnulib                   |    2 +-
 src/ls.c                 |   72 ++++++++++++++++++++++++++++++---------------
 tests/Makefile.am        |    1 +
 tests/ls/time-style-diag |   39 +++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 25 deletions(-)
 create mode 100755 tests/ls/time-style-diag

diff --git a/gnulib b/gnulib
index a5f6df2..f5c2e2a 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit a5f6df2b1f3f0fdc73635de3ad285d21703dab18
+Subproject commit f5c2e2ac7d4ca2f6ba15e56a245f348899360a00
diff --git a/src/ls.c b/src/ls.c
index 0d64bab..672237a 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.  */
+              fputs (_("Valid arguments are:\n"), stderr);
+              char const *const *p = time_style_args;
+              while (*p)
+                fprintf (stderr, "  - [posix-]%s\n", *p++);
+              fputs (_("  - +FORMAT (e.g., +%H:%M) for a `date'-style"
+                       " format\n"), stderr);
+              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"))
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 48c33cb..23cb70f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -441,6 +441,7 @@ TESTS =						\
   ls/stat-free-symlinks				\
   ls/stat-vs-dirent				\
   ls/symlink-slash				\
+  ls/time-style-diag				\
   ls/x-option					\
   mkdir/p-1					\
   mkdir/p-2					\
diff --git a/tests/ls/time-style-diag b/tests/ls/time-style-diag
new file mode 100755
index 0000000..d756cfe
--- /dev/null
+++ b/tests/ls/time-style-diag
@@ -0,0 +1,39 @@
+#!/bin/sh
+# Ensure that an invalid --time-style=ARG is diagnosed the way we want.
+
+# Copyright (C) 2011 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
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ ls
+
+ls -l --time-style=XX > out 2> err
+test $? = 2 || fail=1
+
+cat <<\EOF > exp || fail=1
+ls: invalid argument `XX' for `time style'
+Valid arguments are:
+  - [posix-]full-iso
+  - [posix-]long-iso
+  - [posix-]iso
+  - [posix-]locale
+  - +FORMAT (e.g., +%H:%M) for a `date'-style format
+Try `ls --help' for more information.
+EOF
+
+compare exp err || fail=1
+compare /dev/null out || fail=1
+
+Exit $fail
--
1.7.8.163.g9859a

Subject: mention +FORMAT in ls time style reminder help blurb
From: [email-masked]
To: [email-masked]
Date: Fri, 09 Dec 2011 09:51:37 +0800
List-Id: <debbugs-submit.debbugs.gnu.org>

$ 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