On Mon, 21 Dec 2015 17:20:34 +0100
Quentin Rameau <quinq_AT_fifth.space> wrote:
Hey Quentin,
> Some tools didn't use argv0 for tool name, or usage() at all.
thanks for your patch, though there are some issues with it I'd like to point out now.
> +#include "arg.h"
There's no need to include arg.h. This is already included in util.h, so let's keep it
this way and not include it.
> diff --git a/chown.c b/chown.c
>
> - eprintf("usage: %s [-h] [-R [-H | -L | -P]] [owner][:[group]] file ...\n", argv0);
> + eprintf("usage: %s [-h] [-R [-H | -L | -P]] owner[:group] file ...\n"
> + " %s [-h] [-R [-H | -L | -P]] :group file ...\n",
> + argv0, argv0);
Don't change the usage as is, unless you also change the manpage meanfully.
I see what you did there, but if at all, make it a separate patch.
> diff --git a/cksum.c b/cksum.c
>
> +static void
> +usage(void)
> +{
> + eprintf("usage: %s [file ...]\n", argv0);
> +}
> +
> int
> main(int argc, char *argv[])
> {
> FILE *fp;
>
> - argv0 = argv[0], argc--, argv++;
> + ARGBEGIN {
> + default:
> + usage();
> + } ARGEND
NO! The rule of thumb is: If a tool doesn't accept any flags, we won't
handle flags and rather look for files called "-f" or something.
> diff --git a/ed.c b/ed.c
Ok.
> diff --git a/find.c b/find.c
Ok.
> diff --git a/grep.c b/grep.c
Ok.
> diff --git a/join.c b/join.c
>
> - eprintf("usage: %s [-1 field] [-2 field] [-o list] [-e string] "
> - "[-a | -v fileno] [-t delim] file1 file2\n", argv0);
> + eprintf("usage: %s [-1 field] [-2 field] [-a fileno | -v fileno] "
> + "[-e string] [-o list] [-t delim] file1 file2\n", argv0);
Look at the manpage. The non-alphabetical order has a reason. If at all,
the usage-strings can only be changed when the manpage is changes as well.
> diff --git a/nl.c b/nl.c
> eprintf("usage: %s [-p] [-b type] [-d delim] [-f type]\n"
> - " [-h type] [-i num] [-l num] [-n format]\n"
> - " [-s sep] [-v num] [-w num] [file]\n", argv0);
> + " %*s [-h type] [-i num] [-l num] [-n format]\n"
> + " %*s [-s sep] [-v num] [-w num] [file]\n",
> + argv0, strlen(argv0), " ", strlen(argv0), " ");
What the fuck? Just keep the 7 spaces, alright? We don't need to become
dynamic here.
> diff --git a/renice.c b/renice.c
Ok.
> diff --git a/seq.c b/seq.c
>
> - eprintf("usage: %s [-f fmt] [-s sep] [-w] [startnum"
> - " [step]] endnum\n", argv0);
> + eprintf("usage: %s [-f fmt] [-s sep] [-w] [startnum [step]] endnum\n",
> + argv0);
Why break the line limit here? Just keep it as-is.
> diff --git a/sort.c b/sort.c
>
> enprintf(2, "usage: %s [-Cbcmnru] [-o outfile] [-t delim] "
> - "[-k def]... [file ...]\n", argv0);
> + "[[-k def] ...] [file ...]\n", argv0);
Please make that a separate patch and change the manpage accordingly.
> diff --git a/tar.c b/tar.c
Ok.
> diff --git a/touch.c b/touch.c
Ok.
> diff --git a/tr.c b/tr.c
>
> static void
> usage(void)
> {
> - eprintf("usage: %s [-cCds] set1 [set2]\n", argv0);
> + eprintf("usage: %s [-Ccds] set1 [set2]\n", argv0);
Same here. Change the manpage.
> diff --git a/xargs.c b/xargs.c
Ok.
Cheers
FRIGN
--
FRIGN <dev_AT_frign.de>
Received on Mon Dec 21 2015 - 17:50:31 CET