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

From: Junio C Hamano <[email-masked]>
Date: Mon, 22 Feb 2021 10:12:33 -0800

[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" <[email-masked]> writes:

> @@ -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.