{"thread":{"id":"66531","subject":"[PATCH] connected: do not walk alternate refs in deepening fetches","startedAt":"2026-10-14T10:35:07Z","lastAt":"2026-10-23T10:44:54Z","messageCount":21,"participants":["Igor Wiedler","Patrick Steinhardt","Junio C Hamano","Justin Tobler","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"554996","messageId":"20261014103507.35317-1-iwiedler@gitlab.com","threadId":"66531","inReplyTo":null,"subject":"[PATCH] connected: do not walk alternate refs in deepening fetches","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-14T10:35:07Z","receivedAt":"2026-10-14T10:35:07Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"The connectivity check spawned by check_connected() uses\n\n git rev-list --objects --stdin --not --all --quiet --alternate-refs\n\nwhere 39b44ba771 (check_everything_connected: assume alternate ref\ntips are valid, 2019-07-01) added \"--alternate-refs\" so that the walk\nfrom the fetched tips stops at the refs of any alternate object store,\non the assumption that the alternate is already fully connected.\n\nThat only works because \"--alternate-refs\" comes after \"--not\".\ncf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n2018-07-02) drops \"--not --all\" for deepening fetches, since those\nneed to verify the whole chain up to the new shallow boundary rather\nthan stopping at refs we already had. For such fetches we therefore\nrun\n\n git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n\nwith the alternate's refs as positive revisions, and rev-list\nenumerates every object reachable from them. Unless one of those refs\nhappens to lie inside the newly fetched shallow range, where the\ngrafts cut the walk short, that is the alternate's entire history.\n\nThis is slow for setups that attach a large alternate to a fresh\nrepository and then do a shallow fetch of a ref that is a few commits\nahead of the alternate, which is how CI runners share a clone of a\nbig repository between jobs. With a 7M-object alternate (a bare clone\nof gitlab-org/gitlab), \"git fetch --depth 1\" transfers an empty pack\nin under a second and the connectivity check then takes 227s. Without\nthe walk over the alternate's history the same rev-list finishes in\n0.0s.\n\nMove \"--alternate-refs\" into the non-deepening branch next to \"--not\n--all\", so that a deepening fetch walks the fetched tips down to the\nnew shallow boundary without looking at the alternate's refs at all.\nNegating them instead would stop the walk at the alternate's tips,\nbut the reasoning of cf1e7c0770 applies to them as well: an alternate\nthat is itself shallow, or a partial clone, has refs without the\nhistory behind them. fetch-pack sends the alternate's refs as \"have\"s\nin that case too, so the server omits everything behind them, and a\ncheck that stops at those refs does not notice that the objects\nbetween the alternate's boundary and the new one are missing. The walk\ndown to the boundary is bounded by the depth, so this does not bring\nback the full history walk, and objects that exist only in the\nalternate are still found through the object store.\n\nAdd tests to t5618 that check the arguments of the rev-list child via\nGIT_TRACE2_EVENT for a regular and for a deepening fetch. The result\nof the check is the same either way for a complete alternate, so\nthere is no behavioural difference to assert on. The difference is\nonly in which objects are walked.\n\nSigned-off-by: Igor Wiedler \n---\n connected.c | 2 +-\n t/t5618-alternate-refs.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/connected.c b/connected.c\nindex 929b9bd28d..d8bfb3dbf2 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n \t\t\t\t opt->exclude_hidden_refs_section);\n \t\tstrvec_push(&rev_list.args, \"--all\");\n+\t\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n \t}\n \tstrvec_push(&rev_list.args, \"--quiet\");\n-\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n \tif (opt->progress)\n \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t _(\"Checking connectivity\"));\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 2fb6d549d3..237eaba85c 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n+\tGIT_TRACE2_EVENT=\"$PWD/fetch.trace\" git fetch origin &&\n+\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n+\t\t--alternate-refs --quiet Add tests to t5618 that check the arguments of the rev-list child via\n> GIT_TRACE2_EVENT for a regular and for a deepening fetch. The result\n> of the check is the same either way for a complete alternate, so\n> there is no behavioural difference to assert on. The difference is\n> only in which objects are walked.\n\nYeah. I couldn't really come up with a reasonable test for this, either.\nWe could corrupt history in the alternate, so if we now were to walk its\nhistory during the connectivity check we'd hit that corruption. But it\nfeels rather roundabout, same as the proposed test, so I'm not sure that\nit's really worth it.\n\n> diff --git a/connected.c b/connected.c\n> index 929b9bd28d..d8bfb3dbf2 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n> \t\t\t\t opt->exclude_hidden_refs_section);\n> \t\tstrvec_push(&rev_list.args, \"--all\");\n> +\t\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n> \t}\n> \tstrvec_push(&rev_list.args, \"--quiet\");\n> -\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n> \tif (opt->progress)\n> \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n> \t\t\t _(\"Checking connectivity\"));\n\nYup, makes sense. \"--all\" and \"--alternate-refs\" are both supposed to\nlimit the revision walk with boundaries that we can already reasonably\nassume to be fully connected. Marking it as interesting is definitely\nthe wrong thing in a deepening fetch, so it makes sense to handle it the\nexact same as we handle \"--all\" already.\n\n> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> index 2fb6d549d3..237eaba85c 100755\n> --- a/t/t5618-alternate-refs.sh\n> +++ b/t/t5618-alternate-refs.sh\n> @@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n> +\tGIT_TRACE2_EVENT=\"$PWD/fetch.trace\" git fetch origin &&\n> +\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n> +\t\t--alternate-refs --quiet +'\n> +\n> +test_expect_success 'connectivity check of a deepening fetch ignores alternate refs' '\n> +\tGIT_TRACE2_EVENT=\"$PWD/deepen.trace\" git fetch --depth=1 origin one &&\n> +\ttest_subcommand_flex git rev-list --objects --stdin --quiet +\ttest_subcommand_flex ! git rev-list --objects --stdin --alternate-refs \\\n> +\t\t +'\n\nThis is a rather roundabout way to test for this, as it exercises what\nwe're doing rather than the actual observed behaviour. But as I said, I\ndon't have a better suggestion either, so I think this is okay-ish.\n\nSo overall I'm happy with this version. Thanks!\n\nPatrick\n\n"},{"id":"555048","messageId":"xmqqcxtcte5m.fsf@gitster.g","threadId":"66531","inReplyTo":"20261014103507.35317-1-iwiedler@gitlab.com","subject":"Re: [PATCH] connected: do not walk alternate refs in deepening fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-14T16:55:49Z","receivedAt":"2026-10-14T16:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Igor Wiedler writes:\n\n> The connectivity check spawned by check_connected() uses\n>\n> git rev-list --objects --stdin --not --all --quiet --alternate-refs\n>\n> where 39b44ba771 (check_everything_connected: assume alternate ref\n> tips are valid, 2019-07-01) added \"--alternate-refs\" so that the walk\n> from the fetched tips stops at the refs of any alternate object store,\n> on the assumption that the alternate is already fully connected.\n>\n> That only works because \"--alternate-refs\" comes after \"--not\".\n> cf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n> 2018-07-02) drops \"--not --all\" for deepening fetches, since those\n> need to verify the whole chain up to the new shallow boundary rather\n> than stopping at refs we already had. For such fetches we therefore\n> run\n>\n> git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n\nOuch! Thanks for spotting the 8-year-old mistake. It shows how\nrare the deepening fetches together with the use of alternate refs\nin real life?\n\n> Move \"--alternate-refs\" into the non-deepening branch next to \"--not\n> --all\", so that a deepening fetch walks the fetched tips down to the\n> new shallow boundary without looking at the alternate's refs at all.\n\nTo paraphrase, we cannot trust refs from repositories we are\nborrowing the objects from without taking the shallow boundaries of\nthese repositories into acocunt, so we ignore them.\n\n> Negating them instead would stop the walk at the alternate's tips,\n> but the reasoning of cf1e7c0770 applies to them as well: an alternate\n> that is itself shallow, or a partial clone, has refs without the\n> history behind them. fetch-pack sends the alternate's refs as \"have\"s\n> in that case too, so the server omits everything behind them, and a\n> check that stops at those refs does not notice that the objects\n> between the alternate's boundary and the new one are missing.\n\nBut stepping back a bit, doesn't the same argument apply even when\nwe are a fully populated repository and making a non-deepening\nfetch? We can trust \"--not --all\" to make the traversal to stop at\ntips of our local refs and can be assured that all history behind\nthe tips we are fetching will still be connected, but a traversal\nthat stops at the tip of a shallow repository will break our\nrepository, and in a worse form than the shallow repository we are\nborrowing from, because operations in that repository are guarded by\nits shallow boundaries that we usually do not see, but operations in\nour repository do not have the same shallow boundaries in effect.\n\nI do agree that for the purpose of \"see if this traversal barfs with\nmissing objects to see if the updated tips have all the objects\nbehind it\", listing the alternate refs behind \"--not\" is the right\nthing to do and it is broken to have \"--alternate-refs\" without\nhaving \"--not\" like the current code does. But I am not convinced\nthat disabling --alternate-refs only when you are deepening is not\nsolving the problems of using alternate-refs introduces. Shouldn't\nthe fix be more like two (or more) steps, i.e.,\n\n * [1/2] for check_connected(), use the attached patch instead.\n\n * [2/2] update odb_for_each_alternate_ref() so that it ignores any\n and all refs from shallow repositories.\n\nHmm?\n\n\n connected.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git i/connected.c w/connected.c\nindex 7e26976832..f4b60489bf 100644\n--- i/connected.c\n+++ w/connected.c\n@@ -91,8 +91,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstrvec_push(&rev_list.args, \"--stdin\");\n \tif (repo_has_promisor_remote(the_repository))\n \t\tstrvec_push(&rev_list.args, \"--exclude-promisor-objects\");\n+\n+\t/* define where the traversal should stop */\n+\tstrvec_push(&rev_list.args, \"--not\");\n \tif (!opt->is_deepening_fetch) {\n-\t\tstrvec_push(&rev_list.args, \"--not\");\n \t\tif (opt->exclude_hidden_refs_section)\n \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n \t\t\t\t opt->exclude_hidden_refs_section);\n\n"},{"id":"555050","messageId":"as-wOIAxpdnVVc8h@denethor","threadId":"66531","inReplyTo":"20261014103507.35317-1-iwiedler@gitlab.com","subject":"Re: [PATCH] connected: do not walk alternate refs in deepening fetches","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-14T17:02:54Z","receivedAt":"2026-10-14T17:02:54Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/14 12:35PM, Igor Wiedler wrote:\n> The connectivity check spawned by check_connected() uses\n> \n> git rev-list --objects --stdin --not --all --quiet --alternate-refs\n> \n> where 39b44ba771 (check_everything_connected: assume alternate ref\n> tips are valid, 2019-07-01) added \"--alternate-refs\" so that the walk\n> from the fetched tips stops at the refs of any alternate object store,\n> on the assumption that the alternate is already fully connected.\n\nOk, when performing a connectivity check, we assume that an alternate\nrepository is already in a valid state. By excluding alternate refs from\nthe git-rev-list(1), some time can potentially be saved.\n\n> That only works because \"--alternate-refs\" comes after \"--not\".\n> cf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n> 2018-07-02) drops \"--not --all\" for deepening fetches, since those\n> need to verify the whole chain up to the new shallow boundary rather\n> than stopping at refs we already had. For such fetches we therefore\n> run\n> \n> git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n> \n> with the alternate's refs as positive revisions, and rev-list\n> enumerates every object reachable from them. Unless one of those refs\n> happens to lie inside the newly fetched shallow range, where the\n> grafts cut the walk short, that is the alternate's entire history.\n\nOk, so for deepening fetches, the `--not --all` is intentially removed\nto allow the newly deepened commits to be verified. A consequence of\nthis though was that the `--alternate-refs` changed from being excluded\nto included unintentially. Makes sense.\n\n> This is slow for setups that attach a large alternate to a fresh\n> repository and then do a shallow fetch of a ref that is a few commits\n> ahead of the alternate, which is how CI runners share a clone of a\n> big repository between jobs. With a 7M-object alternate (a bare clone\n> of gitlab-org/gitlab), \"git fetch --depth 1\" transfers an empty pack\n> in under a second and the connectivity check then takes 227s. Without\n> the walk over the alternate's history the same rev-list finishes in\n> 0.0s.\n\nFor large alternate repositories that contain lots of additional\nhistory, this certainly could result in a significant slow down.\n\n> Move \"--alternate-refs\" into the non-deepening branch next to \"--not\n> --all\", so that a deepening fetch walks the fetched tips down to the\n> new shallow boundary without looking at the alternate's refs at all.\n> Negating them instead would stop the walk at the alternate's tips,\n> but the reasoning of cf1e7c0770 applies to them as well: an alternate\n> that is itself shallow, or a partial clone, has refs without the\n> history behind them. fetch-pack sends the alternate's refs as \"have\"s\n> in that case too, so the server omits everything behind them, and a\n> check that stops at those refs does not notice that the objects\n> between the alternate's boundary and the new one are missing. The walk\n> down to the boundary is bounded by the depth, so this does not bring\n> back the full history walk, and objects that exist only in the\n> alternate are still found through the object store.\n\nNaively I assumed we would still want to negate the alternate refs, but\nIIUC for deepening fetches we may fetch objects that a shallow/partial\nalternate repo _could_ have, but doesn't. Thus we need to perform the\nobject walk up to the new boundary. That makes sense to me.\n\n> Add tests to t5618 that check the arguments of the rev-list child via\n> GIT_TRACE2_EVENT for a regular and for a deepening fetch. The result\n> of the check is the same either way for a complete alternate, so\n> there is no behavioural difference to assert on. The difference is\n> only in which objects are walked.\n> \n> Signed-off-by: Igor Wiedler \n> ---\n> connected.c | 2 +-\n> t/t5618-alternate-refs.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/connected.c b/connected.c\n> index 929b9bd28d..d8bfb3dbf2 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n> \t\t\t\t opt->exclude_hidden_refs_section);\n> \t\tstrvec_push(&rev_list.args, \"--all\");\n> +\t\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n> \t}\n> \tstrvec_push(&rev_list.args, \"--quiet\");\n> -\tstrvec_push(&rev_list.args, \"--alternate-refs\");\n\nOk, now the --alternate-refs option is only used in non-deepening\nfetches. Looks good.\n\n> \tif (opt->progress)\n> \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n> \t\t\t _(\"Checking connectivity\"));\n> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> index 2fb6d549d3..237eaba85c 100755\n> --- a/t/t5618-alternate-refs.sh\n> +++ b/t/t5618-alternate-refs.sh\n> @@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n> +\tGIT_TRACE2_EVENT=\"$PWD/fetch.trace\" git fetch origin &&\n> +\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n> +\t\t--alternate-refs --quiet +'\n> +\n> +test_expect_success 'connectivity check of a deepening fetch ignores alternate refs' '\n> +\tGIT_TRACE2_EVENT=\"$PWD/deepen.trace\" git fetch --depth=1 origin one &&\n> +\ttest_subcommand_flex git rev-list --objects --stdin --quiet +\ttest_subcommand_flex ! git rev-list --objects --stdin --alternate-refs \\\n> +\t\t +'\n> +\n> test_done\n\nOk, the added tests here are simply validing that the spawned\ngit-rev-list(1) command is using the options we now expect.\n\nOverall, this patch looks good to me.\n\n-Justin\n\n"},{"id":"555055","messageId":"20261014181134.56220-1-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"20261014103507.35317-1-iwiedler@gitlab.com","subject":"[PATCH v2 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-14T18:11:32Z","receivedAt":"2026-10-14T18:11:32Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"In a deepening fetch the connectivity check runs rev-list with\n\"--alternate-refs\" as a positive revision, so it walks the alternate's\nwhole history. With a 7M-object alternate the check takes 227s after\na fetch that received an empty pack.\n\nChanges since v1:\n\n * The connectivity check negates \"--alternate-refs\" in both cases,\n as Junio suggested, instead of dropping it for deepening fetches.\n The alternate's refs are a boundary like our own refs. Compared to\n Junio's diff, \"--alternate-refs\" moved up next to \"--all\" so that\n the comment above \"--not\" covers all of the boundary arguments.\n\n * New second patch: odb_for_each_alternate_ref() ignores the refs of\n shallow and grafted alternates, so that neither the connectivity\n check, nor fetch-pack's \"have\"s, nor receive-pack's \".have\"s treat\n history that the alternate does not have as present. Partial\n clones are not detected, see the commit message.\n\nv1: https://lore.kernel.org/git/20261014103507.35317-1-iwiedler@gitlab.com/\n\nIgor Wiedler (2):\n connected: negate alternate refs in deepening fetches too\n odb: ignore refs of shallow and grafted alternates\n\n Documentation/rev-list-options.adoc | 7 ++++---\n connected.c | 6 ++++--\n odb.c | 11 +++++++++++\n t/t5618-alternate-refs.sh | 25 +++++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 5 deletions(-)\n\n\nbase-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd\n-- \n2.56.0\n\n\n"},{"id":"555056","messageId":"20261014181134.56220-2-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"20261014103507.35317-1-iwiedler@gitlab.com","subject":"[PATCH v2 1/2] connected: negate alternate refs in deepening fetches too","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-14T18:11:33Z","receivedAt":"2026-10-14T18:11:33Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"The connectivity check spawned by check_connected() uses\n\n git rev-list --objects --stdin --not --all --quiet --alternate-refs\n\nwhere 39b44ba771 (check_everything_connected: assume alternate ref\ntips are valid, 2019-07-01) added \"--alternate-refs\" so that the walk\nfrom the fetched tips stops at the refs of any alternate object store,\non the assumption that the alternate is already fully connected.\n\nThat only works because \"--alternate-refs\" comes after \"--not\".\ncf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n2018-07-02) drops \"--not --all\" for deepening fetches, since those\nneed to verify the whole chain up to the new shallow boundary rather\nthan stopping at refs we already had. For such fetches we therefore\nrun\n\n git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n\nwith the alternate's refs as positive revisions, and rev-list\nenumerates every object reachable from them. Unless one of those refs\nhappens to lie inside the newly fetched shallow range, where the\ngrafts cut the walk short, that is the alternate's entire history.\n\nThis is slow for setups that attach a large alternate to a fresh\nrepository and then do a shallow fetch of a ref that is a few commits\nahead of the alternate, which is how CI runners share a clone of a\nbig repository between jobs. With a 7M-object alternate (a bare clone\nof gitlab-org/gitlab), \"git fetch --depth 1\" transfers an empty pack\nin under a second and the connectivity check then takes 227s. With\nthe alternate's refs negated the same rev-list finishes in 0.0s.\n\nPass \"--not\" in both cases, before the branch that adds \"--all\" for\nnon-deepening fetches, so that \"--alternate-refs\" is negated for\ndeepening fetches as well. Such a fetch still has to reach the new\nshallow boundary from the fetched tips without stopping at our own\nrefs, which is what cf1e7c0770 wanted. It stops at the alternate's\nrefs, as every other fetch already does. Move \"--alternate-refs\" up\nnext to \"--all\" so that the arguments that bound the walk are in one\nplace.\n\nAdd tests to t5618 that check the arguments of the rev-list child via\nGIT_TRACE2_EVENT for a regular and for a deepening fetch. The result\nof the check is the same either way for a complete alternate, so\nthere is no behavioural difference to assert on. The difference is\nonly in which objects are walked.\n\nHelped-by: Patrick Steinhardt \nHelped-by: Junio C Hamano \nSigned-off-by: Igor Wiedler \n---\n connected.c | 6 ++++--\n t/t5618-alternate-refs.sh | 12 ++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 929b9bd28d..cdf103bba5 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -121,15 +121,17 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstrvec_push(&rev_list.args, \"--stdin\");\n \tif (repo_has_promisor_remote(the_repository))\n \t\tstrvec_push(&rev_list.args, \"--exclude-promisor-objects\");\n+\n+\t/* define where the traversal should stop */\n+\tstrvec_push(&rev_list.args, \"--not\");\n \tif (!opt->is_deepening_fetch) {\n-\t\tstrvec_push(&rev_list.args, \"--not\");\n \t\tif (opt->exclude_hidden_refs_section)\n \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n \t\t\t\t opt->exclude_hidden_refs_section);\n \t\tstrvec_push(&rev_list.args, \"--all\");\n \t}\n-\tstrvec_push(&rev_list.args, \"--quiet\");\n \tstrvec_push(&rev_list.args, \"--alternate-refs\");\n+\tstrvec_push(&rev_list.args, \"--quiet\");\n \tif (opt->progress)\n \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t _(\"Checking connectivity\"));\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 2fb6d549d3..4cc39af0f0 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -58,4 +58,16 @@ test_expect_success 'log --source shows .alternate marker' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n+\tGIT_TRACE2_EVENT=\"$PWD/fetch.trace\" git fetch origin &&\n+\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n+\t\t--alternate-refs --quiet \".\n\n\"git clone --reference\" already rejects a shallow or grafted\nreference repository for this reason, but nothing prevents such a\nrepository from being added via objects/info/alternates or\nGIT_ALTERNATE_OBJECT_DIRECTORIES, or from being made shallow after\nother repositories started borrowing from it.\n\nSkip the refs of an alternate that has a \"shallow\" file or an\n\"info/grafts\" file, the same checks clone uses. Its objects are still\navailable through the object store. Only its refs are no longer used\nas a boundary.\n\nPartial clones have the same problem for the objects they lack, but\ndetecting one requires reading the alternate's config, so this\npatch does not handle them.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Igor Wiedler \n---\n Documentation/rev-list-options.adoc | 7 ++++---\n odb.c | 11 +++++++++++\n t/t5618-alternate-refs.sh | 13 +++++++++++++\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc\nindex bd9f345690..982a53c694 100644\n--- a/Documentation/rev-list-options.adoc\n+++ b/Documentation/rev-list-options.adoc\n@@ -223,9 +223,10 @@ explicitly.\n \tPretend as if all objects mentioned as ref tips of alternate\n \trepositories were listed on the command line. An alternate\n \trepository is any repository whose object directory is specified\n-\tin `objects/info/alternates`. The set of included objects may\n-\tbe modified by `core.alternateRefsCommand`, etc. See\n-\tlinkgit:git-config[1].\n+\tin `objects/info/alternates`. The refs of shallow and grafted\n+\talternate repositories are ignored, as the history behind them\n+\tmay be missing. The set of included objects may be modified by\n+\t`core.alternateRefsCommand`, etc. See linkgit:git-config[1].\n \n `--single-worktree`::\n \tBy default, all working trees will be examined by the\ndiff --git a/odb.c b/odb.c\nindex 0200e26f21..e6bb7ce6e6 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -458,6 +458,17 @@ static int refs_from_alternate_cb(struct odb_source *alternate,\n \t\tgoto out;\n \tstrbuf_setlen(&path, base_len);\n \n+\t/* A shallow or grafted repository may lack the history behind its refs. */\n+\tstrbuf_addstr(&path, \"/shallow\");\n+\tif (!access(path.buf, F_OK))\n+\t\tgoto out;\n+\tstrbuf_setlen(&path, base_len);\n+\n+\tstrbuf_addstr(&path, \"/info/grafts\");\n+\tif (!access(path.buf, F_OK))\n+\t\tgoto out;\n+\tstrbuf_setlen(&path, base_len);\n+\n \tread_alternate_refs(alternate->odb->repo, path.buf, cb->fn, cb->payload);\n \n out:\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 4cc39af0f0..9c3862709a 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate\n \t\t--alternate-refs --quiet shallow-tip &&\n+\tcp .git/objects/info/alternates alternates.bak &&\n+\ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n+\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n+\tgit cat-file -e $(cat shallow-tip) &&\n+\tgit rev-list --remotes=origin >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.56.0\n\n\n"},{"id":"555063","messageId":"xmqqy0c0qe70.fsf@gitster.g","threadId":"66531","inReplyTo":"20261014181134.56220-1-iwiedler@gitlab.com","subject":"Re: [PATCH v2 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-14T19:23:15Z","receivedAt":"2026-10-14T19:23:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Igor Wiedler writes:\n\n> In a deepening fetch ...\n\nThe usual \"engage in discussion before sending an updated patchset\"\napplies here.\n\ncf. https://lore.kernel.org/git/xmqqece02no9.fsf@gitster.g/\n\nThanks.\n\n"},{"id":"555099","messageId":"20261015064158.GA1979109@coredump.intra.peff.net","threadId":"66531","inReplyTo":"xmqqcxtcte5m.fsf@gitster.g","subject":"Re: [PATCH] connected: do not walk alternate refs in deepening fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-15T06:41:58Z","receivedAt":"2026-10-15T06:41:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2026 at 09:55:49AM -0700, Junio C Hamano wrote:\n\n> > That only works because \"--alternate-refs\" comes after \"--not\".\n> > cf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n> > 2018-07-02) drops \"--not --all\" for deepening fetches, since those\n> > need to verify the whole chain up to the new shallow boundary rather\n> > than stopping at refs we already had. For such fetches we therefore\n> > run\n> >\n> > git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n> \n> Ouch! Thanks for spotting the 8-year-old mistake. It shows how\n> rare the deepening fetches together with the use of alternate refs\n> in real life?\n\nThanks for cc-ing me. I looked at Igor's v1 patch, and it's exactly the\nsolution I would have expected. It should have been written that way in\n39b44ba771 (check_everything_connected: assume alternate ref tips are\nvalid, 2019-07-01), because the intent was \"treat alternate ref tips\njust like we treat our own existing ref tips\". So wherever we use\n\"--all\", we should use \"--alternate-refs\", and since --all was moved\ninto that non-deepening conditional, --alternate-refs should go there,\ntoo.\n\nBTW, if you are curious why 39b44ba771 made such an obvious mistake, it\nis exactly because it predated that move! The original patch was written\nin 2014 and used internally at GitHub. I rebased it forward to send to\nthe list in 2019, but I don't even think I got a textual conflict at all\n(because the movement of \"--not\" and \"--all\" were a few lines away).\n\nSo it was a very subtle semantic conflict between the two patches. But\nbecause the rebase rewrote history to put mine on top, it looks like I\njust did it wrong from the start. ;)\n\n> But stepping back a bit, doesn't the same argument apply even when\n> we are a fully populated repository and making a non-deepening\n> fetch? We can trust \"--not --all\" to make the traversal to stop at\n> tips of our local refs and can be assured that all history behind\n> the tips we are fetching will still be connected, but a traversal\n> that stops at the tip of a shallow repository will break our\n> repository, and in a worse form than the shallow repository we are\n> borrowing from, because operations in that repository are guarded by\n> its shallow boundaries that we usually do not see, but operations in\n> our repository do not have the same shallow boundaries in effect.\n\nIt took me a few reads to parse this, but I think you are saying: it is\na problem if we are a non-shallow repository trying to use a shallow\nalternate for verification?\n\nThat sounds likely, though this is the first time I have looked at\ncf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n2018-07-02) and the surrounding issues. As I understand the issue, it is\nreally \"are the refs we are going to use to verify coming from a shallow\nrepo which might not have all objects\". But I'm not sure how that\ninteracts with opt->is_deepening_fetch (and why it isn't just checking\nif we are in a shallow repo).\n\n> I do agree that for the purpose of \"see if this traversal barfs with\n> missing objects to see if the updated tips have all the objects\n> behind it\", listing the alternate refs behind \"--not\" is the right\n> thing to do and it is broken to have \"--alternate-refs\" without\n> having \"--not\" like the current code does. But I am not convinced\n> that disabling --alternate-refs only when you are deepening is not\n> solving the problems of using alternate-refs introduces. Shouldn't\n> the fix be more like two (or more) steps, i.e.,\n> \n> * [1/2] for check_connected(), use the attached patch instead.\n\nOK, so this moves the \"--not\" out so that we always do it. It's ignored\nin the case that we are deepening and don't have any alternates, but it\nshouldn't hurt anything.\n\n> * [2/2] update odb_for_each_alternate_ref() so that it ignores any\n> and all refs from shallow repositories.\n\nYeah, this seems reasonable. Alternates don't necessarily even have a\nfull repo, but in that case they wouldn't have any refs, either (IIRC we\ndo some magic to guess at the containing repo of the alternate).\n\n-Peff\n\n"},{"id":"555114","messageId":"20261015065754.GB1979109@coredump.intra.peff.net","threadId":"66531","inReplyTo":"20261014181134.56220-3-iwiedler@gitlab.com","subject":"Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-15T06:57:54Z","receivedAt":"2026-10-15T06:57:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:\n\n> That assumption does not hold for a shallow repository, whose refs\n> point at commits with no history behind them below its shallow\n> boundary, nor for a grafted one. A repository borrowing objects from\n> such an alternate does not see the alternate's shallow file, so the\n> history that is missing there is missing here as well. The fetch then\n> succeeds, the connectivity check stops at the alternate's refs, and\n> a later \"git log\" fails with \"Could not read \".\n\nIn addition to grafts there are replace refs. I think we generally\ninsisted on having full connectivity of the original objects, though\n(e.g., fsck will ignore replace refs). I thought that was true of\ngrafts, too, though (except for ones grafted by the shallow mechanism,\nwhich is special).\n\n> Partial clones have the same problem for the objects they lack, but\n> detecting one requires reading the alternate's config, so this\n> patch does not handle them.\n\nYeah, I'm sure there are some corner cases there. In theory you can grab\nany of the missing partial-clone objects at will, though probably we\nhave no code to realize that it's the alternate which is partial and\nthrough which the request should be routed. I guess you could argue the\nsame is true for shallow alternates (i.e., we _could_ get those objects\nif we needed them, we just don't have a way to do so).\n\nThese are such weird setups that I think punting and saying \"just ignore\ntheir alternate refs\" is totally fine. The use of the alternate refs is\nan optimization, so falling back to the simpler behavior is good.\n\n> --- a/odb.c\n> +++ b/odb.c\n> @@ -458,6 +458,17 @@ static int refs_from_alternate_cb(struct odb_source *alternate,\n> \t\tgoto out;\n> \tstrbuf_setlen(&path, base_len);\n> \n> +\t/* A shallow or grafted repository may lack the history behind its refs. */\n> +\tstrbuf_addstr(&path, \"/shallow\");\n> +\tif (!access(path.buf, F_OK))\n> +\t\tgoto out;\n> +\tstrbuf_setlen(&path, base_len);\n> +\n> +\tstrbuf_addstr(&path, \"/info/grafts\");\n> +\tif (!access(path.buf, F_OK))\n> +\t\tgoto out;\n> +\tstrbuf_setlen(&path, base_len);\n\nOK. It's a little gross that we have to check these magic paths, but\nthere's no real alternative here. We do not have a real repository\nobject or anything like it representing the alternate.\n\nThe thread has been talking about check_connected(), but the same\nproblem applies to advertising alternates for transfer. But your fix\nhere takes care of both, because it works at the lowest level. Good.\n\n> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> index 4cc39af0f0..9c3862709a 100755\n> --- a/t/t5618-alternate-refs.sh\n> +++ b/t/t5618-alternate-refs.sh\n> @@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate\n> \t\t--alternate-refs --quiet '\n> \n> +test_expect_success 'refs of a shallow alternate are ignored' '\n> +\tgit clone --depth=1 \"file://$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/shallow\" &&\n> +\ttest_commit -C \"$TRASH_DIRECTORY/shallow\" shallow-only &&\n> +\tgit -C \"$TRASH_DIRECTORY/shallow\" rev-parse HEAD >shallow-tip &&\n> +\tcp .git/objects/info/alternates alternates.bak &&\n> +\ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n> +\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n> +\tgit cat-file -e $(cat shallow-tip) &&\n> +\tgit rev-list --remotes=origin >expect &&\n> +\tgit rev-list --alternate-refs >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis is a direct test of --alternate-refs, which is probably reasonable\nand the simplest way of doing it. I think you could also construct a\ncase where a fetch or push causes the recipient repo to be corrupted (it\nclaimed to have object X because of the alternate, so the other side\ndidn't send it, but since _we're_ not shallow, now fsck, etc, fails).\nBut I don't think there's a particular need to construct that full case\nfor the tests.\n\n-Peff\n\n"},{"id":"555173","messageId":"xmqqqzhqoozj.fsf@gitster.g","threadId":"66531","inReplyTo":"20261015065754.GB1979109@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-15T17:25:20Z","receivedAt":"2026-10-15T17:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The thread has been talking about check_connected(), but the same\n> problem applies to advertising alternates for transfer. But your fix\n> here takes care of both, because it works at the lowest level. Good.\n\n;-)\n\n> This is a direct test of --alternate-refs, which is probably reasonable\n> and the simplest way of doing it. I think you could also construct a\n> case where a fetch or push causes the recipient repo to be corrupted (it\n> claimed to have object X because of the alternate, so the other side\n> didn't send it, but since _we're_ not shallow, now fsck, etc, fails).\n> But I don't think there's a particular need to construct that full case\n> for the tests.\n\nYeah, thanks for thinking it through thoroughly. I appreciate it\nvery much.\n\n\n"},{"id":"555205","messageId":"20261016031439.GA3437448@coredump.intra.peff.net","threadId":"66531","inReplyTo":"20261014181134.56220-3-iwiedler@gitlab.com","subject":"Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-16T03:14:39Z","receivedAt":"2026-10-16T03:14:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:\n\n> +test_expect_success 'refs of a shallow alternate are ignored' '\n> +\tgit clone --depth=1 \"file://$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/shallow\" &&\n> +\ttest_commit -C \"$TRASH_DIRECTORY/shallow\" shallow-only &&\n> +\tgit -C \"$TRASH_DIRECTORY/shallow\" rev-parse HEAD >shallow-tip &&\n> +\tcp .git/objects/info/alternates alternates.bak &&\n> +\ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n> +\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n> +\tgit cat-file -e $(cat shallow-tip) &&\n> +\tgit rev-list --remotes=origin >expect &&\n> +\tgit rev-list --alternate-refs >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis seemed to fail the windows ci job with cat-file reporting:\n\n error: unable to normalize alternate object path: /d/a/git/git/t/trash directory.t5618-alternate-refs/shallow/.git/objects\n\nJust a guess, but it might be the Windows vs POSIX path thing? So\nperhaps something like this would work, but I didn't test it:\n\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 9c3862709a..e5dce85cff 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -76,7 +76,7 @@ test_expect_success 'refs of a shallow alternate are ignored' '\n \tgit -C \"$TRASH_DIRECTORY/shallow\" rev-parse HEAD >shallow-tip &&\n \tcp .git/objects/info/alternates alternates.bak &&\n \ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n-\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n+\techo \"$(pwd)/shallow/.git/objects\" >>.git/objects/info/alternates &&\n \tgit cat-file -e $(cat shallow-tip) &&\n \tgit rev-list --remotes=origin >expect &&\n \tgit rev-list --alternate-refs >actual &&\n\nAlternatively, we could actually clone \"shallow\" with --reference or\n--shared, which would let Git write the alternates file itself (but\nwould mean running rev-list, etc, in a subrepo).\n\nWe also mention $TRASH_DIRECTORY in some other spots. I think they are\nnot a problem, but usually we assume that tests start in the trash\ndirectory and don't change directory (at least not outside of a\nsubshell). So as a matter of style, we can probably make all of those\nrelative paths.\n\nAlso, a very minor style nit, but we tend to use environment variables\ninstead of files for holding values. So all together, perhaps (again,\nonly lightly tested):\n\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 9c3862709a..5ac981c5a0 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -71,13 +71,13 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate\n '\n \n test_expect_success 'refs of a shallow alternate are ignored' '\n-\tgit clone --depth=1 \"file://$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/shallow\" &&\n-\ttest_commit -C \"$TRASH_DIRECTORY/shallow\" shallow-only &&\n-\tgit -C \"$TRASH_DIRECTORY/shallow\" rev-parse HEAD >shallow-tip &&\n+\tgit clone --depth=1 --no-local . shallow &&\n+\ttest_commit -C shallow shallow-only &&\n+\tshallow_tip=$(git -C shallow rev-parse HEAD) &&\n \tcp .git/objects/info/alternates alternates.bak &&\n \ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n-\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n-\tgit cat-file -e $(cat shallow-tip) &&\n+\techo \"$(pwd)/shallow/.git/objects\" >>.git/objects/info/alternates &&\n+\tgit cat-file -e $shallow_tip &&\n \tgit rev-list --remotes=origin >expect &&\n \tgit rev-list --alternate-refs >actual &&\n \ttest_cmp expect actual\n\n-Peff\n\n"},{"id":"555339","messageId":"xmqq7bjfby9s.fsf@gitster.g","threadId":"66531","inReplyTo":"20261014181134.56220-3-iwiedler@gitlab.com","subject":"Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-18T01:19:59Z","receivedAt":"2026-10-18T01:19:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Igor Wiedler writes:\n\n> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> index 4cc39af0f0..9c3862709a 100755\n> --- a/t/t5618-alternate-refs.sh\n> +++ b/t/t5618-alternate-refs.sh\n> @@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate\n> \t\t--alternate-refs --quiet '\n> \n> +test_expect_success 'refs of a shallow alternate are ignored' '\n> +\tgit clone --depth=1 \"file://$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/shallow\" &&\n> +\ttest_commit -C \"$TRASH_DIRECTORY/shallow\" shallow-only &&\n> +\tgit -C \"$TRASH_DIRECTORY/shallow\" rev-parse HEAD >shallow-tip &&\n> +\tcp .git/objects/info/alternates alternates.bak &&\n> +\ttest_when_finished \"mv alternates.bak .git/objects/info/alternates\" &&\n> +\techo \"$TRASH_DIRECTORY/shallow/.git/objects\" >>.git/objects/info/alternates &&\n> +\tgit cat-file -e $(cat shallow-tip) &&\n> +\tgit rev-list --remotes=origin >expect &&\n> +\tgit rev-list --alternate-refs >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis seems to break CI only on Windows\n\n https://github.com/git/git/actions/runs/39084117165/job/117547610467#step:5:279\n https://github.com/git/git/actions/runs/39084117165/job/117547743280#step:6:272\n\nI'll eject the topic from 'seen' for now.\n\nThanks.\n\n\n"},{"id":"555409","messageId":"20261019110249.70341-1-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"20261016031439.GA3437448@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-19T11:02:49Z","receivedAt":"2026-10-19T11:02:49Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"On Thu, Oct 15, 2026 at 11:14:39PM -0400, Jeff King wrote:\n> This seemed to fail the windows ci job with cat-file reporting:\n>\n> error: unable to normalize alternate object path: /d/a/git/git/t/trash directory.t5618-alternate-refs/shallow/.git/objects\n>\n> Just a guess, but it might be the Windows vs POSIX path thing? So\n> perhaps something like this would work (...)\n\nThanks, I will include this fix in the v3 patchset.\n\n"},{"id":"555410","messageId":"20261019110918.71193-1-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"20261014181134.56220-1-iwiedler@gitlab.com","subject":"[PATCH v3 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-19T11:09:16Z","receivedAt":"2026-10-19T11:09:16Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"In a deepening fetch the connectivity check passes \"--alternate-refs\"\nas a positive revision and walks the alternate's whole history. With a\n7M-object alternate that takes 227s after a fetch of an empty pack.\n\nChanges since v2:\n\n * The shallow alternate test wrote $TRASH_DIRECTORY into\n objects/info/alternates, which git on Windows cannot resolve. It now\n uses Peff's version with $(pwd) and relative paths.\n\n * The trace tests use $(pwd) instead of $PWD.\n\nNo code changes.\n\nv2: https://lore.kernel.org/git/20261014181134.56220-1-iwiedler@gitlab.com/\nv1: https://lore.kernel.org/git/20261014103507.35317-1-iwiedler@gitlab.com/\n\nIgor Wiedler (2):\n connected: negate alternate refs in deepening fetches too\n odb: ignore refs of shallow and grafted alternates\n\n Documentation/rev-list-options.adoc | 7 ++++---\n connected.c | 6 ++++--\n odb.c | 11 +++++++++++\n t/t5618-alternate-refs.sh | 25 +++++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 5 deletions(-)\n\n\nbase-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd\n-- \n2.56.0\n\n\n"},{"id":"555411","messageId":"20261019110918.71193-2-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"20261014181134.56220-1-iwiedler@gitlab.com","subject":"[PATCH v3 1/2] connected: negate alternate refs in deepening fetches too","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-19T11:09:17Z","receivedAt":"2026-10-19T11:09:17Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"The connectivity check spawned by check_connected() uses\n\n git rev-list --objects --stdin --not --all --quiet --alternate-refs\n\nwhere 39b44ba771 (check_everything_connected: assume alternate ref\ntips are valid, 2019-07-01) added \"--alternate-refs\" so that the walk\nfrom the fetched tips stops at the refs of any alternate object store,\non the assumption that the alternate is already fully connected.\n\nThat only works because \"--alternate-refs\" comes after \"--not\".\ncf1e7c0770 (fetch-pack: write shallow, then check connectivity,\n2018-07-02) drops \"--not --all\" for deepening fetches, since those\nneed to verify the whole chain up to the new shallow boundary rather\nthan stopping at refs we already had. For such fetches we therefore\nrun\n\n git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs\n\nwith the alternate's refs as positive revisions, and rev-list\nenumerates every object reachable from them. Unless one of those refs\nhappens to lie inside the newly fetched shallow range, where the\ngrafts cut the walk short, that is the alternate's entire history.\n\nThis is slow for setups that attach a large alternate to a fresh\nrepository and then do a shallow fetch of a ref that is a few commits\nahead of the alternate, which is how CI runners share a clone of a\nbig repository between jobs. With a 7M-object alternate (a bare clone\nof gitlab-org/gitlab), \"git fetch --depth 1\" transfers an empty pack\nin under a second and the connectivity check then takes 227s. With\nthe alternate's refs negated the same rev-list finishes in 0.0s.\n\nPass \"--not\" in both cases, before the branch that adds \"--all\" for\nnon-deepening fetches, so that \"--alternate-refs\" is negated for\ndeepening fetches as well. Such a fetch still has to reach the new\nshallow boundary from the fetched tips without stopping at our own\nrefs, which is what cf1e7c0770 wanted. It stops at the alternate's\nrefs, as every other fetch already does. Move \"--alternate-refs\" up\nnext to \"--all\" so that the arguments that bound the walk are in one\nplace.\n\nAdd tests to t5618 that check the arguments of the rev-list child via\nGIT_TRACE2_EVENT for a regular and for a deepening fetch. The result\nof the check is the same either way for a complete alternate, so\nthere is no behavioural difference to assert on. The difference is\nonly in which objects are walked.\n\nHelped-by: Patrick Steinhardt \nHelped-by: Junio C Hamano \nSigned-off-by: Igor Wiedler \n---\n connected.c | 6 ++++--\n t/t5618-alternate-refs.sh | 12 ++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 929b9bd28d..cdf103bba5 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -121,15 +121,17 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstrvec_push(&rev_list.args, \"--stdin\");\n \tif (repo_has_promisor_remote(the_repository))\n \t\tstrvec_push(&rev_list.args, \"--exclude-promisor-objects\");\n+\n+\t/* define where the traversal should stop */\n+\tstrvec_push(&rev_list.args, \"--not\");\n \tif (!opt->is_deepening_fetch) {\n-\t\tstrvec_push(&rev_list.args, \"--not\");\n \t\tif (opt->exclude_hidden_refs_section)\n \t\t\tstrvec_pushf(&rev_list.args, \"--exclude-hidden=%s\",\n \t\t\t\t opt->exclude_hidden_refs_section);\n \t\tstrvec_push(&rev_list.args, \"--all\");\n \t}\n-\tstrvec_push(&rev_list.args, \"--quiet\");\n \tstrvec_push(&rev_list.args, \"--alternate-refs\");\n+\tstrvec_push(&rev_list.args, \"--quiet\");\n \tif (opt->progress)\n \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t _(\"Checking connectivity\"));\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 2fb6d549d3..3c5fbef463 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -58,4 +58,16 @@ test_expect_success 'log --source shows .alternate marker' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/fetch.trace\" git fetch origin &&\n+\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n+\t\t--alternate-refs --quiet \".\n\n\"git clone --reference\" already rejects a shallow or grafted\nreference repository for this reason, but nothing prevents such a\nrepository from being added via objects/info/alternates or\nGIT_ALTERNATE_OBJECT_DIRECTORIES, or from being made shallow after\nother repositories started borrowing from it.\n\nSkip the refs of an alternate that has a \"shallow\" file or an\n\"info/grafts\" file, the same checks clone uses. Its objects are still\navailable through the object store. Only its refs are no longer used\nas a boundary.\n\nPartial clones have the same problem for the objects they lack, but\ndetecting one requires reading the alternate's config, so this\npatch does not handle them.\n\nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Igor Wiedler \n---\n Documentation/rev-list-options.adoc | 7 ++++---\n odb.c | 11 +++++++++++\n t/t5618-alternate-refs.sh | 13 +++++++++++++\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc\nindex bd9f345690..982a53c694 100644\n--- a/Documentation/rev-list-options.adoc\n+++ b/Documentation/rev-list-options.adoc\n@@ -223,9 +223,10 @@ explicitly.\n \tPretend as if all objects mentioned as ref tips of alternate\n \trepositories were listed on the command line. An alternate\n \trepository is any repository whose object directory is specified\n-\tin `objects/info/alternates`. The set of included objects may\n-\tbe modified by `core.alternateRefsCommand`, etc. See\n-\tlinkgit:git-config[1].\n+\tin `objects/info/alternates`. The refs of shallow and grafted\n+\talternate repositories are ignored, as the history behind them\n+\tmay be missing. The set of included objects may be modified by\n+\t`core.alternateRefsCommand`, etc. See linkgit:git-config[1].\n \n `--single-worktree`::\n \tBy default, all working trees will be examined by the\ndiff --git a/odb.c b/odb.c\nindex 0200e26f21..e6bb7ce6e6 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -458,6 +458,17 @@ static int refs_from_alternate_cb(struct odb_source *alternate,\n \t\tgoto out;\n \tstrbuf_setlen(&path, base_len);\n \n+\t/* A shallow or grafted repository may lack the history behind its refs. */\n+\tstrbuf_addstr(&path, \"/shallow\");\n+\tif (!access(path.buf, F_OK))\n+\t\tgoto out;\n+\tstrbuf_setlen(&path, base_len);\n+\n+\tstrbuf_addstr(&path, \"/info/grafts\");\n+\tif (!access(path.buf, F_OK))\n+\t\tgoto out;\n+\tstrbuf_setlen(&path, base_len);\n+\n \tread_alternate_refs(alternate->odb->repo, path.buf, cb->fn, cb->payload);\n \n out:\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 3c5fbef463..68139a1b3e 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate\n \t\t--alternate-refs --quiet >.git/objects/info/alternates &&\n+\tgit cat-file -e $shallow_tip &&\n+\tgit rev-list --remotes=origin >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.56.0\n\n\n"},{"id":"555480","messageId":"xmqqy0btz3m6.fsf@gitster.g","threadId":"66531","inReplyTo":"20261019110918.71193-1-iwiedler@gitlab.com","subject":"Re: [PATCH v3 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-19T23:12:49Z","receivedAt":"2026-10-19T23:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Igor Wiedler writes:\n\n> In a deepening fetch the connectivity check passes \"--alternate-refs\"\n> as a positive revision and walks the alternate's whole history. With a\n> 7M-object alternate that takes 227s after a fetch of an empty pack.\n>\n> Changes since v2:\n>\n> * The shallow alternate test wrote $TRASH_DIRECTORY into\n> objects/info/alternates, which git on Windows cannot resolve. It now\n> uses Peff's version with $(pwd) and relative paths.\n>\n> * The trace tests use $(pwd) instead of $PWD.\n>\n> No code changes.\n>\n> v2: https://lore.kernel.org/git/20261014181134.56220-1-iwiedler@gitlab.com/\n> v1: https://lore.kernel.org/git/20261014103507.35317-1-iwiedler@gitlab.com/\n>\n> Igor Wiedler (2):\n> connected: negate alternate refs in deepening fetches too\n> odb: ignore refs of shallow and grafted alternates\n\nYour patches are not sent as replies to the cover letter of the same\nround, which is not how we expect the threads to be structured.\nPlease study how others' patch threads look like.\n\nThanks.\n\n"},{"id":"555505","messageId":"20261020075536.49286-1-iwiedler@gitlab.com","threadId":"66531","inReplyTo":"xmqqy0btz3m6.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Igor Wiedler","fromEmail":"iwiedler@gitlab.com","sentAt":"2026-10-20T07:55:36Z","receivedAt":"2026-10-20T07:55:36Z","isPatch":true,"sender":{"key":"iwiedler@gitlab.com","avatar":null},"body":"On Mon, Oct 19, 2026 at 04:12:49PM -0700, Junio C Hamano wrote:\n> Your patches are not sent as replies to the cover letter of the same\n> round, which is not how we expect the threads to be structured.\n> Please study how others' patch threads look like.\n\nAck. Thanks for the pointer (and the patience), will fix this on the\nnext round.\n\n"},{"id":"555979","messageId":"ats6VLXUqKwKZzIY@pks.im","threadId":"66531","inReplyTo":"20261019110918.71193-3-iwiedler@gitlab.com","subject":"Re: [PATCH v3 2/2] odb: ignore refs of shallow and grafted alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-23T10:43:47Z","receivedAt":"2026-10-23T10:43:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 19, 2026 at 01:09:18PM +0200, Igor Wiedler wrote:\n> odb_for_each_alternate_ref() reports the refs of every alternate\n> object store that is a repository. Its three callers use them as\n> commits whose history can be assumed to be present: \"rev-list\n> --alternate-refs\" (and through it the connectivity check after a\n> fetch), fetch-pack, which sends them as \"have\"s so that the server\n> does not resend what the alternate has, and receive-pack, which\n> advertises them as \".have\".\n\nRight, for all of these three callers it doesn't make any sense to\nadvertise those haves in case the repository is shallow. It's a somewhat\nfunny setup to have shallow alternates, but in case anybody does, this\nchange here would avoid a potentially corrupted repository.\n\n> That assumption does not hold for a shallow repository, whose refs\n> point at commits with no history behind them below its shallow\n> boundary, nor for a grafted one. A repository borrowing objects from\n> such an alternate does not see the alternate's shallow file, so the\n> history that is missing there is missing here as well. The fetch then\n> succeeds, the connectivity check stops at the alternate's refs, and\n> a later \"git log\" fails with \"Could not read \".\n\nYup.\n\n> \"git clone --reference\" already rejects a shallow or grafted\n> reference repository for this reason, but nothing prevents such a\n> repository from being added via objects/info/alternates or\n> GIT_ALTERNATE_OBJECT_DIRECTORIES, or from being made shallow after\n> other repositories started borrowing from it.\n> \n> Skip the refs of an alternate that has a \"shallow\" file or an\n> \"info/grafts\" file, the same checks clone uses. Its objects are still\n> available through the object store. Only its refs are no longer used\n> as a boundary.\n\nIt could of course be that somebody manually created such a repository\nand due to some obscure reason still expects \"--alternate-refs\" to work.\nBut I feel like this use case is sufficiently unlikely to simply not\nmatter in practice, and the fact that Git refuses creating this setup in\nthe first place is a good indicator that nobody will care about this\nchange.\n\nI feel like the whole concept of alternate refs is quite flawed in the\nfirst place. We get as input an object directory and then we hope that\nits parent directory _may_ be a proper repository.\n\nOther than that this looks good to me, thanks!\n\nPatrick\n\n"},{"id":"555980","messageId":"ats6prUg8edTtUgY@pks.im","threadId":"66531","inReplyTo":"20261020075536.49286-1-iwiedler@gitlab.com","subject":"Re: [PATCH v3 0/2] connected: fix the connectivity check with alternates in deepening fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-23T10:44:54Z","receivedAt":"2026-10-23T10:44:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 20, 2026 at 09:55:36AM +0200, Igor Wiedler wrote:\n> On Mon, Oct 19, 2026 at 04:12:49PM -0700, Junio C Hamano wrote:\n> > Your patches are not sent as replies to the cover letter of the same\n> > round, which is not how we expect the threads to be structured.\n> > Please study how others' patch threads look like.\n> \n> Ack. Thanks for the pointer (and the patience), will fix this on the\n> next round.\n\nThis version looks good to me, so I guess there's no need for another\nreroll just to fix the threading. Thanks!\n\nPatrick\n\n"}]}