From: Jeff King <peff@peff.net>
Date: Mon, 29 Mar 2021 11:42:18 GMT
Subject: Re: [PATCH v3 06/16] builtin/multi-pack-index.c: display usage on unrecognized command
Message-ID: <YGG9GogdjlijGxSF@coredump.intra.peff.net>
In-Reply-To: <f117e442c3460661dc88beba6f1853d6c388b0fd.1615482270.git.me@ttaylorr.com>

On Thu, Mar 11, 2021 at 12:04:57PM -0500, Taylor Blau wrote:

> When given a sub-command that it doesn't understand, 'git
> multi-pack-index' dies with the following message:
> 
>     $ git multi-pack-index bogus
>     fatal: unrecognized subcommand: bogus
> 
> Instead of 'die()'-ing, we can display the usage text, which is much
> more helpful:
> 
>     $ git.compile multi-pack-index bogus
>     usage: git multi-pack-index [<options>] write
>        or: git multi-pack-index [<options>] verify
>        or: git multi-pack-index [<options>] expire
>        or: git multi-pack-index [<options>] repack [--batch-size=<size>]
> 
> 	--object-dir <file>   object directory containing set of packfile and pack-index pairs
> 	--progress            force progress reporting
> 
> While we're at it, clean up some duplication between the "no sub-command"
> and "unrecognized sub-command" conditionals.

I agree that it's much nicer to give the usage. But my preference in
general for cases like this is to _also_ explain what we found wrong
with the options we were given.

E.g., with a bogus option, we say so:

  $ git multi-pack-index --foo
  error: unknown option `foo'
  usage: git multi-pack-index [<options>] write [--preferred-pack=<pack>]
  [etc...]

but with a bogus sub-command, we get just the usage string:

  $ git multi-pack-index foo
  usage: git multi-pack-index [<options>] write [--preferred-pack=<pack>]
  [etc...]

Sometimes it is quote obvious what is wrong, but sometimes typos can be
hard to spot, especially because the usage message is so long.

I.e., I'd suggest changing this:

>  	else
> -		die(_("unrecognized subcommand: %s"), argv[0]);
> +usage:
> +		usage_with_options(builtin_multi_pack_index_usage,
> +				   builtin_multi_pack_index_options);

to:

  error(_("unrecognized subcommand: %s"), argv[0]);
  usage_with_options(...);

-Peff