Re: [PATCH v2 6/8] builtin/pack-objects.c: rewrite honor-pack-keep logic

From: Jeff King <peff@peff.net>
Date: Feb 17, 2021, 19:29 UTC
Message-ID: <YC1ugXFk32xHY4k0@coredump.intra.peff.net>
In-Reply-To: <YC1tLzivHqnoV6U7@nand.local>

On Wed, Feb 17, 2021 at 02:23:27PM -0500, Taylor Blau wrote:

Show 24 quoted lines

> On Wed, Feb 17, 2021 at 11:05:22AM -0500, Jeff King wrote:
> > I think just bumping that:
> >
> >   if (local && !p->pack_local)
> > 	return 0;
> 
> > above the new code would fix it. Or to lay out the logic more fully, the
> > order of checks should be:
> 
> >   - does _this_ pack we found the object in disqualify it. If so, we can
> >     cheaply return 0. And that applies to both keep and local rules.
> >
> >   - otherwise, check all packs via has_object_kept_pack(), which is
> >     cheaper than continuing to iterate through all packs by returning
> >     -1.
> >
> >   - once we know definitively about keep-packs, then check any shortcuts
> >     related to local packs (like !have_non_local_packs)
> >
> >   - and then if no shortcuts, we return -1
> 
> I don't understand what you're suggesting. Is the (local &&
> !p->pack_local) a disqualifying condition? Reading the comment, I think
> it is, and so we could do something like:

That's exactly what I'm suggesting. If we have a non-local pack and were given --local, then we can shortcut immediately without caring about kept packs: we know that we do not want the object.

> [...]
> But your "check any shortcuts related to local packs" makes me think
> that we should leave the code as-is.

No, the "shortcuts" there is the opposite:

  if (!local || !have_non_local_packs)
	return 1;

If either of those is true, we can say "definitely include" but only with respect to the --local requirement. So we _can't_ bump that up, but must check it only after we've definitively resolved the keep-pack requirement.

-Peff