[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

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

[Junio C Hamano]

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

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

[Justin Tobler]

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

[PATCH v2 0/2] connected: fix the connectivity check with alternates in deepening fetches

[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

[PATCH v2 1/2] connected: negate alternate refs in deepening fetches too

[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

[PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates

[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

Re: [PATCH v2 0/2] connected: fix the connectivity check with alternates in deepening fetches

[Junio C Hamano]

Igor Wiedler <iwiedler@gitlab.com> writes:

> In a deepening fetch ...

The usual "engage in discussion before sending an updated patchset"
applies here.

cf. https://lore.kernel.org/git/xmqqece02no9.fsf@gitster.g/

Thanks.

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

[Jeff King]

On Wed, Oct 14, 2026 at 09:55:49AM -0700, Junio C Hamano wrote:

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

Thanks for cc-ing me. I looked at Igor's v1 patch, and it's exactly the
solution I would have expected. It should have been written that way in
39b44ba771 (check_everything_connected: assume alternate ref tips are
valid, 2019-07-01), because the intent was "treat alternate ref tips
just like we treat our own existing ref tips". So wherever we use
"--all", we should use "--alternate-refs", and since --all was moved
into that non-deepening conditional, --alternate-refs should go there,
too.

BTW, if you are curious why 39b44ba771 made such an obvious mistake, it
is exactly because it predated that move! The original patch was written
in 2014 and used internally at GitHub. I rebased it forward to send to
the list in 2019, but I don't even think I got a textual conflict at all
(because the movement of "--not" and "--all" were a few lines away).

So it was a very subtle semantic conflict between the two patches. But
because the rebase rewrote history to put mine on top, it looks like I
just did it wrong from the start. ;)

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

It took me a few reads to parse this, but I think you are saying: it is
a problem if we are a non-shallow repository trying to use a shallow
alternate for verification?

That sounds likely, though this is the first time I have looked at
cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
2018-07-02) and the surrounding issues. As I understand the issue, it is
really "are the refs we are going to use to verify coming from a shallow
repo which might not have all objects". But I'm not sure how that
interacts with opt->is_deepening_fetch (and why it isn't just checking
if we are in a shallow repo).

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

OK, so this moves the "--not" out so that we always do it. It's ignored
in the case that we are deepening and don't have any alternates, but it
shouldn't hurt anything.

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

Yeah, this seems reasonable. Alternates don't necessarily even have a
full repo, but in that case they wouldn't have any refs, either (IIRC we
do some magic to guess at the containing repo of the alternate).

-Peff

Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates

[Jeff King]

On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:

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

In addition to grafts there are replace refs. I think we generally
insisted on having full connectivity of the original objects, though
(e.g., fsck will ignore replace refs). I thought that was true of
grafts, too, though (except for ones grafted by the shallow mechanism,
which is special).

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

Yeah, I'm sure there are some corner cases there. In theory you can grab
any of the missing partial-clone objects at will, though probably we
have no code to realize that it's the alternate which is partial and
through which the request should be routed. I guess you could argue the
same is true for shallow alternates (i.e., we _could_ get those objects
if we needed them, we just don't have a way to do so).

These are such weird setups that I think punting and saying "just ignore
their alternate refs" is totally fine. The use of the alternate refs is
an optimization, so falling back to the simpler behavior is good.

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

OK. It's a little gross that we have to check these magic paths, but
there's no real alternative here. We do not have a real repository
object or anything like it representing the alternate.

The thread has been talking about check_connected(), but the same
problem applies to advertising alternates for transfer. But your fix
here takes care of both, because it works at the lowest level. Good.

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

This is a direct test of --alternate-refs, which is probably reasonable
and the simplest way of doing it. I think you could also construct a
case where a fetch or push causes the recipient repo to be corrupted (it
claimed to have object X because of the alternate, so the other side
didn't send it, but since _we're_ not shallow, now fsck, etc, fails).
But I don't think there's a particular need to construct that full case
for the tests.

-Peff

Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The thread has been talking about check_connected(), but the same
> problem applies to advertising alternates for transfer. But your fix
> here takes care of both, because it works at the lowest level. Good.

;-)

> This is a direct test of --alternate-refs, which is probably reasonable
> and the simplest way of doing it. I think you could also construct a
> case where a fetch or push causes the recipient repo to be corrupted (it
> claimed to have object X because of the alternate, so the other side
> didn't send it, but since _we're_ not shallow, now fsck, etc, fails).
> But I don't think there's a particular need to construct that full case
> for the tests.

Yeah, thanks for thinking it through thoroughly.  I appreciate it
very much.