Re: [PATCH v2 04/15] builtin/multi-pack-index.c: split sub-commands

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 2, 2021, 04:06 UTC
Message-ID: <20210302040625.4035284-1-jonathantanmy@google.com>
In-Reply-To: <bc42b56ea2c3ac7ead1cad2a95ac01a7adfbb28f.1614193703.git.me@ttaylorr.com>

Show 14 quoted lines

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index eea498e026..caf0248a98 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -5,17 +5,33 @@
>  #include "midx.h"
>  #include "trace2.h"
>  
> +static char const * const builtin_multi_pack_index_write_usage[] = {
>  #define BUILTIN_MIDX_WRITE_USAGE \
>  	N_("git multi-pack-index [<options>] write")
> +	BUILTIN_MIDX_WRITE_USAGE,
> +	NULL
> +};

I think this way of writing is vulnerable to confusing errors if a missing or extra backslash happens, so I would prefer the #define to be outside the variable declaration.

Show 11 quoted lines

> +static int cmd_multi_pack_index_repack(int argc, const char **argv)
> +{
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_repack_options[] = {
>  		OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
>  		  N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
>  		OPT_END(),
>  	};
>  
> +	options = parse_options_dup(builtin_multi_pack_index_repack_options);
> +	options = add_common_options(options);

I looked for where this was freed, but I guess freeing this struct is not really something we're worried about (which makes sense).

The other patches up to this one look good.