Re: [PATCH v3 2/5] fetch-pack: load tip_oids eagerly iff needed

[Jeff King <peff@peff.net>]

On Thu, Oct 04, 2018 at 02:38:13PM -0700, Jonathan Tan wrote:

> > -		if ((allow_unadvertised_object_request &
> > -		     (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) ||
> > -		    tip_oids_contain(&tip_oids, unmatched, newlist,
> > -				     &ref->old_oid)) {
> > +		if (!strict || oidset_contains(&tip_oids, &ref->old_oid)) {
> >  			ref->match_status = REF_MATCHED;
> >  			*newtail = copy_ref(ref);
> >  			newtail = &(*newtail)->next;
> 
> I don't think the concerns are truly separated - the first loop quoted
> needs to know that in the second loop, tip_oids is accessed only if
> there is at least one unmatched ref. Would it be better to expose the
> size of the oidset and then use it in place of
> "tip_oids->map.map.tablesize"? Checking for initialization (using
> "tablesize") is conceptually different from checking the size, but in
> this code, both initialization and the first increase in size occur upon
> the first oidset_insert(), so we should still get the same result.

Yes, I agree with you that the loops are still entwined. They're at
least now in a single function, though, which IMHO is a slight
improvement.

I agree with you that just checking:

  if (oidset_count() != 0)

would be fine, too. Or I am even OK with leaving the existing tablesize
check. It is a little intimate with the implementation details, but I
suspect that if oidset were to change (e.g., to initialize the buckets
immediately), the problem would be pretty apparent in the tests.

And in fact, we can test by just changing the conditional in
tip_oid_contains to if(0), which quite clearly fails t5500.60 (along
with others).

So I don't think it's the end of the world to leave it (but I also am
not opposed to the other options discussed).

-Peff