On Mon, Jan 24, 2022 at 11:27:01AM -0800, Jonathan Tan wrote:
> > 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.
I considered it, i.e., by having the two knobs be:
- GIT_TEST_MIDX_WRITE_REV
- GIT_TEST_MIDX_WRITE_RIDX
where the pair (true, false) would correspond to what a corrupt
repository would look like before this series. But I dislike that it
allows the caller to alter the MIDX's checksum by controlling whether or
not the chunk is written.
So it's really looking at the same problem from two sides: do you make
the RIDX chunk disappear by not reading it, or by never writing it in
the first place? And although the latter is more "accurate", it did
allow me to sidestep a lot of gory details like the ones I outlined in
the second patch.
I don't remember everything fully, since some time has passed since I
originally wrote this, but I remember encountering some of the races
where you'd read the old bitmap in the new object order, and other
annoyances.
Thanks,
Taylor
