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

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 9, 2023, 21:40 UTC
Message-ID: <ZNQH6EMKqbuUzEhs@nand.local>
In-Reply-To: <20230808082608.582319-6-christian.couder@gmail.com>

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

Show 6 quoted lines

> @@ -871,6 +925,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>
> +	list_objects_filter_init(&po_args.fturailter_options);
> +	list_objects_filter_init(&cruft_po_args.filter_options);

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