Package: coreutils;
Reported by: jidanni <at> jidanni.org
Date: Fri, 9 Dec 2011 01:53:02 UTC
Severity: normal
Done: Jim Meyering <jim <at> meyering.net>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: help-debbugs <at> gnu.org (GNU bug Tracking System) To: jidanni <at> jidanni.org Subject: bug#10253: closed (Re: bug#10253: mention +FORMAT in ls time style reminder help blurb) Date: Mon, 12 Dec 2011 11:23:01 +0000
[Message part 1 (text/plain, inline)]
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 10253 <at> debbugs.gnu.org. -- 10253: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10253 GNU Bug Tracking System Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
From: Jim Meyering <jim <at> meyering.net> To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: 10253-done <at> debbugs.gnu.org, Eric Blake <eblake <at> redhat.com>, jidanni <at> jidanni.org Subject: Re: bug#10253: mention +FORMAT in ls time style reminder help blurb Date: Mon, 12 Dec 2011 12:20:51 +0100Jim 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 <meyering <at> 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. * 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
[Message part 3 (message/rfc822, inline)]
From: jidanni <at> jidanni.org To: bug-coreutils <at> gnu.org Subject: mention +FORMAT in ls time style reminder help blurb Date: Fri, 09 Dec 2011 09:51:37 +0800$ 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.