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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 2, 2021, 19:02 UTC
Message-ID: <YD6Ly6EIcz4PT6uB@nand.local>
In-Reply-To: <20210302040625.4035284-1-jonathantanmy@google.com>

On Mon, Mar 01, 2021 at 08:06:25PM -0800, Jonathan Tan wrote:

Show 10 quoted lines

> > +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.

Yeah, I can't say that I disagree with you. Of course, having the #define's outside of the declaration makes the whole thing a little more verbose, which isn't a huge deal.

But I was mirroring what Ævar was doing in the sub-thread he started at:

    https://public-inbox.org/git/20210215184118.11306-1-avarab@gmail.com/

Unless you feel strongly, I think that what we have isn't so bad here.

Show 14 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).

Right.

Thanks, Taylor