On Thu, Jul 12, 2018 at 3:40 PM Derrick Stolee <stolee@xxxxxxxxx> wrote:
> In anticipation of writing multi-pack-indexes, add a skeleton
> 'git multi-pack-index write' subcommand and send the options to a
> write_midx_file() method. Also create a skeleton test script that
> tests the 'write' subcommand.
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> @@ -30,5 +31,17 @@ int cmd_multi_pack_index(int argc, const char **argv,
> + if (argc == 0)
> + goto usage;
> +
> + if (!strcmp(argv[0], "write")) {
> + if (argc > 1)
> + goto usage;
> +
> + return write_midx_file(opts.object_dir);
> + }
> +
> +usage:
> + usage_with_options(builtin_multi_pack_index_usage,
> + builtin_multi_pack_index_options);
> }
I realize that some other commands may work this way, but merely
showing 'usage' is fairly user-hostile. What would be much more
helpful would be to explain to the user what the problem is; for
instance, "no action specified" and "'write' takes no arguments", or
something. That way the user knows exactly what corrective action to
take rather than having to try to guess based upon 'usage'. Showing
'usage' after the actual error message may or may not make sense
(though, I prefer not doing so since noisy 'usage' tends to swamp the
actual error message, making it easy to miss).
I wouldn't want to see a re-roll just for this, especially for such a
long series. Perhaps such a change can be done as a follow-up patch by
someone at some point.
By the way, if you ever need to add options or arguments to "write" or
"verify", you might want to model it after how it's done in
builtin/worktree.c, in which each subcommand is responsible for its
own argument processing, rather than handling subcommand arguments
here in the top-level function.
