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

>> Determine if the oidset needs to be populated upfront and then do that
>> instead.  This duplicates a loop, but simplifies the existing one by
>> separating concerns between the two.
> 
> [snip]
> 
>> +	if (strict) {
>> +		for (i = 0; i < nr_sought; i++) {
>> +			ref = sought[i];
>> +			if (!is_unmatched_ref(ref))
>> +				continue;
>> +
>> +			add_refs_to_oidset(&tip_oids, unmatched);
>> +			add_refs_to_oidset(&tip_oids, newlist);
>> +			break;
>> +		}
>> +	}
>> +
>>  	/* Append unmatched requests to the list */
>>  	for (i = 0; i < nr_sought; i++) {
>>  		ref = sought[i];
>>  		if (!is_unmatched_ref(ref))
>>  			continue;
>>  
>> -		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.