Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 28, 2019, 12:51 UTC
Message-ID: <601d8561-6e24-559c-6fbb-fa25a7389fa0@gmail.com>
In-Reply-To: <20190628101131.GA22862@sigill.intra.peff.net>

On 6/28/2019 6:11 AM, Jeff King wrote:

Show 9 quoted lines

> When we receive a remote ref update to sha1 "X", we want to check that
> we have all of the objects needed by "X". We can assume that our
> repository is not currently corrupted, and therefore if we have a ref
> pointing at "Y", we have all of its objects. So we can stop our
> traversal from "X" as soon as we hit "Y".
> 
> If we make the same non-corruption assumption about any repositories we
> use to store alternates, then we can also use their ref tips to shorten
> the traversal.

I was confused by this paragraph, because I didn't know about for_each_alternate_ref() and how refs_From_alternate_cb() will strip the "/objects" and append "/refs" to check refs if they exist. All of that logic is in transport.c but used by fetch-pack.c and builtin/receive-pack.c. But now we are adding to revision.c, so the restriction to "this helps data transfer" is getting murkier.

Is this something that should be extracted to the object-store layer? Or is it so tricky to use that we shouldn't make it too easy to fall into a bad pattern?

Show 13 quoted lines

> This is especially useful when cloning with "--reference", as we
> otherwise do not have any local refs to check against, and have to
> traverse the whole history, even though the other side may have sent us
> few or no objects. Here are results for the included perf test (which
> shows off more or less the maximal savings, getting one new commit and
> sharing the whole history):
> 
> Test                        HEAD^             HEAD
> --------------------------------------------------------------------
> [on git.git]
> 5600.3: clone --reference   2.94(2.86+0.08)   0.09(0.08+0.01) -96.9%
> [on linux.git]
> 5600.3: clone --reference   45.74(45.34+0.41)   0.36(0.30+0.08) -99.2%

It's really hard to argue with numbers like these. Kudos!

Show 8 quoted lines

> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/rev-list-options.txt |  8 ++++
>  connected.c                        |  1 +
>  revision.c                         | 30 +++++++++++++++
>  t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
>  t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++

Other than the high-level questions above, the code and tests look good to me.

Thanks, -Stolee