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

[Jonathan Tan <jonathantanmy@google.com>]

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

But if we do end up going with the approach in this patch, then this
patch is obviously correct.

I also looked at patch 1 of this patch set and it is obviously correct.
I didn't look at the other patches.