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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 9, 2021, 14:47 UTC
Message-ID: <YToeZIIrFH42BRe7@nand.local>
In-Reply-To: <87zgsmdu6d.fsf@evledraar.gmail.com>

On Thu, Sep 09, 2021 at 10:18:10AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 55 quoted lines

>
> On Tue, Sep 07 2021, Taylor Blau wrote:
>
> > 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).
>
> I didn't notice your earlier d356d5debe5 (commit-graph: introduce
> 'commitGraph.maxNewFilters', 2020-09-17). As an aside the test added in
> that commit seems to be broken or not testing that code change at all,
> if I comment out the git_config(git_commit_graph_write_config, &opts)
> it'll pass.

That makes sense; the test that d356d5debe5 added is ensuring that the option `--max-new-filters` overrides any configured value of `commitGraph.maxNewFilters` (so not reading the configuration would be fine there).

> More importantly, the same issue with the commit-graph test seems to be
> the case here, if I comment out the added config reading code it'll
> still pass, it seems to be testing something, but not that the config is
> being read.

I think this also makes sense; since MIDX_WRITE_BITMAP_HASH_CACHE is the default and is set in cmd_multi_pack_index_write(). So it may be worth adding a test to say "make sure the hash-cache _isn't_ written when I:

    git config pack.writeBitmapHashCache &&
    git multi-pack-index write --bitmap

But I don't feel strongly about it (hence I didn't write such a test in the original version which I sent here). If you think it would be helpful in a newer version, I'm happy to add it.

Thanks, Taylor