From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 06 Jul 2018 03:54:08 GMT
Subject: Re: [PATCH v3 03/24] multi-pack-index: add builtin
Message-ID: <CAPig+cQS-sFHtgMZdaW2tTPSLAo=pytFJ5_zhVvUU1=8Mq74ng@mail.gmail.com>
In-Reply-To: <20180706005321.124643-4-dstolee@microsoft.com>

On Thu, Jul 5, 2018 at 8:53 PM Derrick Stolee <stolee@gmail.com> wrote:
> This new 'git multi-pack-index' builtin will be the plumbing access
> for writing, reading, and checking multi-pack-index files. The
> initial implementation is a no-op.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
> +++ b/Documentation/git-multi-pack-index.txt
> +SYNOPSIS
> +--------
> +'git multi-pack-index' [--object-dir <dir>]

In Git documentation, this is more typically written: [--object-dir=<dir>]

> +OPTIONS
> +-------
> +--object-dir <dir>::

Ditto: --object-dir=<dir>::

> +       Use given directory for the location of Git objects. We check
> +       `<dir>/packs/multi-pack-index` for the current MIDX file, and
> +       `<dir>/packs` for the pack-files to index.
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> @@ -0,0 +1,38 @@
> +static char const * const builtin_multi_pack_index_usage[] = {
> +       N_("git multi-pack-index [--object-dir <dir>]"),

Likewise.

> +int cmd_multi_pack_index(int argc, const char **argv,
> +                        const char *prefix)
> +{
> +       static struct option builtin_multi_pack_index_options[] = {
> +               OPT_FILENAME(0, "object-dir", &opts.object_dir,
> +                 N_("The object directory containing set of packfile and pack-index pairs")),

It's more typical not to capitalize these. Also, keep them short, if
possible, so perhaps drop "The".

> +               OPT_END(),
> +       };
> +
> +       if (argc == 2 && !strcmp(argv[1], "-h"))
> +               usage_with_options(builtin_multi_pack_index_usage,
> +                                  builtin_multi_pack_index_options);

Unless you are planning on adding a short "-h <something>" option
later in the series, then you can do away with this conditional
altogether since the below parse_options() will give you "-h" as help
for free.

> +       git_config(git_default_config, NULL);
> +
> +       argc = parse_options(argc, argv, prefix,
> +                            builtin_multi_pack_index_options,
> +                            builtin_multi_pack_index_usage, 0);