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

[Derrick Stolee <stolee@gmail.com>]

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?

Thanks,
-Stolee