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
