On Fri, Oct 05, 2018 at 11:22:31PM +0200, René Scharfe wrote:
> > 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.
If you think that tip_oids is a fast lookup for what's in newlist, then
I think it is a reasonable question to ask whether new additions to
newlist need the same treatment. That was what the comment in the
original lazy-load was answering.
> I feel the code needs be simplified further; not sure how, though,
> except perhaps by using the `unfound` array added in another reply.
I agree it's not the most clear code in the world, but we may be
reaching a point of diminishing returns in discussing it further.
-Peff
