Re: [PATCH] fetch: when deepening, check connectivity fully

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2018, 19:57 UTC
Message-ID: <xmqqin64dmui.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180627173237.25620-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> Do not stop at ancestors of our refs when deepening during fetching.
> This is because when performing such a fetch, shallow entries may have
> been updated; the client can reasonably assume that the existing refs
> are connected up to the old shallow points, but not the new.

OK.

Show 17 quoted lines

> diff --git a/connected.c b/connected.c
> index 91feb78815..1bba888eff 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -58,8 +58,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  	argv_array_push(&rev_list.args, "--stdin");
>  	if (repository_format_partial_clone)
>  		argv_array_push(&rev_list.args, "--exclude-promisor-objects");
> -	argv_array_push(&rev_list.args, "--not");
> -	argv_array_push(&rev_list.args, "--all");
> +	if (!opt->is_deepening_fetch) {
> +		argv_array_push(&rev_list.args, "--not");
> +		argv_array_push(&rev_list.args, "--all");
> +	}
>  	argv_array_push(&rev_list.args, "--quiet");
>  	if (opt->progress)
>  		argv_array_pushf(&rev_list.args, "--progress=%s",

Hmph, remind me how old and/or new shallow cut-off points affect this traversal? In order to verify that everything above the new cut-off points, opt->shallow_file should be pointing at the new cut-off points, then we do the full sweep (without --not --all) to ensure we won't find missing or broken objects but still stop at the new cut-off points, and then only after check_connected() passes, update the shallow file to make new shallow cut-off points active (and if the traversal fails, then we do nto install the new shallow cut-off points)?

If so, that sounds sensible. By not having "--not --all", we potentially would do a full sweep, but if we are really deepening to the root commits, then we do need one full sweep anyway (as these deepest parts of the history were previously hidden by the shallow cutoff points), and if we are only deepening the history by a bit, even if we do not have "--not --all", we would hit the updated shallow cutoff points (which may be at older parts of the history) and stop, and for correctness we need to visit there anyway, so I think we are not being overly pessimistic with this change, either.