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

2018-10-05 Thread Jeff King
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, > >

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

2018-10-05 Thread René Scharfe
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

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

2018-10-05 Thread 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

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

2018-10-05 Thread René Scharfe
Am 05.10.2018 um 00:11 schrieb René Scharfe: > Am 04.10.2018 um 23:38 schrieb Jonathan Tan: >> 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. > > Right, the two

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

2018-10-05 Thread René Scharfe
Am 05.10.2018 um 00:07 schrieb Jeff King: > On Thu, Oct 04, 2018 at 05:09:39PM +0200, René Scharfe wrote: > >> tip_oids_contain() lazily loads refs into an oidset at its first call. >> It abuses the internal (sub)member .map.tablesize of that oidset to >> check if it has done that already. >> >>

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

2018-10-04 Thread Jeff King
On Thu, Oct 04, 2018 at 03:52:05PM -0700, Jonathan Tan wrote: > > Or I am even OK with leaving the existing tablesize > > check. It is a little intimate with the implementation details, but I > > suspect that if oidset were to change (e.g., to initialize the buckets > > immediately), the problem

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

2018-10-04 Thread Jonathan Tan
> Yes, I agree with you that the loops are still entwined. They're at > least now in a single function, though, which IMHO is a slight > improvement. Hmm...originally, the main functionality was in a single loop in a single function. But I say that because I consider the lazy loading in

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

2018-10-04 Thread Jeff King
On Thu, Oct 04, 2018 at 02:38:13PM -0700, Jonathan Tan wrote: > > - if ((allow_unadvertised_object_request & > > -(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)) || > > - tip_oids_contain(_oids, unmatched, newlist, > > ->old_oid)) {

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

2018-10-04 Thread René Scharfe
Am 04.10.2018 um 23:38 schrieb Jonathan Tan: >> 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

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

2018-10-04 Thread Jeff King
On Thu, Oct 04, 2018 at 05:09:39PM +0200, René Scharfe wrote: > tip_oids_contain() lazily loads refs into an oidset at its first call. > It abuses the internal (sub)member .map.tablesize of that oidset to > check if it has done that already. > > Determine if the oidset needs to be populated

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

2018-10-04 Thread Jonathan Tan
> 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 =

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

2018-10-04 Thread René Scharfe
tip_oids_contain() lazily loads refs into an oidset at its first call. It abuses the internal (sub)member .map.tablesize of that oidset to check if it has done that already. Determine if the oidset needs to be populated upfront and then do that instead. This duplicates a loop, but simplifies the