From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 20 Aug 2021 14:47:11 GMT
Subject: Re: [PATCH 6/6] fetch: avoid second connectivity check if we already have all objects
Message-ID: <1d5ae688-fa08-6c8c-345f-9b0389e21dd7@gmail.com>
In-Reply-To: <646ac90e62aab4e4aec595d6848b60233bbe8c77.1629452412.git.ps@pks.im>

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