Re: [PATCH v4 5/8] repack: add `--filter=<filter-spec>` option

On Tue, Aug 08, 2023 at 10:26:05AM +0200, Christian Couder wrote:

Initializing `po_args`'s `filter_options` makes sense to me, but do we
ever use the cruft_po_args ones? From looking at this patch, I don't
think we do.

Initializing them and then calling list_objects_filter_release() on it
isn't wrong, but I can't tell whether initializing these is necessary
or not in the first place.

+test_expect_success '--filter fails with --write-bitmap-index' '
+	GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 test_must_fail git -C bare.git repack \
+		-a -d --write-bitmap-index --filter=blob:none
+'

I can't remember off-hand why, but I am pretty sure that we usually
write "test_must_fail env ..." before the command-line arguments instead
of setting the environment variable outside of the test_must_fail call.

I *think* that this is the same issue as the single-shot environment
variable assignment before function call thing that we see in some
shells.

Regardless, I wonder if we should be catching this --filter +
--write-bitmap-index thing earlier. The error message I get when
running this is:

    warning: Failed to write bitmap index. Packfile doesn't have full closure (object ac3e272b72bbf89def8657766b855d0656630ed4 is missing)
    fatal: failed to write bitmap index

Which comes from deep within the pack-bitmap-write.c internals
(specifically in a failing call to `find_object_pos()`).

I don't think that's wrong per-se, but I wonder if catching the
combination earlier would allow us to carry on writing the pack even if
the caller erroneously specified that they wanted a bitmap, similar to
how we handle that combination with other options (see the comment in
builtin/pack-objects.c that starts with "'hard reasons not to use
bitmaps [...]'").

I doubt we'd see many naturally occurring instances of users running
"git repack" with both the --filter spec option and
--write-bitmap-index. But, I think that it would come up more often in
bare repositories, where writing reachability bitmaps is the default
state during repacking unless specified otherwise.

I suspect doing something like:

--- 8< ---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 000ebec7ab..d75d122a86 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -4431,7 +4431,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		use_bitmap_index = use_bitmap_index_default;

 	/* "hard" reasons not to use bitmaps; these just won't work at all */
-	if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) || is_repository_shallow(the_repository))
+	if (!use_internal_rev_list ||
+	    (!pack_to_stdout && write_bitmap_index) ||
+	    is_repository_shallow(the_repository) ||
+	    filter_options.choice)
 		use_bitmap_index = 0;

 	if (pack_to_stdout || !rev_list_all)
--- >8 ---

would do the trick (perhaps with a warning(), and the corresponding test
modification, but I think I could go either way on the warning() since
there isn't one there currently.)

I looked through the rest of the tests, and they all looked good to me,
thanks.

Thanks,
Taylor