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

[Junio C Hamano <gitster@pobox.com>]

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