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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 22, 2021 at 07:08:30PM -0500, Jeff King wrote:
> 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'd think that we'd want a single option to control whether or not
reverse indexes are written to disk. I briefly considered (and I believe
that you and I even discussed) having options to control this behavior
per command, but it got out of hand quickly.

And that might have been OK, but I don't think the complexity was even
warranted, because really on-disk reverse indexes are an all-or-nothing
thing. That is: either you want to have revindexes accompanying .packs,
or you don't. IOW, it doesn't matter whether those packs were pushed to
us, or generated during repack, or from another pack-objects or what
have you.

Thanks,
Taylor