Re: Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
- From
- lilinchao@oschina.cn <lilinchao@oschina.cn>
- Date
- Feb 28, 2021, 17:58 UTC
- Message-ID
- <905d20d879ee11eb9c1e0024e87935e7@oschina.cn>
- In-Reply-To
- <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
-------------- lilinchao@oschina.cn
Show 33 quoted lines
>[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:
>
>> @@ -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.
> 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.
Show 20 quoted lines
>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.