Re: [PATCH v3 3/9] pack-revindex.c: instrument loading on-disk reverse index

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jan 20, 2022 at 10:15:45AM -0800, Jonathan Tan wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > In a subsequent commit, we'll use the MIDX's new 'RIDX' chunk as a
> > source for the reverse index's data. But it will be useful for tests to
> > be able to determine whether the reverse index was loaded from the
> > separate .rev file, or from a chunk within the MIDX.
>
> As for this patch and all subsequent patches, we (at $DAYJOB) discussed
> during our Review Club the idea of not supporting .rev files altogether.
> Firstly, because of this bug, we cannot fully trust .rev files anymore.
> And even if we decided to trust it (or to trust it after some
> verification step that I can't think of), that file would only be useful
> for a short time until a regularly scheduled maintenance step
> regenerates the bitmaps anyway.

I assume that you're talking about MIDX .rev files here, not the usual
packfile ones which are only related in the sense that they follow the
same file format, but are otherwise distinct.

We could drop support for the MIDX .rev files, but I do not think it is
a very good idea. There are tagged versions of Git which have .rev files
out in the wild, so suddenly dropping support for them would mean that
existing repositories would no longer be able to read their multi-pack
bitmaps after upgrading to a version of Git which doesn't include
support for separate .rev files.

So there are some backwards compatibility concerns there, and I think
that that alone means we can't do it.

In order to maintain good test coverage of the soon-to-be deprecated
external MIDX .rev files, this patch series adds some additional testing
knobs to make sure that we're thoroughly exercising both cases.

It's somewhat unfortunate that we are stuck with on-disk .rev files for
MIDXs, since it opened the door for this bug to manifest itself. But I
am not comfortable getting rid of them at this point.

In any case, the .rev file's contents are moving to the MIDX, and by the
end of this series we do not write an external .rev file when creating a
multi-pack bitmap, so you are more than free to not trust any existing
.rev files anymore.

Thanks,
Taylor