Re: [External] Re: [PATCH] midx: disable replace objects

[鑫邢 <xingxin.xx@bytedance.com>]

> 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