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

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

On Tue, Sep 07 2021, Taylor Blau wrote:

quoted

On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

On Tue, Sep 07 2021, Taylor Blau wrote:

quoted

+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