On Mon, Aug 14, 2023 at 05:51:05PM -0700, Junio C Hamano wrote: > 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/
