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

From: Jeff King <peff@peff.net>
Date: Oct 5, 2018, 21:47 UTC
Message-ID: <20181005214721.GA12992@sigill.intra.peff.net>
In-Reply-To: <a5a5b0c5-cc97-dc50-56dd-25ec64c03e48@web.de>

On Fri, Oct 05, 2018 at 11:22:31PM +0200, René Scharfe wrote:

Show 21 quoted lines

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