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

Taylor Blau [off-list ref] writes:

+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()'.
+	 */

It's probably not just "No need to", but calling default_config() or
any "more generic" config this late is a wrong pattern, as the one
that was already called in the caller may have been overridden by
the command line options parser, and calling "more generic" config
callback after that would be a no-no.  So I think this should read
"we should never make a fall-back call to ...", instead.

Granted, cmd_multi_pack_index() only uses default_config(), and what
it sets will not be overridden by the command line argument parser
of the multi-pack-index command, so it is not a problem yet, but if
we ever introduce configuration variables that are applicable to
multiple subcommands of multi-pack-index, this distinction may start
to matter.

Unless a Git subcommand cannot work with just a single call to
git_config() with a callback upfront, I actually think it may be
better for them to issue a more pointed git_config_get_*() on the
variables they are interested in, never making a separate call to
git_config().

But that can be left for the future.

On Sun, Sep 12, 2021 at 05:38:36PM -0700, Junio C Hamano wrote:

Taylor Blau [off-list ref] writes:

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()'.
+	 */

It's probably not just "No need to", but calling default_config() or
any "more generic" config this late is a wrong pattern [...]

Makes sense to me, will apply. Thanks!

Thanks,
Taylor