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

--------------
lilinchao@oschina.cn

[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" [off-list ref] writes:

quoted

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

quoted

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

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

Uh, IMO the goal of this new option is not to make sure the cloned repo
is not shallow, but to prevent(just as optional) the remote repo is shallow, 
we still allow the resulting repo is shallow by using "--depth" option.
so, if we apply "clone --depth=1 --reject-shallow=true" to a clone process,
the expected result is a shallow repo.
Oh, wait, what if we apply "--depth=1" to a remote shallow repo, in other word,
shallow a remote shallow repo? then the result will not be what we expected.
This can be confusing.

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. 

I took the time to update the patch as you suggested there, it may look
imperfect, I only tested the local protocol, and the smart protocols,
not dumb protocol, and not protocol version1 yet. 
Hope to get some suggestions from you.