All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] connected: do not walk alternate refs in deepening fetches
@ 2026-10-14 10:35 Igor Wiedler
  2026-10-14 12:25 ` Patrick Steinhardt
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Igor Wiedler @ 2026-10-14 10:35 UTC (permalink / raw)
  To: git; +Cc: ps, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
@ 2026-10-14 12:25 ` Patrick Steinhardt
  2026-10-14 16:55 ` Junio C Hamano
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Patrick Steinhardt @ 2026-10-14 12:25 UTC (permalink / raw)
  To: Igor Wiedler; +Cc: git

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
  2026-10-14 12:25 ` Patrick Steinhardt
@ 2026-10-14 16:55 ` Junio C Hamano
  2026-10-14 17:02 ` Justin Tobler
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2026-10-14 16:55 UTC (permalink / raw)
  To: Igor Wiedler; +Cc: git, ps, Jeff King

Igor Wiedler <iwiedler@gitlab.com> writes:

> 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

Ouch!  Thanks for spotting the 8-year-old mistake.  It shows how
rare the deepening fetches together with the use of alternate refs
in real life?

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

To paraphrase, we cannot trust refs from repositories we are
borrowing the objects from without taking the shallow boundaries of
these repositories into acocunt, so we ignore them.

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

But stepping back a bit, doesn't the same argument apply even when
we are a fully populated repository and making a non-deepening
fetch?  We can trust "--not --all" to make the traversal to stop at
tips of our local refs and can be assured that all history behind
the tips we are fetching will still be connected, but a traversal
that stops at the tip of a shallow repository will break our
repository, and in a worse form than the shallow repository we are
borrowing from, because operations in that repository are guarded by
its shallow boundaries that we usually do not see, but operations in
our repository do not have the same shallow boundaries in effect.

I do agree that for the purpose of "see if this traversal barfs with
missing objects to see if the updated tips have all the objects
behind it", listing the alternate refs behind "--not" is the right
thing to do and it is broken to have "--alternate-refs" without
having "--not" like the current code does.  But I am not convinced
that disabling --alternate-refs only when you are deepening is not
solving the problems of using alternate-refs introduces.  Shouldn't
the fix be more like two (or more) steps, i.e.,

 * [1/2] for check_connected(), use the attached patch instead.

 * [2/2] update odb_for_each_alternate_ref() so that it ignores any
   and all refs from shallow repositories.

Hmm?


 connected.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git i/connected.c w/connected.c
index 7e26976832..f4b60489bf 100644
--- i/connected.c
+++ w/connected.c
@@ -91,8 +91,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	strvec_push(&rev_list.args, "--stdin");
 	if (repo_has_promisor_remote(the_repository))
 		strvec_push(&rev_list.args, "--exclude-promisor-objects");
+
+	/* define where the traversal should stop */
+	strvec_push(&rev_list.args, "--not");
 	if (!opt->is_deepening_fetch) {
-		strvec_push(&rev_list.args, "--not");
 		if (opt->exclude_hidden_refs_section)
 			strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
 				     opt->exclude_hidden_refs_section);

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
  2026-10-14 12:25 ` Patrick Steinhardt
  2026-10-14 16:55 ` Junio C Hamano
@ 2026-10-14 17:02 ` Justin Tobler
  2026-10-14 18:11 ` [PATCH v2 0/2] connected: fix the connectivity check with alternates " Igor Wiedler
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Justin Tobler @ 2026-10-14 17:02 UTC (permalink / raw)
  To: Igor Wiedler; +Cc: git, ps

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 0/2] connected: fix the connectivity check with alternates in deepening fetches
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
                   ` (2 preceding siblings ...)
  2026-10-14 17:02 ` Justin Tobler
