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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2021, 18:12 UTC
Message-ID: <xmqq35xo7yzy.fsf@gitster.g>
In-Reply-To: <pull.865.v4.git.1613891147977.gitgitgadget@gmail.com>

[jc: I've CC'ed Jonathan Tan, who is much more knowledgeable than I am on the transport layer issues, to sanity check my assumption]

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

Show 6 quoted lines

> @@ -1363,6 +1384,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			goto cleanup;
>  	}
>  
> +	if (reject_shallow) {
> +		if (local_shallow || is_repository_shallow(the_repository)) {

This may reject to clone from a shallow repository, but at this point the bulk of the tranfer from the origin repository has already happened, no? Rejecting after transferring many megabytes feels a bit too late. That is one of the reasons why I kept hinting that the transport layer needs to be taught an option to reject talking to a shallow counterpart if we want to add this feature [*1*].

Also, wouldn't "clone --depth=1 --reject-shallow" from a repository that is not shallow make the_repository a shallow one at this point and makes it fail? If the goal of the --reject-shallow option were to make sure the resulting repository is not shallow, then that is a technically correct implementation (even though it is wasteful to transfer a full tree worth of megabytes and then abort), but is the feature is explained to reject cloning from a shallow one, then users would be suprised to see ...

> +			die(_("source repository is shallow, reject to clone."));

... this message, when cloning from well known publich repositories that are not shallow.

I think cloning with --depth=<n> when the source repository is deep enough, should be allowed, so the cleanest solution for the latter may be to notice the combination of options that make the resulting repository shallow (I mentioned --depth=<n>, but there may be others) and the --reject-shallow option and error out before even talking to the other side at the time we parse the command line.

Thanks.

[Footnote]

*1* Looking at Documentation/technical/pack-protocol.txt, "git
    fetch" seem to learn if the repository is shallow immediately
    upon contacting "upload-pack" during the Reference Discovery
    phase (we'd see 'shallow' packets if they are shallow). I
    suspect that the right solution would be to teach the codepath
    on the "git fetch" side that accepts, parses, and acts on this
    packet to optionally stop communication and error out when the
    caller asks not to talk with a shallow repository.