Re: [PATCH] midx: disable replace objects

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 7, 2024, 18:02 UTC
Message-ID: <ZhLfqU9VNUW+2mmV@nand.local>
In-Reply-To: <ZhKqvA1NQwrVfnfE@nand.local>

On Sun, Apr 07, 2024 at 10:16:28AM -0400, Taylor Blau wrote:

Show 7 quoted lines

> , I can still produce the failure that you are seeing here. So I suspect
> that while it's entirely possible that there is a bug in the MIDX/bitmap
> code, that this test is not exercising it.
>
> I think the first step to demonstrate a bug in the MIDX/bitmap machinery
> would be to provide a reproducer that fails only when using a MIDX
> and/or bitmap.

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().

It is tempting to instead do something like:

--- 8< ---

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index c6c8f94cc5..cbc543caad 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -17,6 +17,7 @@
 #include "trace2.h"
 #include "tree.h"
 #include "tree-walk.h"
+#include "replace-object.h"

 struct bitmapped_commit {
 	struct commit *commit;
@@ -223,6 +224,8 @@ static void bitmap_builder_init(struct bitmap_builder *bb,
 	memset(bb, 0, sizeof(*bb));
 	init_bb_data(&bb->data);

+	parsed_object_pool_clear(the_repository->parsed_objects);
+
 	reset_revision_walk();
 	repo_init_revisions(writer->to_pack->repo, &revs, NULL);
 	revs.topo_order = 1;
--- >8 ---

But by then it is too late, because the replace refs have already been
taken into account for parsed objects.

An alternative is to clear the parsed_object_pool before (or after)
calling disable_replace_refs(), but I think that approach that feels
sub-optimal for a couple of reasons:

  - We're wasting time re-parsing objects that we've already seen

  - We're banking on the fact that the MIDX generation does not lookup
    objects with the OBJECT_INFO_LOOKUP_REPLACE flag set, which would
    cause the MIDX to be broken in the same way.

So I think that disabling replace refs at the outset within the
multi-pack-index builtin is the right way to go. 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.

Thanks,
Taylor