Re: [PATCH v3 1/9] t5326: demonstrate bitmap corruption after permutation

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jan 20, 2022 at 09:55:41AM -0800, Jonathan Tan wrote:
> As an effort to ensure that Git reads coherent .midx, .rev, and .bitmap
> files, both the .rev and .bitmap files are keyed on the checksum of the
> .midx file. But the issue here is that a .rev and a .bitmap could both
> refer to the same .midx checksum when the .rev and .bitmap files are not
> coherent with respect to each other (e.g. when a Git process has written
> the .rev, but not the .bitmap yet - but this would appear perfectly
> ordinary to another concurrently running Git process, since the .midx
> checksum in the .rev and .bitmap files match).

Kind of, and it's possible that we're saying the same thing here using
different words.

But seeing an out-of-sync .bitmap and .rev is really a symptom of the
underlying problem, which is that the MIDX checksum could (prior to
these patches) be unchanged even when the object order it represents
_does_ change (e.g., in the case of swapping the preferred pack around
like the test here demonstrates).

> This problem is exacerbated by the fact that the .rev has its .midx
> checksum in its filename, whereas the .bitmap has its .midx checksum in
> its file contents. When generating .midx+.rev+.bitmap, it would write
> the .bitmap but not the .rev, since a .rev of the same filename already
> exists.

This isn't quite right: both have the MIDX's checksum in their filename.
For example after running `git repack --write-midx --write-bitmap-index`
on a random repository, I get these MIDX-related files:

    $ find .git/objects/pack -type f -name 'multi-pack-index*'
    .git/objects/pack/multi-pack-index-fd71600b4ceb4caf23a538c7829b9284f2b66d73.rev
    .git/objects/pack/multi-pack-index-fd71600b4ceb4caf23a538c7829b9284f2b66d73.bitmap
    .git/objects/pack/multi-pack-index

Before these patches, it was possible for the MIDX's object order to
change but for its checksum to remain the same. The problem here is that
we rename(2)'d the .bitmap into place, but only link(2)'d the .rev into
place.

So one of the two was updated, and the other was left behind. That does
make them incoherent with respect to each other; but I find it more
useful to think of it as the .rev being out-of-sync with the MIDX.

> The solution is to embed the .rev in the .midx. This means that the
> checksum stored in .bitmap takes into account the contents of what would
> have been in .rev, solving the coherency issue. (There are other
> solutions like storing the name of the preferred pack in .midx, but I
> think that putting the contents of .rev in the .midx is best.)

Right. The problem being that it's possible to change the MIDX's
object order without changing its checksum. Since the .rev file's
contents encodes the pseudo-pack order, embedding it into the MIDX is
sufficient to guarantee that the MIDX's checksum changes whenever the
object order does.

Apologies if that all exactly matched up with your understanding, and I
was just telling you stuff that you already knew. But I figure that this
bug is subtle enough that a little bit of hair-splitting doesn't hurt
;).

Thanks,
Taylor