| Message ID | f117e442c3460661dc88beba6f1853d6c388b0fd.1615482270.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | midx: implement a multi-pack reverse index | expand |
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
On Mon, Mar 29, 2021 at 07:42:18AM -0400, Jeff King wrote: > 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(...); Thanks, that's a helpful suggestion (and pretty easy to change, too). Thanks, Taylor
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index b5678cc2bb..243b6ccc7c 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -153,8 +153,7 @@ int cmd_multi_pack_index(int argc, const char **argv, opts.object_dir = get_object_directory(); if (argc == 0) - usage_with_options(builtin_multi_pack_index_usage, - builtin_multi_pack_index_options); + goto usage; if (!strcmp(argv[0], "repack")) return cmd_multi_pack_index_repack(argc, argv); @@ -165,5 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv, else if (!strcmp(argv[0], "expire")) return cmd_multi_pack_index_expire(argc, argv); else - die(_("unrecognized subcommand: %s"), argv[0]); +usage: + usage_with_options(builtin_multi_pack_index_usage, + builtin_multi_pack_index_options); }
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/multi-pack-index.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)