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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 4, 2018, 22:52 UTC
Message-ID: <20181004225205.176593-1-jonathantanmy@google.com>
In-Reply-To: <20181004221406.GB28287@sigill.intra.peff.net>

> 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 tip_oids_contain() as something both peripheral and independent (if the main loop's logic changes, the lazy loading most likely does not need to be changed).

Show 5 quoted lines

> I agree with you that just checking:
> 
>   if (oidset_count() != 0)
> 
> would be fine, too.

OK, we're agreed on this :-)

> 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 would be pretty apparent in the tests.

I am OK with this too, except that (as far as I can tell) the point of this patch set is to replace the internals of oidset, so we no longer have the tablesize check. Unless you meant the khash analog of tablesize? I would be OK if all tablesize references are replaced with the khash analog in the same patch that the oidset internals are replaced.