Re: [PATCH v3 04/24] multi-pack-index: add 'write' verb

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jul 5, 2018 at 8:53 PM Derrick Stolee <stolee@gmail.com> wrote:
> In anticipation of writing multi-pack-indexes, add a
> 'git multi-pack-index write' subcommand and send the options to a
> write_midx_file() method.

Since the 'write' command is a no-op at this point, perhaps say so in
the commit message. Something like:

    ... add a skeleton 'git multi-pack-index write' subcommand,
    which will be fleshed-out by a later commit.

The bit about sending options to write_midx_file() is superfluous;
it's a mere implementation detail which is clearly seen by reading the
patch.

> Also create a basic test file that tests
> the 'write' subcommand.

Maybe: s/file/script

And, as above, perhaps mention that this is a _skeleton_ test script
so as to avoid confusing readers into thinking that something
significant is happening at this stage.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
> +* Write a MIDX file for the packfiles in an alternate.

In an alternate what?

> +-----------------------------------------------
> +$ git multi-pack-index --object-dir <alt> write
> +-----------------------------------------------
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> @@ -2,9 +2,10 @@
>  static char const * const builtin_multi_pack_index_usage[] = {
> -       N_("git multi-pack-index [--object-dir <dir>]"),
> +       N_("git multi-pack-index [--object-dir <dir>] [write]"),

Is there going to be some default behavior when no verb is provided?
The below implementation seems to suggest that the verb is required,
so this probably ought to be typeset as:

    git multi-pack-index [--object-dir=<dir>] write

Later, when you add more (mutually exclusive) verbs, change the typesetting to:

    git multi-pack-index [--object-dir=<dir>] (write|...|...)

Alternately, just use:

    git multi-pack-index [--object-dir=<dir>] <verb>

> @@ -34,5 +35,12 @@ int cmd_multi_pack_index(int argc, const char **argv,
> +       if (argc == 0)
> +               usage_with_options(builtin_multi_pack_index_usage,
> +                                  builtin_multi_pack_index_options);
> +
> +       if (!strcmp(argv[0], "write"))
> +               return write_midx_file(opts.object_dir);
> +
>         return 0;

This should be throwing an error when an unrecognized verb is provided.

It also should be throwing an error when 'write' is given too many
arguments (which, at this point, appears to be 0).