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