Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Sep 07 2021, Taylor Blau wrote:
>
> > +static int git_multi_pack_index_write_config(const char *var, const char *value,
> > +					     void *cb)
> > +{
> > +	if (!strcmp(var, "pack.writebitmaphashcache")) {
> > +		if (git_config_bool(var, value))
> > +			opts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;
> > +		else
> > +			opts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;
> > +	}
> > +
> > +	/*
> > +	 * No need to fall-back to 'git_default_config', since this was already
> > +	 * called in 'cmd_multi_pack_index()'.
> > +	 */
> > +	return 0;
> > +}
> > +
> >  static int cmd_multi_pack_index_write(int argc, const char **argv)
> >  {
> >  	struct option *options;
> > @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
> >  		OPT_END(),
> >  	};
> >
> > +	opts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;
> > +
> > +	git_config(git_multi_pack_index_write_config, NULL);
> > +
>
> Since this is a write-only config option it would seem more logical to
> just call git_config() once, and have a git_multip_pack_index_config,
> which then would fall back on git_default_config, so we iterate it once,
> and no need for a comment about the oddity.

Perhaps, but I'm not crazy about each sub-command having to call
git_config() itself when 'write' is the only one that actually has any
values to read.

FWIW, the commit-graph builtin does the same thing as is written here
(calling git_config() twice, once in cmd_commit_graph() with
git_default_config as the callback and again in cmd_commit_graph_write()
with git_commit_graph_write_config as the callback).

So I'm not opposed to cleaning it up, but I'd rather be consistent with
the existing behavior. To be honest, I'm not at all convinced that
reading the config twice is a bottleneck here when compared to
generating a MIDX.

Thanks,
Taylor