On Tue, Aug 08, 2023 at 10:26:05AM +0200, Christian Couder wrote:
> @@ -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
