Taylor Blau <me@xxxxxxxxxxxx> writes: > The previous patch demonstrates a bug where a MIDX's auxiliary object > order can become out of sync with a MIDX bitmap. > > This is because of two confounding factors: > > - First, the object order is stored in a file which is named according > to the multi-pack index's checksum, and the MIDX does not store the > object order. This means that the object order can change without > altering the checksum. > > - But the .rev file is moved into place with finalize_object_file(), > which link(2)'s the file into place instead of renaming it. For us, > that means that a modified .rev file will not be moved into place if > MIDX's checksum was unchanged. > > The fix here is two-fold. First, we need to stop linking the file into > place and instead rename it. It's likely we were using > finalize_object_file() instead of a pure rename() because the former > also adjusts shared permissions. But that is unnecessary, because we > already do so in write_rev_file_order(), so rename alone is safe. > > But we also need to make the MIDX's checksum change in some way when the > preferred pack changes without altering the set of packs stored in a > MIDX to prevent a race where the new .rev file is moved into place > before the MIDX is updated. Here, you'd get the opposite effect: reading > old bitmaps with the new object order. I think the main issue is the first confounding factor you listed above: even if we didn't have the other confounding factor, that issue alone is enough to motivate the entire patch set. Likewise, as Junio said [1], I don't think we need to switch to rename() if we make the checksum different, so the fix is one-fold, not two-fold. For what it's worth, I switched back to finalize_object_file() and ran the tests, and they all pass. So I would simplify the commit message to just talk about the checksum issue. (This is definitely not a blocker for merging, though - others might find the additional context helpful.) The code up to this patch (apart from the rename()) looks good. [1] https://lore.kernel.org/git/xmqqtue54iop.fsf@gitster.g/
