From: Jeff King <peff@peff.net>
Date: Fri, 02 Oct 2026 23:25:29 GMT
Subject: Re: [PATCH v2 5/8] repack: follow kept packs when omitting cruft from the MIDX
Message-ID: <20261002232529.GD834759@coredump.intra.peff.net>
In-Reply-To: <51e20444dac1223f0e0485dc5799ed6d592f7614.1790827875.git.me@ttaylorr.com>

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

> 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)

which at least untangles it. The comment makes sense to say "we do not
need to do this in geometric" mode, which matches the first half. But
why does midx_must_contain_cruft trigger it? I guess it is "we do not
need to bother doing the "^"-exclusion later in that mode", but I wonder
if there is any advantage to suppressing it. I don't remember enough of
the details here about why we were treating keep packs specially in the
first place.

> @@ -593,6 +595,29 @@ int cmd_repack(int argc,
>  
>  			fprintf(in, "%c%s\n", marker, basename);
>  		}
> +		if (!midx_must_contain_cruft) {

OK, and this is the flip side of the earlier conditional. We are in
geometric mode if we get here, and we kick in only in non-midx-cruft
mode.

IMHO the De Morgan untangling above makes it more clear, but you could
probably even further with:

  /* explanatory comment here */
  int handle_keep_packs_via_follow = geometry.split_factor && !midx_must_contain_cruft;

And then use that in both spots. That might be overkill, though (and the
name I proposed certainly sucks).

-Peff