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

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

On Wed, Feb 17, 2021 at 11:05:22AM -0500, Jeff King wrote:

quoted

I think just bumping that:

  if (local && !p->pack_local)
	return 0;

quoted

above the new code would fix it. Or to lay out the logic more fully, the
order of checks should be:

quoted

  - 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