* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
@ 2026-10-14 12:25 ` Patrick Steinhardt
2026-10-14 16:55 ` Junio C Hamano
2026-10-14 17:02 ` Justin Tobler
2 siblings, 0 replies; 4+ messages in thread
From: Patrick Steinhardt @ 2026-10-14 12:25 UTC (permalink / raw)
To: Igor Wiedler; +Cc: git
On Wed, Oct 14, 2026 at 12:35:07PM +0200, Igor Wiedler wrote:
For transparency: I've reviewed this patch internally, so I won't have
much to complain about as we've already been iterating on the patch.
[snip]
> Add tests to t5618 that check the arguments of the rev-list child via
> GIT_TRACE2_EVENT for a regular and for a deepening fetch. The result
> of the check is the same either way for a complete alternate, so
> there is no behavioural difference to assert on. The difference is
> only in which objects are walked.
Yeah. I couldn't really come up with a reasonable test for this, either.
We could corrupt history in the alternate, so if we now were to walk its
history during the connectivity check we'd hit that corruption. But it
feels rather roundabout, same as the proposed test, so I'm not sure that
it's really worth it.
> diff --git a/connected.c b/connected.c
> index 929b9bd28d..d8bfb3dbf2 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
> opt->exclude_hidden_refs_section);
> strvec_push(&rev_list.args, "--all");
> + strvec_push(&rev_list.args, "--alternate-refs");
> }
> strvec_push(&rev_list.args, "--quiet");
> - strvec_push(&rev_list.args, "--alternate-refs");
> if (opt->progress)
> strvec_pushf(&rev_list.args, "--progress=%s",
> _("Checking connectivity"));
Yup, makes sense. "--all" and "--alternate-refs" are both supposed to
limit the revision walk with boundaries that we can already reasonably
assume to be fully connected. Marking it as interesting is definitely
the wrong thing in a deepening fetch, so it makes sense to handle it the
exact same as we handle "--all" already.
> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> index 2fb6d549d3..237eaba85c 100755
> --- a/t/t5618-alternate-refs.sh
> +++ b/t/t5618-alternate-refs.sh
> @@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '
> test_cmp expect actual
> '
>
> +test_expect_success 'connectivity check of a fetch excludes alternate refs' '
> + GIT_TRACE2_EVENT="$PWD/fetch.trace" git fetch origin &&
> + test_subcommand_flex git rev-list --objects --stdin --not --all \
> + --alternate-refs --quiet <fetch.trace
> +'
> +
> +test_expect_success 'connectivity check of a deepening fetch ignores alternate refs' '
> + GIT_TRACE2_EVENT="$PWD/deepen.trace" git fetch --depth=1 origin one &&
> + test_subcommand_flex git rev-list --objects --stdin --quiet <deepen.trace &&
> + test_subcommand_flex ! git rev-list --objects --stdin --alternate-refs \
> + <deepen.trace
> +'
This is a rather roundabout way to test for this, as it exercises what
we're doing rather than the actual observed behaviour. But as I said, I
don't have a better suggestion either, so I think this is okay-ish.
So overall I'm happy with this version. Thanks!
Patrick
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
2026-10-14 12:25 ` Patrick Steinhardt
@ 2026-10-14 16:55 ` Junio C Hamano
2026-10-14 17:02 ` Justin Tobler
2 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2026-10-14 16:55 UTC (permalink / raw)
To: Igor Wiedler; +Cc: git, ps, Jeff King
Igor Wiedler <iwiedler@gitlab.com> writes:
> The connectivity check spawned by check_connected() uses
>
> git rev-list --objects --stdin --not --all --quiet --alternate-refs
>
> where 39b44ba771 (check_everything_connected: assume alternate ref
> tips are valid, 2019-07-01) added "--alternate-refs" so that the walk
> from the fetched tips stops at the refs of any alternate object store,
> on the assumption that the alternate is already fully connected.
>
> That only works because "--alternate-refs" comes after "--not".
> cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
> 2018-07-02) drops "--not --all" for deepening fetches, since those
> need to verify the whole chain up to the new shallow boundary rather
> than stopping at refs we already had. For such fetches we therefore
> run
>
> git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs
Ouch! Thanks for spotting the 8-year-old mistake. It shows how
rare the deepening fetches together with the use of alternate refs
in real life?
> Move "--alternate-refs" into the non-deepening branch next to "--not
> --all", so that a deepening fetch walks the fetched tips down to the
> new shallow boundary without looking at the alternate's refs at all.
To paraphrase, we cannot trust refs from repositories we are
borrowing the objects from without taking the shallow boundaries of
these repositories into acocunt, so we ignore them.
> Negating them instead would stop the walk at the alternate's tips,
> but the reasoning of cf1e7c0770 applies to them as well: an alternate
> that is itself shallow, or a partial clone, has refs without the
> history behind them. fetch-pack sends the alternate's refs as "have"s
> in that case too, so the server omits everything behind them, and a
> check that stops at those refs does not notice that the objects
> between the alternate's boundary and the new one are missing.
But stepping back a bit, doesn't the same argument apply even when
we are a fully populated repository and making a non-deepening
fetch? We can trust "--not --all" to make the traversal to stop at
tips of our local refs and can be assured that all history behind
the tips we are fetching will still be connected, but a traversal
that stops at the tip of a shallow repository will break our
repository, and in a worse form than the shallow repository we are
borrowing from, because operations in that repository are guarded by
its shallow boundaries that we usually do not see, but operations in
our repository do not have the same shallow boundaries in effect.
I do agree that for the purpose of "see if this traversal barfs with
missing objects to see if the updated tips have all the objects
behind it", listing the alternate refs behind "--not" is the right
thing to do and it is broken to have "--alternate-refs" without
having "--not" like the current code does. But I am not convinced
that disabling --alternate-refs only when you are deepening is not
solving the problems of using alternate-refs introduces. Shouldn't
the fix be more like two (or more) steps, i.e.,
* [1/2] for check_connected(), use the attached patch instead.
* [2/2] update odb_for_each_alternate_ref() so that it ignores any
and all refs from shallow repositories.
Hmm?
connected.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git i/connected.c w/connected.c
index 7e26976832..f4b60489bf 100644
--- i/connected.c
+++ w/connected.c
@@ -91,8 +91,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
strvec_push(&rev_list.args, "--stdin");
if (repo_has_promisor_remote(the_repository))
strvec_push(&rev_list.args, "--exclude-promisor-objects");
+
+ /* define where the traversal should stop */
+ strvec_push(&rev_list.args, "--not");
if (!opt->is_deepening_fetch) {
- strvec_push(&rev_list.args, "--not");
if (opt->exclude_hidden_refs_section)
strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
opt->exclude_hidden_refs_section);
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] connected: do not walk alternate refs in deepening fetches
2026-10-14 10:35 [PATCH] connected: do not walk alternate refs in deepening fetches Igor Wiedler
2026-10-14 12:25 ` Patrick Steinhardt
2026-10-14 16:55 ` Junio C Hamano
@ 2026-10-14 17:02 ` Justin Tobler
2 siblings, 0 replies; 4+ messages in thread
From: Justin Tobler @ 2026-10-14 17:02 UTC (permalink / raw)
To: Igor Wiedler; +Cc: git, ps
On 26/10/14 12:35PM, Igor Wiedler wrote:
> The connectivity check spawned by check_connected() uses
>
> git rev-list --objects --stdin --not --all --quiet --alternate-refs
>
> where 39b44ba771 (check_everything_connected: assume alternate ref
> tips are valid, 2019-07-01) added "--alternate-refs" so that the walk
> from the fetched tips stops at the refs of any alternate object store,
> on the assumption that the alternate is already fully connected.
Ok, when performing a connectivity check, we assume that an alternate
repository is already in a valid state. By excluding alternate refs from
the git-rev-list(1), some time can potentially be saved.
> That only works because "--alternate-refs" comes after "--not".
> cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
> 2018-07-02) drops "--not --all" for deepening fetches, since those
> need to verify the whole chain up to the new shallow boundary rather
> than stopping at refs we already had. For such fetches we therefore
> run
>
> git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs
>
> with the alternate's refs as positive revisions, and rev-list
> enumerates every object reachable from them. Unless one of those refs
> happens to lie inside the newly fetched shallow range, where the
> grafts cut the walk short, that is the alternate's entire history.
Ok, so for deepening fetches, the `--not --all` is intentially removed
to allow the newly deepened commits to be verified. A consequence of
this though was that the `--alternate-refs` changed from being excluded
to included unintentially. Makes sense.
> This is slow for setups that attach a large alternate to a fresh
> repository and then do a shallow fetch of a ref that is a few commits
> ahead of the alternate, which is how CI runners share a clone of a
> big repository between jobs. With a 7M-object alternate (a bare clone
> of gitlab-org/gitlab), "git fetch --depth 1" transfers an empty pack
> in under a second and the connectivity check then takes 227s. Without
> the walk over the alternate's history the same rev-list finishes in
> 0.0s.
For large alternate repositories that contain lots of additional
history, this certainly could result in a significant slow down.
> Move "--alternate-refs" into the non-deepening branch next to "--not
> --all", so that a deepening fetch walks the fetched tips down to the
> new shallow boundary without looking at the alternate's refs at all.
> Negating them instead would stop the walk at the alternate's tips,
> but the reasoning of cf1e7c0770 applies to them as well: an alternate
> that is itself shallow, or a partial clone, has refs without the
> history behind them. fetch-pack sends the alternate's refs as "have"s
> in that case too, so the server omits everything behind them, and a
> check that stops at those refs does not notice that the objects
> between the alternate's boundary and the new one are missing. The walk
> down to the boundary is bounded by the depth, so this does not bring
> back the full history walk, and objects that exist only in the
> alternate are still found through the object store.
Naively I assumed we would still want to negate the alternate refs, but
IIUC for deepening fetches we may fetch objects that a shallow/partial
alternate repo _could_ have, but doesn't. Thus we need to perform the
object walk up to the new boundary. That makes sense to me.
> Add tests to t5618 that check the arguments of the rev-list child via
> GIT_TRACE2_EVENT for a regular and for a deepening fetch. The result
> of the check is the same either way for a complete alternate, so
> there is no behavioural difference to assert on. The difference is
> only in which objects are walked.
>
> Signed-off-by: Igor Wiedler <iwiedler@gitlab.com>
> ---
> connected.c | 2 +-
> t/t5618-alternate-refs.sh | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/connected.c b/connected.c
> index 929b9bd28d..d8bfb3dbf2 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -127,9 +127,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> strvec_pushf(&rev_list.args, "--exclude-hidden=%s",
> opt->exclude_hidden_refs_section);
> strvec_push(&rev_list.args, "--all");
> + strvec_push(&rev_list.args, "--alternate-refs");
> }
> strvec_push(&rev_list.args, "--quiet");
> - strvec_push(&rev_list.args, "--alternate-refs");
Ok, now the --alternate-refs option is only used in non-deepening
fetches. Looks good.
> if (opt->progress)
> strvec_pushf(&rev_list.args, "--progress=%s",
> _("Checking connectivity"));
> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> index 2fb6d549d3..237eaba85c 100755
> --- a/t/t5618-alternate-refs.sh
> +++ b/t/t5618-alternate-refs.sh
> @@ -58,4 +58,17 @@ test_expect_success 'log --source shows .alternate marker' '
> test_cmp expect actual
> '
>
> +test_expect_success 'connectivity check of a fetch excludes alternate refs' '
> + GIT_TRACE2_EVENT="$PWD/fetch.trace" git fetch origin &&
> + test_subcommand_flex git rev-list --objects --stdin --not --all \
> + --alternate-refs --quiet <fetch.trace
> +'
> +
> +test_expect_success 'connectivity check of a deepening fetch ignores alternate refs' '
> + GIT_TRACE2_EVENT="$PWD/deepen.trace" git fetch --depth=1 origin one &&
> + test_subcommand_flex git rev-list --objects --stdin --quiet <deepen.trace &&
> + test_subcommand_flex ! git rev-list --objects --stdin --alternate-refs \
> + <deepen.trace
> +'
> +
> test_done
Ok, the added tests here are simply validing that the spawned
git-rev-list(1) command is using the options we now expect.
Overall, this patch looks good to me.
-Justin
^ permalink raw reply [flat|nested] 4+ messages in thread