Taylor Blau <me@xxxxxxxxxxxx> writes: > 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. I think that if the user really wanted to manipulate the checksum by using an environment variable for Git tests, they could just as easily do so by using an old version of Git. > 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. I think that feeling the need to avoid the gory details and races in test is an argument for also avoiding it in production (by not supporting external .rev files any more), but I've already explained my opinion on this and will leave it to others to decide. In any case, I still think that this patch set in its current form is worth merging - it is a strict improvement over what we had before (the race condition I'm worried about has been there before this patch set).
