Re: [PATCH v5 0/8] Repack objects into separate packfiles based on a filter

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 15, 2023, 21:43 UTC
Message-ID: <ZNvxg7BVJ+C5XFY4@nand.local>
In-Reply-To: <xmqqv8dhjfgm.fsf@gitster.g>

On Mon, Aug 14, 2023 at 05:51:05PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Thanks.  I do not recall if the previous version with SQUASH??? passed
> the tests or not, but this round seems to be breaking the exact test
> we had trouble with with the previous round:
>
>   https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822
>
> The symptom looks like that "test_must_fail env" test is not
> failing.  Ring a bell?

That does ring a bell for me, but this is a different failure than before, IIRC.

This time we're expecting to fail writing a bitmap during a filtered repack, but we succeed. I was wondering in [1] whether or not we should be catching this bad combination of options more eagerly than relying on the pack-bitmap machinery to notice that we're missing a reachability closure.

I think the reason that this succeeds is that we already have a bitmap, and it likely reuses all of the existing bitmaps before discovering that the pack we wrote doesn't contain all objects. So doing this "fixes" the immediate issue:

--- 8< ---

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 48e92aa6f7..e5134d3451 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -342,6 +342,7 @@ test_expect_success 'repacking with a filter works' '
 '

 test_expect_success '--filter fails with --write-bitmap-index' '
+	rm -f bare.git/objects/pack/*.bitmap &&
 	test_must_fail \
 		env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
 		git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
--- >8 ---

but I wonder if a more complete fix would be something like:

--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index c396029ec9..f021349c4e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -48,6 +48,11 @@ static const char incremental_bitmap_conflict_error[] = N_(
 "--no-write-bitmap-index or disable the pack.writeBitmaps configuration."
 );

+static const char filtered_bitmap_conflict_error[] = N_(
+"Filtered repacks are incompatible with bitmap indexes.  Use\n"
+"--no-write-bitmap-index or disable the pack.writeBitmaps configuration."
+);
+
 struct pack_objects_args {
 	const char *window;
 	const char *window_memory;
@@ -953,7 +958,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)

 	if (write_bitmaps < 0) {
 		if (!write_midx &&
-		    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
+		    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()) &&
+		    !po_args.filter_options.choice)
 			write_bitmaps = 0;
 	} else if (write_bitmaps &&
 		   git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0) &&
@@ -966,6 +972,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
 		die(_(incremental_bitmap_conflict_error));

+	if (write_bitmaps && po_args.filter_options.choice)
+		die(_(filtered_bitmap_conflict_error));
+
 	if (write_bitmaps && po_args.local && has_alt_odb(the_repository)) {
 		/*
 		 * When asked to do a local repack, but we have
--- >8 ---

would be preferable.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/ZNQH6EMKqbuUzEhs@nand.local/