Re: [PATCH] upload-pack: reject shallow requests that would return nothing

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2018, 05:55 UTC
Message-ID: <xmqqy3g4jpck.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180526113518.22403-1-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> To avoid this, if rev-list returns nothing, we abort the clone/fetch.
> The user could adjust their request (e.g. --shallow-since further back
> in the past) and retry.

Yeah, that makes sense.

> Another possible option for this case is to fall back to a default
> depth (like depth 1). But I don't like too much magic that way because
> we may return something unexpected to the user.

I agree that it would be a horrible fallback. I actually am wondering if we should just silently return no objects without even telling the user there is something unexpected happening. After all, the user may well be expecting with --shallow-since that is too recent that the fetch may not result in pulling anything new, and giving a "die" message, which now needs to be distinguished from other forms of die's like network connectivity or auth failures, is not all that helpful.

Show 6 quoted lines

> Note that we need to die() in get_shallow_commits_by_rev_list()
> instead of just checking for empty result from its caller
> deepen_by_rev_list() and handling the error there. The reason is,
> empty result could be a valid case: if you have commits in year 2013
> and you request --shallow-since=year.2000 then you should get a full
> clone (i.e. empty result).

Yup, that latter example makes me more convinced that it also is a valid outcome if we end up requesting "no" object when shallow-since names too recent date, e.g. against a project that is dormant since 2013 with --shallow-since=2018/01/01 or something like that, instead of dying.

Show 43 quoted lines

> Reported-by: Andreas Krey <a.krey@gmx.de>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  shallow.c             |  3 +++
>  t/t5500-fetch-pack.sh | 11 +++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/shallow.c b/shallow.c
> index df4d44ea7a..44fdca1ace 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -175,6 +175,9 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
>  		die("revision walk setup failed");
>  	traverse_commit_list(&revs, show_commit, NULL, &not_shallow_list);
>  
> +	if (!not_shallow_list)
> +		die("no commits selected for shallow requests");
> +
>  	/* Mark all reachable commits as NOT_SHALLOW */
>  	for (p = not_shallow_list; p; p = p->next)
>  		p->item->object.flags |= not_shallow_flag;
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 0680dec808..c8f2d38a58 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -711,6 +711,17 @@ test_expect_success 'fetch shallow since ...' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'clone shallow since selects no commits' '
> +	test_create_repo shallow-since-the-future &&
> +	(
> +	cd shallow-since-the-future &&
> +	GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
> +	GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
> +	GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&
> +	test_must_fail git clone --shallow-since "900000000 +0700" "file://$(pwd)/." ../shallow111
> +	)
> +'
> +
>  test_expect_success 'shallow clone exclude tag two' '
>  	test_create_repo shallow-exclude &&
>  	(