Re: Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
- From
- lilinchao@oschina.cn <lilinchao@oschina.cn>
- Date
- Mar 26, 2021, 03:49 UTC
- Message-ID
- <552b26168de611eb8af90024e87935e7@oschina.cn>
- In-Reply-To
- <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>
Thanks for your review.
-------------- lilinchao@oschina.cn
Show 14 quoted lines
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -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?
> Uh, the idea to warn comes from previous comments I wrote 25 days ago:
"and maybe we could warn client in fetch-pack stage, if we don't choose to reject shallow cloning."
then no one response to that point, so I think is ok to apply it. Now, it seems like a bad idea. I will remove it.
Show 43 quoted lines
> $ 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?
>
>> 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.
>
>> @@ -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. Yes, this is for protocol v2.
Show 21 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.
Ok, will do.
Show 8 quoted lines
> >> +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?
Yes, exactly.
> >The changes look more-or-less good to me, except for the "warning()" >bit, which I do not think is a good idea.
I will remove the unnecessary warning() part.
Thanks!