Taylor Blau <me@xxxxxxxxxxxx> writes: > On Thu, Jan 20, 2022 at 10:15:45AM -0800, Jonathan Tan wrote: > > Taylor Blau <me@xxxxxxxxxxxx> 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. Ah, yes - sorry for the lack of clarity. > 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. Thanks for this explanation of your viewpoint. I still think that it's fine to drop support for reading something that a tagged and released version of Git has created, since it is a potentially wrong cache (and does not contain any new information), but what I can do is to review the rest of this patch set assuming that we want to keep support for the on-disk MIDX .rev files, so I'll do that. This way, if we decide to keep support for these files, I would be happy to see this patch set merged.
