threads / patch / 66531

patchconnected: do not walk alternate refs in deepening fetches

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

## tl;dr

A mistake dating to 2018 makes the post-fetch check traverse all of an alternate's history, which took 227 seconds with a 7M-object alternate. Read the story.

replies: 20people: 5as markdown or json

Igor Wiedler· Oct 14, 2026, 10:35 UTC · lore
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(-)
Show changes to 2 files +14 −1

connected.c, t/t5618-alternate-refs.sh

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
Patrick Steinhardt· Oct 14, 2026, 12:25 UTC · re: Igor Wiedler · lore

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

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
Junio C Hamano· Oct 14, 2026, 16:55 UTC · re: Igor Wiedler · lore

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

Igor Wiedler <iwiedler@gitlab.com> writes:
Show 17 quoted lines
> 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.

Show 7 quoted lines
> 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(-)
Show changes to diff +3 −1
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);
Jeff King· Oct 15, 2026, 06:41 UTC · re: Junio C Hamano · lore

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

On Wed, Oct 14, 2026 at 09:55:49AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> > 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. ;)

Show 10 quoted lines
> 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).

Show 10 quoted lines
> 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
Justin Tobler· Oct 14, 2026, 17:02 UTC · re: Igor Wiedler · lore

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

On 26/10/14 12:35PM, Igor Wiedler wrote:
Show 8 quoted lines
> 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.

Show 13 quoted lines
> 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.

Show 8 quoted lines
> 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.

Show 13 quoted lines
> 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.

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

Show 25 quoted lines
>  	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
Igor Wiedler· Oct 14, 2026, 18:11 UTC · re: Igor Wiedler · lore

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

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
Junio C Hamano· Oct 14, 2026, 19:23 UTC · re: Igor Wiedler · lore

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

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.
Igor Wiedler· Oct 19, 2026, 11:09 UTC · re: Igor Wiedler · lore

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

In a deepening fetch the connectivity check passes "--alternate-refs" as a positive revision and walks the alternate's whole history. With a 7M-object alternate that takes 227s after a fetch of an empty pack.

Changes since v2:
 * The shallow alternate test wrote $TRASH_DIRECTORY into
   objects/info/alternates, which git on Windows cannot resolve. It now
   uses Peff's version with $(pwd) and relative paths.
 * The trace tests use $(pwd) instead of $PWD.
No code changes.

v2: https://lore.kernel.org/git/20261014181134.56220-1-iwiedler@gitlab.com/ 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
Junio C Hamano· Oct 19, 2026, 23:12 UTC · re: Igor Wiedler · lore

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

Igor Wiedler <iwiedler@gitlab.com> writes:
Show 20 quoted lines
> In a deepening fetch the connectivity check passes "--alternate-refs"
> as a positive revision and walks the alternate's whole history. With a
> 7M-object alternate that takes 227s after a fetch of an empty pack.
>
> Changes since v2:
>
>  * The shallow alternate test wrote $TRASH_DIRECTORY into
>    objects/info/alternates, which git on Windows cannot resolve. It now
>    uses Peff's version with $(pwd) and relative paths.
>
>  * The trace tests use $(pwd) instead of $PWD.
>
> No code changes.
>
> v2: https://lore.kernel.org/git/20261014181134.56220-1-iwiedler@gitlab.com/
> 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

Your patches are not sent as replies to the cover letter of the same round, which is not how we expect the threads to be structured. Please study how others' patch threads look like.

Thanks.
Igor Wiedler· Oct 20, 2026, 07:55 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 19, 2026 at 04:12:49PM -0700, Junio C Hamano wrote:
> Your patches are not sent as replies to the cover letter of the same
> round, which is not how we expect the threads to be structured.
> Please study how others' patch threads look like.

Ack. Thanks for the pointer (and the patience), will fix this on the next round.

Patrick Steinhardt· Oct 23, 2026, 10:44 UTC · re: Igor Wiedler · lore

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

On Tue, Oct 20, 2026 at 09:55:36AM +0200, Igor Wiedler wrote:
Show 7 quoted lines
> On Mon, Oct 19, 2026 at 04:12:49PM -0700, Junio C Hamano wrote:
> > Your patches are not sent as replies to the cover letter of the same
> > round, which is not how we expect the threads to be structured.
> > Please study how others' patch threads look like.
> 
> Ack. Thanks for the pointer (and the patience), will fix this on the
> next round.

This version looks good to me, so I guess there's no need for another reroll just to fix the threading. Thanks!

Patrick
Igor Wiedler· Oct 19, 2026, 11:09 UTC · re: Igor Wiedler · lore

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

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(-)
Show changes to 2 files +16 −2

connected.c, t/t5618-alternate-refs.sh

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..3c5fbef463 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
Igor Wiedler· Oct 19, 2026, 11:09 UTC · re: Igor Wiedler · lore

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

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>
Helped-by: Jeff King <peff@peff.net>
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(-)
Show changes to 3 files +28 −3

