Re: [PATCH 6/6] fetch: avoid second connectivity check if we already have all objects

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

On Fri, Aug 20, 2021 at 10:47:11AM -0400, Derrick Stolee wrote:
> On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:
> > When fetching refs, we are doing two connectivity checks:
> > 
> >     - The first one in `fetch_refs()` is done such that we can
> >       short-circuit the case where we already have all objects
> >       referenced by the updated set of refs.
> > 
> >     - The second one in `store_updated_refs()` does a sanity check that
> >       we have all objects after we have fetched the packfile.
> > 
> > We always execute both connectivity checks, but this is wasteful in case
> > the first connectivity check already notices that we have all objects
> > locally available.
> > 
> > Refactor the code to do both connectivity checks in `fetch_refs()`,
> > which allows us to easily skip the second connectivity check if we
> > already have all objects available. This refactoring is safe to do given
> > that we always call `fetch_refs()` followed by `consume_refs()`, which
> > is the only caller of `store_updated_refs()`.
> 
> Should we try to make it more clear that fetch_refs() must be followed
> by consume_refs() via a comment above the fetch_refs(), or possibly even
> its call sites?

I wasn't quite happy with this outcome, either. How about we instead
merge both functions into `fetch_and_consume_refs()`? Both are quite
short, and that makes sure we always call them together to make the
requirement explicit.

I'll add another patch to do this refactoring.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]