@ 2026-10-14 18:11 ` Igor Wiedler
  2026-10-14 18:11 ` [PATCH v2 1/2] connected: negate alternate refs in deepening fetches too Igor Wiedler
  2026-10-14 18:11 ` [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates Igor Wiedler
  5 siblings, 0 replies; 7+ messages in thread
From: Igor Wiedler @ 2026-10-14 18:11 UTC (permalink / raw)
  To: git; +Cc: ps, gitster, jltobler, Igor Wiedler

In a deepening fetch the connectivity check runs rev-list with
"--alternate-refs" as a positive revision, so it walks the alternate's
whole history. With a 7M-object alternate the check takes 227s after
a fetch that received an empty pack.

Changes since v1:

 * The connectivity check negates "--alternate-refs" in both cases,
   as Junio suggested, instead of dropping it for deepening fetches.
   The alternate's refs are a boundary like our own refs. Compared to
   Junio's diff, "--alternate-refs" moved up next to "--all" so that
   the comment above "--not" covers all of the boundary arguments.

 * New second patch: odb_for_each_alternate_ref() ignores the refs of
   shallow and grafted alternates, so that neither the connectivity
   check, nor fetch-pack's "have"s, nor receive-pack's ".have"s treat
   history that the alternate does not have as present. Partial
   clones are not detected, see the commit message.

v1: https://lore.kernel.org/git/20261014103507.35317-1-iwiedler@gitlab.com/

Igor Wiedler (2):
  connected: negate alternate refs in deepening fetches too
  odb: ignore refs of shallow and grafted alternates

 Documentation/rev-list-options.adoc |  7 ++++---
 connected.c                         |  6 ++++--
 odb.c                               | 11 +++++++++++
 t/t5618-alternate-refs.sh           | 25 +++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 5 deletions(-)


base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.56.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] connected: negate alternate refs in deepening fetches too
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
                   ` (3 preceding siblings ...)
  2026-10-14 18:11 ` [PATCH v2 0/2] connected: fix the connectivity check with alternates " Igor Wiedler
@ 2026-10-14 18:11 ` Igor Wiedler
  2026-10-14 18:11 ` [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates Igor Wiedler
  5 siblings, 0 replies; 7+ messages in thread
From: Igor Wiedler @ 2026-10-14 18:11 UTC (permalink / raw)
  To: git; +Cc: ps, gitster, jltobler, 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. With
the alternate's refs negated the same rev-list finishes in 0.0s.

Pass "--not" in both cases, before the branch that adds "--all" for
non-deepening fetches, so that "--alternate-refs" is negated for
deepening fetches as well. Such a fetch still has to reach the new
shallow boundary from the fetched tips without stopping at our own
refs, which is what cf1e7c0770 wanted. It stops at the alternate's
refs, as every other fetch already does. Move "--alternate-refs" up
next to "--all" so that the arguments that bound the walk are in one
place.

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.

Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Igor Wiedler <iwiedler@gitlab.com>
---
 connected.c               |  6 ++++--
 t/t5618-alternate-refs.sh | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/connected.c b/connected.c
index 929b9bd28d..cdf103bba5 100644
--- a/connected.c
+++ b/connected.c
@@ -121,15 +121,17 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	strvec_push(&rev_list.args, "--stdin");
 	if (repo_has_promisor_remote(the_repository))
 		strvec_push(&rev_list.args, "--exclude-promisor-objects");
+
+	/* define where the traversal should stop */
+	strvec_push(&rev_list.args, "--not");
 	if (!opt->is_deepening_fetch) {
-		strvec_push(&rev_list.args, "--not");
 		if (opt->exclude_hidden_refs_section)
 			strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
 				     opt->exclude_hidden_refs_section);
 		strvec_push(&rev_list.args, "--all");
 	}
-	strvec_push(&rev_list.args, "--quiet");
 	strvec_push(&rev_list.args, "--alternate-refs");
+	strvec_push(&rev_list.args, "--quiet");
 	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..4cc39af0f0 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -58,4 +58,16 @@ 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 excludes alternate refs' '
+	GIT_TRACE2_EVENT="$PWD/deepen.trace" git fetch --depth=1 origin one &&
+	test_subcommand_flex git rev-list --objects --stdin --not \
+		--alternate-refs --quiet <deepen.trace
+'
+
 test_done
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates
  2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
                   ` (4 preceding siblings ...)
  2026-10-14 18:11 ` [PATCH v2 1/2] connected: negate alternate refs in deepening fetches too Igor Wiedler
@ 2026-10-14 18:11 ` Igor Wiedler
  5 siblings, 0 replies; 7+ messages in thread
From: Igor Wiedler @ 2026-10-14 18:11 UTC (permalink / raw)
  To: git; +Cc: ps, gitster, jltobler, Igor Wiedler