Documentation/rev-list-options.adoc, odb.c, t/t5618-alternate-refs.sh

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 3c5fbef463..68139a1b3e 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 --no-local . shallow &&
+	test_commit -C shallow shallow-only &&
+	shallow_tip=$(git -C shallow rev-parse HEAD) &&
+	cp .git/objects/info/alternates alternates.bak &&
+	test_when_finished "mv alternates.bak .git/objects/info/alternates" &&
+	echo "$(pwd)/shallow/.git/objects" >>.git/objects/info/alternates &&
+	git cat-file -e $shallow_tip &&
+	git rev-list --remotes=origin >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.56.0
Patrick Steinhardt· Oct 23, 2026, 10:43 UTC · re: Igor Wiedler · lore

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

On Mon, Oct 19, 2026 at 01:09:18PM +0200, Igor Wiedler wrote:
Show 7 quoted lines
> 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".

Right, for all of these three callers it doesn't make any sense to advertise those haves in case the repository is shallow. It's a somewhat funny setup to have shallow alternates, but in case anybody does, this change here would avoid a potentially corrupted repository.

Show 7 quoted lines
> 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>".
Yup.
Show 10 quoted lines
> "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.

It could of course be that somebody manually created such a repository and due to some obscure reason still expects "--alternate-refs" to work. But I feel like this use case is sufficiently unlikely to simply not matter in practice, and the fact that Git refuses creating this setup in the first place is a good indicator that nobody will care about this change.

I feel like the whole concept of alternate refs is quite flawed in the first place. We get as input an object directory and then we hope that its parent directory _may_ be a proper repository.

Other than that this looks good to me, thanks!
Patrick
Igor Wiedler· Oct 14, 2026, 18:11 UTC · re: Igor Wiedler · lore

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

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(-)
Show changes to 2 files +16 −2

connected.c, t/t5618-alternate-refs.sh

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
Igor Wiedler· Oct 14, 2026, 18:11 UTC · re: Igor Wiedler · lore

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

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(-)
Show changes to 3 files +28 −3

Documentation/rev-list-options.adoc, odb.c, t/t5618-alternate-refs.sh

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
Jeff King· Oct 15, 2026, 06:57 UTC · re: Igor Wiedler · lore

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

On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:
Show 7 quoted lines
> 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.

Show 16 quoted lines
> --- 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.

Show 20 quoted lines
> 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
Junio C Hamano· Oct 15, 2026, 17:25 UTC · re: Jeff King · lore

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

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.
;-)
Show 7 quoted lines
> 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.

Jeff King· Oct 16, 2026, 03:14 UTC · re: Igor Wiedler · lore

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

On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:
Show 12 quoted lines
> +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 seemed to fail the windows ci job with cat-file reporting:
  error: unable to normalize alternate object path: /d/a/git/git/t/trash directory.t5618-alternate-refs/shallow/.git/objects

Just a guess, but it might be the Windows vs POSIX path thing? So perhaps something like this would work, but I didn't test it:

Show changes to t/t5618-alternate-refs.sh +6 −8
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 9c3862709a..e5dce85cff 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -76,7 +76,7 @@ test_expect_success 'refs of a shallow alternate are ignored' '
 	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 &&
+	echo "$(pwd)/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 &&

Alternatively, we could actually clone "shallow" with --reference or
--shared, which would let Git write the alternates file itself (but
would mean running rev-list, etc, in a subrepo).

We also mention $TRASH_DIRECTORY in some other spots. I think they are
not a problem, but usually we assume that tests start in the trash
directory and don't change directory (at least not outside of a
subshell). So as a matter of style, we can probably make all of those
relative paths.

Also, a very minor style nit, but we tend to use environment variables
instead of files for holding values. So all together, perhaps (again,
only lightly tested):

diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 9c3862709a..5ac981c5a0 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -71,13 +71,13 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate
 '
 
 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 &&
+	git clone --depth=1 --no-local . shallow &&
+	test_commit -C shallow shallow-only &&
+	shallow_tip=$(git -C shallow rev-parse HEAD) &&
 	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) &&
+	echo "$(pwd)/shallow/.git/objects" >>.git/objects/info/alternates &&
+	git cat-file -e $shallow_tip &&
 	git rev-list --remotes=origin >expect &&
 	git rev-list --alternate-refs >actual &&
 	test_cmp expect actual

-Peff
Igor Wiedler· Oct 19, 2026, 11:02 UTC · re: Jeff King · lore

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

On Thu, Oct 15, 2026 at 11:14:39PM -0400, Jeff King wrote:
Show 6 quoted lines
> This seemed to fail the windows ci job with cat-file reporting:
>
>   error: unable to normalize alternate object path: /d/a/git/git/t/trash directory.t5618-alternate-refs/shallow/.git/objects
>
> Just a guess, but it might be the Windows vs POSIX path thing? So
> perhaps something like this would work (...)
Thanks, I will include this fix in the v3 patchset.
Junio C Hamano· Oct 18, 2026, 01:19 UTC · re: Igor Wiedler · lore

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

Igor Wiedler <iwiedler@gitlab.com> writes:
Show 20 quoted lines
> 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 seems to break CI only on Windows
  https://github.com/git/git/actions/runs/39084117165/job/117547610467#step:5:279
  https://github.com/git/git/actions/runs/39084117165/job/117547743280#step:6:272
I'll eject the topic from 'seen' for now.
Thanks.

← back to recent threads