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

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

[Igor Wiedler]

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.

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.

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.

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.

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");
 	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

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.56.0

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

[Patrick Steinhardt]

On Wed, Oct 14, 2026 at 12:35:07PM +0200, Igor Wiedler wrote:

For transparency: I've reviewed this patch internally, so I won't have
much to complain about as we've already been iterating on the patch.

[snip]
> 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.

Yeah. I couldn't really come up with a reasonable test for this, either.
We could corrupt history in the alternate, so if we now were to walk its
history during the connectivity check we'd hit that corruption. But it
feels rather roundabout, same as the proposed test, so I'm not sure that
it's really worth it.

> 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");
>  	if (opt->progress)
>  		strvec_pushf(&rev_list.args, "--progress=%s",
>  			     _("Checking connectivity"));

Yup, makes sense. "--all" and "--alternate-refs" are both supposed to
limit the revision walk with boundaries that we can already reasonably
assume to be fully connected. Marking it as interesting is definitely
the wrong thing in a deepening fetch, so it makes sense to handle it the
exact same as we handle "--all" already.

> 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
> +'

This is a rather roundabout way to test for this, as it exercises what
we're doing rather than the actual observed behaviour. But as I said, I
don't have a better suggestion either, so I think this is okay-ish.

So overall I'm happy with this version. Thanks!

Patrick