Re: [PATCH v7] builtin/clone.c: add --reject-shallow option

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2021, 22:57 UTC
Message-ID: <xmqqmtuqu9io.fsf@gitster.g>
In-Reply-To: <pull.865.v7.git.1616670558261.gitgitgadget@gmail.com>

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> @@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		if (filter_options.choice)
>  			warning(_("--filter is ignored in local clones; use file:// instead."));
>  		if (!access(mkpath("%s/shallow", path), F_OK)) {
> +			if (reject_shallow)
> +				die(_("source repository is shallow, reject to clone."));
> +			else
> +				warning(_("source repository is shallow."));

Hmph, is it an improvement to warn() when the user does not mind cloning a shallow repository?

	$ git clone --depth=3 $URL clone-1
	$ git clone file://$(pwd)/clone-1 clone-2

would give us clone-2 that is just as functional as clone-1 is, no? clone-1 may be missing objects that is needed far into the past, and clone-2 would lack the same set of objects as clone-1 does, but a user who is happily using clone-1 would be happy with clone-2 the same way, no?

Show 12 quoted lines

> diff --git a/fetch-pack.c b/fetch-pack.c
> index fb04a76ca263..72b378449a07 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1129,9 +1129,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	if (args->deepen)
>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>  					NULL);
> -	else if (si->nr_ours || si->nr_theirs)
> +	else if (si->nr_ours || si->nr_theirs) {
> +		if (args->remote_shallow)
> +			die(_("source repository is shallow, reject to clone."));

Stopping early before calling get_pack() would significantly reduce the overhead, which is good.

> +		else
> +			warning(_("source repository is shallow."));

The same question on the wisdom of warning here.

Show 10 quoted lines

> @@ -1498,10 +1502,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>  		 * rejected (unless --update-shallow is set); do the same.
>  		 */
>  		prepare_shallow_info(si, shallows);
> -		if (si->nr_ours || si->nr_theirs)
> +		if (si->nr_ours || si->nr_theirs) {
> +			if (args->remote_shallow)
> +				die(_("source repository is shallow, reject to clone."));
> +			else
> +				warning(_("source repository is shallow."));

OK, so, this is the equivalent of the above for protocol-v2? The same comments apply, then.

Show 18 quoted lines

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 428b0aac93fa..2863b8b28d44 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
>  
>  test_expect_success 'setup' '
>  
> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>  
>  '
>  
> +test_expect_success 'fail to clone http shallow repository' '

s/fail to clone/reject cloning/, perhaps.

> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
> +	rm -rf shallow-repo &&
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	git clone --no-reject-shallow --no-local shallow-repo clone-repo

OK. Also without "--no-reject-shallow" option, the command would successfully clone from the shallow-repo, I presume?

The changes look more-or-less good to me, except for the "warning()" bit, which I do not think is a good idea.