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

[Jonathan Tan <jonathantanmy@google.com>]

Taylor Blau <me@ttaylorr.com> writes:
> 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).

Yeah - I think the way you're describing it is that the concept of
preferred packfile is inherent to the .midx, whereas I just think of it
as an input that helps the algorithm choose between the many .midx
possible given a set of packfiles. Maybe these are just two different
ways of looking at the same thing.

> > 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

Ah, thanks for the clarification. I saw that bitmaps were overridden
correctly and saw BITMAP_OPT_HASH_CACHE in the bitmap-format
documentation, and made an incorrect assumption.

> 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 for all the clarification, especially about the bitmap.