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

[Taylor Blau <ttaylorr@openai.com>]

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