> I had a some more time to look into this, and I think that your original > fix is correct. > > The issue is, as you suggest, due to the following (from your original > patch): > > > After some investigation we found that all repositories experiencing > > failures contain replace references, which seem to be improperly > > acknowledged by the MIDX bitmap generation logic. > > Indeed, the pack-bitmap-write machinery does not itself call > disable_replace_refs(). So when it generates a reachability bitmap, it > is doing so with the replace refs in mind. You can see that this is > indeed the cause of the problem by looking at the output of an > instrumented version of Git that indicates what bits are being set > during the bitmap generation phase. > > With replace refs (incorrectly) enabled, we get: > > [2, 4, 6, 8, 13, 3, 6, 7, 3, 4, 6, 8] > > and doing the same after calling disable_replace_refs(), we instead get: > > [2, 5, 6, 13, 3, 6, 7, 3, 4, 6, 8] > > Single pack bitmaps are unaffected by this issue because we generate > them from within pack-objects, which does call disable_replace_refs(). Thank you for the comprehensive investigation. I have quoted them in the commit message to provide a clearer explanation of the patch. > In addition to the test fixes I suggested earlier, I would instead demonstrate > the bug by showing a clone (which fails with MIDXs, but doesn't without > MIDXs) like so: > > --- 8< --- > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh > index 5e4cdef6a8..1fb3b0f9d7 100755 > --- a/t/t5326-multi-pack-bitmaps.sh > +++ b/t/t5326-multi-pack-bitmaps.sh > @@ -442,19 +442,16 @@ test_expect_success 'do not follow replace objects for MIDX bitmap' ' > cd repo && > > test_commit A && > - A=$(git rev-parse HEAD) && > test_commit B && > - B=$(git rev-parse HEAD) && > - git checkout --orphan=orphan $A && > + git checkout --orphan=orphan A && > test_commit orphan && > - C=$(git rev-parse HEAD) && > - git rev-list --objects --no-object-names $B |sort >expected && > > - git replace $A $C && > - git repack -ad && > - git multi-pack-index write --bitmap && > - git rev-list --objects --no-object-names --use-bitmap-index $B |sort >actual && > - test_cmp expected actual > + git replace A HEAD && > + git repack -ad --write-midx --write-bitmap-index && > + > + # generating reachability bitmaps with replace refs > + # enabled will result in broken clones > + git clone --no-local --bare . clone.git > ) > ' > --- >8 --- > > With the change in your patch to call disable_replace_refs() in > builtin/multi-pack-index.c, this test passes as expected. With that > change compiled out, we instead get: > > [...] > + git clone --no-local --bare . clone.git > Cloning into bare repository 'clone.git'... > remote: Enumerating objects: 8, done. > remote: Total 8 (delta 0), reused 0 (delta 0), pack-reused 8 (from 1) > Receiving objects: 100% (8/8), done. > fatal: did not receive expected object da5497437fd67ca928333aab79c4b4b55036ea66 > fatal: fetch-pack: invalid index-pack output > error: last command exited with $?=128 > not ok 352 - do not follow replace objects for MIDX bitmap > > as expected. > Applied! The test looks much clearer now, thanks! Xing Xin
