Re: [PATCH v2 4/8] builtin/pack-objects.c: respect 'pack.writeReverseIndex'

From: Jeff King <peff@peff.net>
Date: Jan 23, 2021, 00:08 UTC
Message-ID: <YAto/ndOVfKSkizS@coredump.intra.peff.net>
In-Reply-To: <YAtmb7wXQse76kJi@coredump.intra.peff.net>

On Fri, Jan 22, 2021 at 06:57:35PM -0500, Jeff King wrote:

Show 14 quoted lines

> > +	if (!strcmp(k, "pack.writereverseindex")) {
> > +		if (git_config_bool(k, v))
> > +			pack_idx_opts.flags |= WRITE_REV;
> > +		else
> > +			pack_idx_opts.flags &= ~WRITE_REV;
> > +		return 0;
> > +	}
> 
> This turned out delightfully simple. And I guess this is the "why is
> WRITE_REV" caller I asked about from patch 2. It is
> finish_tmp_packfile() where the magic happens. That unconditionally
> calls write_rev_file(), but it's a noop if WRITE_REV isn't specified.
> 
> Makes sense.

Oh, one subtlety here: this is in pack-objects itself, _not_ in git-repack. This has bit us before with options like repack.writebitmaps, which was originally pack.writebitmaps and introduced all sorts of awkwardness (because pack-objects serves many other purposes besides repacks).

I think this _might_ be OK, because we wouldn't even hit the code-paths that handle this unless we are also writing a .idx (and really, the point is that the two should always be written together or not at all).

So probably it's fine, but I wonder if we should err on the side of conservatism by saying that pack-objects will not change behavior without receiving a new command-line option, and that repack should trigger that option. I dunno. I guess that it makes it weird with respect to index-pack, which wants to see a very low-level option, too. So maybe this is the best way forward.

(Sorry for being non-committal; I'm mostly thinking out loud).

-Peff