odb_for_each_alternate_ref() reports the refs of every alternate
object store that is a repository. Its three callers use them as
commits whose history can be assumed to be present: "rev-list
--alternate-refs" (and through it the connectivity check after a
fetch), fetch-pack, which sends them as "have"s so that the server
does not resend what the alternate has, and receive-pack, which
advertises them as ".have".

That assumption does not hold for a shallow repository, whose refs
point at commits with no history behind them below its shallow
boundary, nor for a grafted one. A repository borrowing objects from
such an alternate does not see the alternate's shallow file, so the
history that is missing there is missing here as well. The fetch then
succeeds, the connectivity check stops at the alternate's refs, and
a later "git log" fails with "Could not read <oid>".

"git clone --reference" already rejects a shallow or grafted
reference repository for this reason, but nothing prevents such a
repository from being added via objects/info/alternates or
GIT_ALTERNATE_OBJECT_DIRECTORIES, or from being made shallow after
other repositories started borrowing from it.

Skip the refs of an alternate that has a "shallow" file or an
"info/grafts" file, the same checks clone uses. Its objects are still
available through the object store. Only its refs are no longer used
as a boundary.

Partial clones have the same problem for the objects they lack, but
detecting one requires reading the alternate's config, so this
patch does not handle them.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Igor Wiedler <iwiedler@gitlab.com>
---
 Documentation/rev-list-options.adoc |  7 ++++---
 odb.c                               | 11 +++++++++++
 t/t5618-alternate-refs.sh           | 13 +++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index bd9f345690..982a53c694 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -223,9 +223,10 @@ explicitly.
 	Pretend as if all objects mentioned as ref tips of alternate
 	repositories were listed on the command line. An alternate
 	repository is any repository whose object directory is specified
-	in `objects/info/alternates`.  The set of included objects may
-	be modified by `core.alternateRefsCommand`, etc. See
-	linkgit:git-config[1].
+	in `objects/info/alternates`.  The refs of shallow and grafted
+	alternate repositories are ignored, as the history behind them
+	may be missing.  The set of included objects may be modified by
+	`core.alternateRefsCommand`, etc. See linkgit:git-config[1].
 
 `--single-worktree`::
 	By default, all working trees will be examined by the
diff --git a/odb.c b/odb.c
index 0200e26f21..e6bb7ce6e6 100644
--- a/odb.c
+++ b/odb.c
@@ -458,6 +458,17 @@ static int refs_from_alternate_cb(struct odb_source *alternate,
 		goto out;
 	strbuf_setlen(&path, base_len);
 
+	/* A shallow or grafted repository may lack the history behind its refs. */
+	strbuf_addstr(&path, "/shallow");
+	if (!access(path.buf, F_OK))
+		goto out;
+	strbuf_setlen(&path, base_len);
+
+	strbuf_addstr(&path, "/info/grafts");
+	if (!access(path.buf, F_OK))
+		goto out;
+	strbuf_setlen(&path, base_len);
+
 	read_alternate_refs(alternate->odb->repo, path.buf, cb->fn, cb->payload);
 
 out:
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 4cc39af0f0..9c3862709a 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate
 		--alternate-refs --quiet <deepen.trace
 '
 
+test_expect_success 'refs of a shallow alternate are ignored' '
+	git clone --depth=1 "file://$TRASH_DIRECTORY" "$TRASH_DIRECTORY/shallow" &&
+	test_commit -C "$TRASH_DIRECTORY/shallow" shallow-only &&
+	git -C "$TRASH_DIRECTORY/shallow" rev-parse HEAD >shallow-tip &&
+	cp .git/objects/info/alternates alternates.bak &&
+	test_when_finished "mv alternates.bak .git/objects/info/alternates" &&
+	echo "$TRASH_DIRECTORY/shallow/.git/objects" >>.git/objects/info/alternates &&
+	git cat-file -e $(cat shallow-tip) &&
+	git rev-list --remotes=origin >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-10-14 18:12 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
2026-10-14 12:25 ` Patrick Steinhardt
2026-10-14 16:55 ` Junio C Hamano
2026-10-14 17:02 ` Justin Tobler
2026-10-14 18:11 ` [PATCH v2 0/2] connected: fix the connectivity check with alternates " Igor Wiedler
2026-10-14 18:11 ` [PATCH v2 1/2] connected: negate alternate refs in deepening fetches too Igor Wiedler
2026-10-14 18:11 ` [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates Igor Wiedler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.