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? 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; }