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

From: Taylor Blau <ttaylorr@openai.com>
Date: Oct 3, 2026, 00:50 UTC
Message-ID: <asBRayrg2RvzjevI@com-79390>
In-Reply-To: <20261002232529.GD834759@coredump.intra.peff.net>

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

Show 23 quoted lines

> 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