Re: [PATCH] connected: do not walk alternate refs in deepening fetches

[Justin Tobler <jltobler@gmail.com>]

On 26/10/14 12:35PM, Igor Wiedler wrote:
> The connectivity check spawned by check_connected() uses
> 
>   git rev-list --objects --stdin --not --all --quiet --alternate-refs
> 
> where 39b44ba771 (check_everything_connected: assume alternate ref
> tips are valid, 2019-07-01) added "--alternate-refs" so that the walk
> from the fetched tips stops at the refs of any alternate object store,
> on the assumption that the alternate is already fully connected.

Ok, when performing a connectivity check, we assume that an alternate
repository is already in a valid state. By excluding alternate refs from
the git-rev-list(1), some time can potentially be saved.

> That only works because "--alternate-refs" comes after "--not".
> cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
> 2018-07-02) drops "--not --all" for deepening fetches, since those
> need to verify the whole chain up to the new shallow boundary rather
> than stopping at refs we already had. For such fetches we therefore
> run
> 
>   git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs
> 
> with the alternate's refs as positive revisions, and rev-list
> enumerates every object reachable from them. Unless one of those refs
> happens to lie inside the newly fetched shallow range, where the
> grafts cut the walk short, that is the alternate's entire history.

Ok, so for deepening fetches, the `--not --all` is intentially removed
to allow the newly deepened commits to be verified. A consequence of
this though was that the `--alternate-refs` changed from being excluded
to included unintentially. Makes sense.

> This is slow for setups that attach a large alternate to a fresh
> repository and then do a shallow fetch of a ref that is a few commits
> ahead of the alternate, which is how CI runners share a clone of a
> big repository between jobs. With a 7M-object alternate (a bare clone
> of gitlab-org/gitlab), "git fetch --depth 1" transfers an empty pack
> in under a second and the connectivity check then takes 227s. Without
> the walk over the alternate's history the same rev-list finishes in
> 0.0s.

For large alternate repositories that contain lots of additional
history, this certainly could result in a significant slow down.

> Move "--alternate-refs" into the non-deepening branch next to "--not
> --all", so that a deepening fetch walks the fetched tips down to the
> new shallow boundary without looking at the alternate's refs at all.
> Negating them instead would stop the walk at the alternate's tips,
> but the reasoning of cf1e7c0770 applies to them as well: an alternate
> that is itself shallow, or a partial clone, has refs without the
> history behind them. fetch-pack sends the alternate's refs as "have"s
> in that case too, so the server omits everything behind them, and a
> check that stops at those refs does not notice that the objects
> between the alternate's boundary and the new one are missing. The walk
> down to the boundary is bounded by the depth, so this does not bring
> back the full history walk, and objects that exist only in the
> alternate are still found through the object store.

Naively I assumed we would still want to negate the alternate refs, but
IIUC for deepening fetches we may fetch objects that a shallow/partial
alternate repo _could_ have, but doesn't. Thus we need to perform the
object walk up to the new boundary. That makes sense to me.

> Add tests to t5618 that check the arguments of the rev-list child via
> GIT_TRACE2_EVENT for a regular and for a deepening fetch. The result
> of the check is the same either way for a complete alternate, so
> there is no behavioural difference to assert on. The difference is
> only in which objects are walked.
> 
> Signed-off-by: Igor Wiedler <iwiedler@gitlab.com>
> ---
>  connected.c               |  2 +-
>  t/t5618-alternate-refs.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/connected.c b/connected.c
> index 929b9bd28d..d8bfb3dbf2 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  			strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
>  				     opt->exclude_hidden_refs_section);
>  		strvec_push(&rev_list.args, "--all");
> +		strvec_push(&rev_list.args, "--alternate-refs");
>  	}
>  	strvec_push(&rev_list.args, "--quiet");
> -	strvec_push(&rev_list.args, "--alternate-refs");

Ok, now the --alternate-refs option is only used in non-deepening
fetches. Looks good.

>  	if (opt->progress)
>  		strvec_pushf(&rev_list.args, "--progress=%s",
>  			     _("Checking connectivity"));
> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> index 2fb6d549d3..237eaba85c 100755
> --- a/t/t5618-alternate-refs.sh
> +++ b/t/t5618-alternate-refs.sh
> @@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'connectivity check of a fetch excludes alternate refs' '
> +	GIT_TRACE2_EVENT="$PWD/fetch.trace" git fetch origin &&
> +	test_subcommand_flex git rev-list --objects --stdin --not --all \
> +		--alternate-refs --quiet <fetch.trace
> +'
> +
> +test_expect_success 'connectivity check of a deepening fetch ignores alternate refs' '
> +	GIT_TRACE2_EVENT="$PWD/deepen.trace" git fetch --depth=1 origin one &&
> +	test_subcommand_flex git rev-list --objects --stdin --quiet <deepen.trace &&
> +	test_subcommand_flex ! git rev-list --objects --stdin --alternate-refs \
> +		<deepen.trace
> +'
> +
>  test_done

Ok, the added tests here are simply validing that the spawned
git-rev-list(1) command is using the options we now expect.

Overall, this patch looks good to me.

-Justin