Re: [PATCH v3 8/9] midx: read `RIDX` chunk when present

[Jonathan Tan <jonathantanmy@google.com>]

Taylor Blau <me@ttaylorr.com> writes:
> When a MIDX contains the new `RIDX` chunk, ensure that the reverse index
> is read from it instead of the on-disk .rev file. Since we need to
> encode the object order in the MIDX itself for correctness reasons,
> there is no point in storing the same data again outside of the MIDX.
> 
> So, this patch stops writing separate .rev files, and reads it out of
> the MIDX itself. This is possible to do with relatively little new code,
> since the format of the RIDX chunk is identical to the data in the .rev
> file. In other words, we can implement this by pointing the
> `revindex_data` field at the reverse index chunk of the MIDX instead of
> the .rev file without any other changes.

Ah, that's great.

> Note that we have two knobs that are adjusted for the new tests:
> GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls
> whether the MIDX .rev is written at all, and the latter controls whether
> we read the MIDX's RIDX chunk.
> 
> Both are necessary to ensure that the test added at the beginning of
> this series continues to work. This is because we always need to write
> the RIDX chunk in the MIDX in order to change its checksum, but we want
> to make sure reading the existing .rev file still works (since the RIDX
> chunk takes precedence by default).

Could we disable the beginning-of-this-series test when the MIDX .rev is
not written instead? Then, we can test what the user would actually
experience (no reverse index in .midx, reverse index in .rev) instead of
simulating it by skipping over the reverse index section in .midx.

If we can't do that, then I would agree that the approach in this patch
seems like the best approach.