Re: [PATCH v2 2/4] builtin/multi-pack-index.c: don't handle 'progress' separately

[Taylor Blau <me@ttaylorr.com>]

On Mon, Feb 15, 2021 at 10:39:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Feb 15 2021, Taylor Blau wrote:
>
> > @@ -31,15 +30,14 @@ int cmd_multi_pack_index(int argc, const char **argv,
> >
> >  	git_config(git_default_config, NULL);
> >
> > -	opts.progress = isatty(2);
> > +	if (isatty(2))
> > +		opts.flags |= MIDX_PROGRESS;
> >  	argc = parse_options(argc, argv, prefix,
> >  			     builtin_multi_pack_index_options,
> >  			     builtin_multi_pack_index_usage, 0);
> >
> >  	if (!opts.object_dir)
> >  		opts.object_dir = get_object_directory();
> > -	if (opts.progress)
> > -		opts.flags |= MIDX_PROGRESS;
>
>
> Funnily enough we could also just do:
>
>     opts.flags = isatty(2);
>
> Since there's a grand total of one flag it knows about, and
> MIDX_PROGRESS is defined as 1.

:-). I have a handful of branches that add some new flags (including the
original series I sent down-thread), so I'm not sure that I'm in favor
of this (admittedly cute) hack.

> Not the problem of this series really, just a nit: In efbc3aee08d (midx:
> add MIDX_PROGRESS flag, 2019-10-21) we added this flag, and around the
> same time the similar commit-graph code got refactored to have an enum
> of flags in 5af80394521 (commit-graph: collapse parameters into flags,
> 2019-06-12).

Hmm. I don't really have a strong opinion either way. I'd like to avoid
steering too far away from my original goal of multi-pack reverse
indexes, at least for now...

> I prefer the commit-graph way of having a clean boundary between the two
> a bit more, and then just setting a flag based on an OPT_BOOL...

Me too. But if you can part ways with it, it cuts down on the code
duplication (since callers in the sub-commands don't have to set that
bit on the flags themselves).

OTOH, we could keep half of this change and store the flags in the
options structure in addition to the progress field, then set the
appropriate bit in "flags" in cmd_builtin_multi_pack_index().

But I think at that point you're already sharing the flags field
everywhere, so you're just as well off to have something like what's
written in this patch here.

I don't have strong feelings either way.

Thanks,
Taylor