On Fri, Dec 10, 2021 at 01:36:02PM -0500, Taylor Blau wrote: > On Wed, Dec 08, 2021 at 02:55:13PM -0500, Jeff King wrote: > > On Wed, Dec 08, 2021 at 02:30:17PM -0500, Derrick Stolee wrote: > > > > > > Taylor Blau (2): > > > > t5326: demonstrate bitmap corruption after permutation > > > > midx.c: make changing the preferred pack safe > > > > > > Just chiming in to say that I reviewed an earlier version of this series > > > and the version in this submission looks good to me. > > > > Ditto. ;) > > All three of us missed that this PORD chunk actually contains the > psuedo-pack position for every object in the MIDX. That is OK, but it's > definitely adding more than 4 bytes per pack to the MIDX (in practice, > it's adding 4 bytes per object). > > I'm semi-OK with this direction, since it's tantamount to storing the > .rev file's contents in the MIDX itself. And even though we're not > reading from it, it is doing the thing we need it to which is causing > the MIDX to change its checksum when the object order changes. > > But I'm curious what both of your thoughts are before moving forward. To just add a little bit more detail before I mostly close my computer for the weekend: The key part of this bug is that the MIDX checksum could remain unchanged even when the object order used to write the MIDX bitmap does, and that's what the first patch here is demonstrating. Having PORD contain the same data as the .rev file still accomplishes our original goal of preventing this bug, because it forces the checksum to change when the object order does. But it's definitely more invasive than I had imagined. I had originally imagined that storing the preferred pack's identity alone would be enough to solve this bug. But that isn't quite so, because we break ties among duplicate objects first by prefered-ness, then by their pack's mtime. So that could change too, and it would cause us to break in the same way. At the bare minimum you need an ordering of all of the packs in the MIDX (like I had originally imagined here). At most, we could do something like what is unintentionally written here, which would allow us to get rid of MIDX .rev files entirely. I think doing the former is simpler, and I am not sure if there are practical advantages to the latter. But I'm definitely curious as to what others think would be a good direction to pursue. Thanks, Taylor
