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

From: Jeff King <peff@peff.net>
Date: Oct 4, 2018, 22:14 UTC
Message-ID: <20181004221406.GB28287@sigill.intra.peff.net>
In-Reply-To: <20181004213813.158688-1-jonathantanmy@google.com>

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

Show 17 quoted lines

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