Re: [PATCH v3 2/9] midx.c: make changing the preferred pack safe

Taylor Blau [off-list ref] 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/ (local)