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

["René Scharfe" <l.s.r@web.de>]

Am 05.10.2018 um 22:27 schrieb Jeff King:
> On Fri, Oct 05, 2018 at 10:13:34PM +0200, René Scharfe wrote:
> 
>>>> -{
>>>> -	/*
>>>> -	 * Note that this only looks at the ref lists the first time it's
>>>> -	 * called. This works out in filter_refs() because even though it may
>>>> -	 * add to "newlist" between calls, the additions will always be for
>>>> -	 * oids that are already in the set.
>>>> -	 */
>>>
>>> I don't think the subtle point this comment is making goes away. We're
>>> still growing the list in the loop that calls tip_oids_contain() (and
>>> which now calls just oidset_contains). That's OK for the reasons given
>>> here, but I think that would need to be moved down to this code:
>>>
>>>> +	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;
>>>> +		}
>>>> +	}
>>>
>>> I.e., we need to say here why it's OK to summarize newlist in the
>>> oidset, even though we're adding to it later.
>>
>> There is already this comment:
>>
>> 	/* Append unmatched requests to the list */
>>
>> And that's enough in my eyes.  The refs loop at the top splits the list
>> into matched ("the list") and unmatched, and the loop below said comment
>> adds a few more.  I see no subtlety left -- what do I miss?
> 
> It looks like tip_oids is meant as a fast lookup into what's in
> unmatched and newlist. But in the second loop we continue appending to
> newlist. Why is it OK that we do not update tip_oids when we do so?

`tip_oids` contains the object_ids of the all `refs` passed to
filter_refs().  Instead of adding them at the top of the function that
is done only when it has become clear that there are unmatched ones,
as an optimization.  (That optimization was implemented by lazy-loading
in tip_oids_contain() earlier.)  At that point the list has been split
into `newlist` and `unmatched`, so we load from them instead of `refs`.

> 
> I.e., something like this explains it:
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 53914563b5..c0a1b80f4c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -606,6 +606,12 @@ static void filter_refs(struct fetch_pack_args *args,
>  			ref->match_status = REF_MATCHED;
>  			*newtail = copy_ref(ref);
>  			newtail = &(*newtail)->next;
> +			/*
> +			 * No need to update tip_oids with ref->old_oid; we got
> +			 * here because either it was already there, or we are
> +			 * in !strict mode, in which case we do not use
> +			 * tip_oids at all.
> +			 */
>  		} else {
>  			ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
>  		}

This comment puzzles me -- why ask the question it answers?
`tip_oids` has been loaded with all `refs` at that point; adding
more seems odd.

I feel the code needs be simplified further; not sure how, though,
except perhaps by using the `unfound` array added in another reply.

René