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

[Jonathan Tan <jonathantanmy@google.com>]

Taylor Blau <me@ttaylorr.com> 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/