Re: [PATCH 2/2] partial-clone: avoid fetching when looking for objects

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 19, 2020, 18:10 UTC
Message-ID: <20200219181042.2192-1-jonathantanmy@google.com>
In-Reply-To: <937a882261d4d4552a144e5f0efad8abd8002ab4.1582129312.git.gitgitgadget@gmail.com>

Show 20 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> When using partial-clone, do_oid_object_info_extended() can trigger a
> fetch for missing objects. This can be extremely expensive when asking
> for a tag or commit, as we are completely removed from the context of
> the missing object and thus supply no "haves" in the request.
> 
> 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a
> global variable that prevented these fetches in favor of a bitflag.
> However, some object existence checks were not updated to use this flag.
> 
> Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in
> addition to OBJECT_INFO_QUICK. The _QUICK option only prevents
> repreparing the pack-file structures. We need to be extremely careful
> about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist
> due to updated refs.
> 
> This resolves a broken test in t5616-partial-clone.sh.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Thanks for catching this.

I wonder if the commit message in this patch could be better worded - the first paragraph seems to say that fetching missing commits and tags are expensive, but that is not the problem here; the problem is that the client lazily fetches refs advertised by the server, thinking that it is lacking them due to partial clone, even when there is no expectation that the client have them (so the commits and tags are not truly missing).

So I would reword the first paragraph as:

  When using partial clone, find_non_local_tags() in builtin/fetch.c
  checks each remote tag to see if its object also exists locally. There
  is no expectation that the object exist locally, but this function
  nevertheless triggers a lazy fetch if the object does not exist. This
  can be extremely expensive when asking for a commit, as we are
  completely removed from the context of the non-existent object and
  thus supply no "haves" in the request.

All this rests on my thinking that "missing" has the connotation (or actual meaning) that we expect the object to be there. If we think that "missing" can also mean that the remote has it but the local doesn't, then you can ignore what I just said :-)

Other than that, both patches look good to me.