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

[Jeff King <peff@peff.net>]

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

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