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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 9, 2021, 08:18 UTC
Message-ID: <87zgsmdu6d.fsf@evledraar.gmail.com>
In-Reply-To: <YTgfuAKJv1rXXm7i@nand.local>

On Tue, Sep 07 2021, Taylor Blau wrote:

Show 46 quoted lines

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

As a comment on this series I'd find 4/4 squashed into 3/4 easier to read, when I did a "git blame" and found d356d5debe5 I discovered the test right away, if and when this gets merged someone might do the same, but not find the test as easily (they'd probably then grep the config variable name and find it eventually...).

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.

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

It's never going to matter at all for performance, I should have been clearer with my comments. I meant them purely as a "this code is hard to follow" comment.

I.e. since we read the config twice, and in both commit-graph.c and multi-pack-index.c munge and write to the "opts" struct on parse_options(), you'll need to follow logic like:

    1. Read config in cmd_X(), might set variable xyz
    2. Do parse_options() in cmd_X(), might set variable xyz also
    3. Now in cmd_X_subcmd(), read config, might set variable xyz
    4. Do parse_options() in cmd_X(), migh set variable xyz also

Of course in this case the relevant opts.flags only matters for the "write" subcommand, so on more careful reading we don't need to worry about the value flip-flopping between config defaults and getopts settings, but just in terms of establishing a pattern we'll be following in the subcommand built-ins I think this is setting us up for more complexity than is needed.

As far as being consistent with existing behavior, in git-worktree, git-stash which are both similarly structured subcommands we follow the pattern of calling git_config() once, it seems to me better to follow that pattern than the one in d356d5debe5 if the config can be unambiguously parsed in one pass.