Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX

On Fri, Oct 02, 2026 at 07:25:29PM -0400, Jeff King wrote:

On Wed, Sep 30, 2026 at 11:11:51PM -0500, Taylor Blau wrote:

quoted

diff --git a/builtin/repack.c b/builtin/repack.c
index 88b05e96b5b..27d6668a4ab 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -476,9 +476,11 @@ int cmd_repack(int argc,
 	show_progress = !po_args.quiet && isatty(2);

 	strvec_push(&cmd.args, "--keep-true-parents");
-	for (i = 0; i < keep_pack_list.nr; i++)
-		strvec_pushf(&cmd.args, "--keep-pack=%s",
-			     keep_pack_list.items[i].string);
+	/* Geometric follow walks exclude these packs through stdin instead. */
+	if (!(geometry.split_factor && !midx_must_contain_cruft))
+		for (i = 0; i < keep_pack_list.nr; i++)
+			strvec_pushf(&cmd.args, "--keep-pack=%s",
+				     keep_pack_list.items[i].string);

This conditional makes my head hurt because of the double-negation. By
De Morgan's it is just:

  if (!geometry.split_factor || midx_must_contain_cruft)

Yeah, I struggled a bit when writing it TBH and flip-flopped between the
two. I read the conditional (as proposed in my patch) as:

    "If we aren't doing a geometric repack where the MIDX is allowed to
    omit cruft objects".

But I think the original sin here is midx_must_contain_cruft, which
probably should have been midx_may_exclude_cruft, which defaults to
false as opposed to the former which defaults to true.

It's not quite a double negation, but I agree that it's a little
awkward. TBH I find the rewritten version just as confusing if not more
so.

Thanks,
Taylor