| Message ID | 20240806003539.3292562-2-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | make "too many arguments" a bit more useful | expand |
On Mon, Aug 05, 2024 at 05:35:36PM -0700, Junio C Hamano wrote: > diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh > index f7c0783d30..b32e740001 100755 > --- a/t/t1460-refs-migrate.sh > +++ b/t/t1460-refs-migrate.sh > @@ -31,9 +31,10 @@ test_expect_success "superfluous arguments" ' > test_when_finished "rm -rf repo" && > git init repo && > test_must_fail git -C repo refs migrate foo 2>err && > - cat >expect <<-EOF && > - usage: too many arguments > - EOF > + { > + printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" && > + ( git -C repo refs migrate -h || : ) > + } >expect && I always have to wonder how helpful it really is to print the usage information in such a context. I feel that it is too distracting because in many cases, we end up printing dozens of lines of options that drown out the single line of information that the user actually cares for, namely why the command has failed. In this case here it is somewhat manageable, because only 4/5th of the output are unnecessary noise. But the picture changes as commands grow more options over time, making the output less and less usable. So while I think that it is a big improvement to explicitly point out the unknown argument, I think it is step backwards to also print the usage info. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> - cat >expect <<-EOF && >> - usage: too many arguments >> - EOF >> + { >> + printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" && >> + ( git -C repo refs migrate -h || : ) >> + } >expect && > > I always have to wonder how helpful it really is to print the usage > information in such a context. I feel that it is too distracting because > in many cases, we end up printing dozens of lines of options that drown > out the single line of information that the user actually cares for, > namely why the command has failed. > > In this case here it is somewhat manageable, because only 4/5th of the > output are unnecessary noise. But the picture changes as commands grow > more options over time, making the output less and less usable. > > So while I think that it is a big improvement to explicitly point out > the unknown argument, I think it is step backwards to also print the > usage info. Yeah, I somehow was fooled by the original that called a usage() function ;-). "usage:" should signal that the message given is a command line to show the usage, i.e. $ git grep -E -e '[^_]usage\("' builtin/\*.c builtin/merge-index.c: usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])"); builtin/unpack-file.c: usage("git unpack-file <blob>"); and is not a signal that the message explains what it found problematic in this particular usage of the command. builtin/refs.c being relatively young do not honor the tradition, it seems. $ git grep -E -e 'usage\(' builtin/refs.c builtin/refs.c: usage(_("too many arguments")); builtin/refs.c: usage(_("missing --ref-format=<format>")); I think die(_("...")) would be a lot more appropriate in these two places, including the one I touched. Thanks.
diff --git a/builtin/refs.c b/builtin/refs.c index 46dcd150d4..a2aac38ceb 100644 --- a/builtin/refs.c +++ b/builtin/refs.c @@ -30,7 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, migrate_usage, 0); if (argc) - usage(_("too many arguments")); + usage_msg_optf(_("unknown argument: '%s'"), + migrate_usage, options, + argv[0]); if (!format_str) usage(_("missing --ref-format=<format>")); diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh index f7c0783d30..b32e740001 100755 --- a/t/t1460-refs-migrate.sh +++ b/t/t1460-refs-migrate.sh @@ -31,9 +31,10 @@ test_expect_success "superfluous arguments" ' test_when_finished "rm -rf repo" && git init repo && test_must_fail git -C repo refs migrate foo 2>err && - cat >expect <<-EOF && - usage: too many arguments - EOF + { + printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" && + ( git -C repo refs migrate -h || : ) + } >expect && test_cmp expect err '
Running "git refs migrate master main" would fail and say "too many arguments". By reading that message, you cannot tell if you just should have given a single ref and made it "git refs migrate master", or the command refuses to take any arguments. Let's report that 'master' is unknown in such an input, which would be easier for the user to understand. In this particular case, "the command takes no arguments" would also be a good alternative message, but because we plan to reuse the same pattern for commands that take 1 or more messages, and saying "the command takes exactly 1 argument" when "git foo --option bar baz" has to fail (because it does not want to see "baz") can mislead the reader into thinking that "--option" may count as that single argument, so let's be a bit more explicit and mention the first thing we do not want to see on the command line instead. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/refs.c | 4 +++- t/t1460-refs-migrate.sh | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-)