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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 14, 2026, 12:25 UTC
Message-ID: <as90v189oxrlRdX0@pks.im>
In-Reply-To: <20261014103507.35317-1-iwiedler@gitlab.com>

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]

Show 5 quoted lines

> 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.

Show 15 quoted lines

> 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.

Show 20 quoted lines

> 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