From: Junio C Hamano Date: Wed, 14 Oct 2026 16:55:49 GMT Subject: Re: [PATCH] connected: do not walk alternate refs in deepening fetches Message-ID: In-Reply-To: <20261014103507.35317-1-iwiedler@gitlab.com> Igor Wiedler 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);