From: "lilinchao@oschina.cn" <lilinchao@oschina.cn>
To: "Junio C Hamano" <gitster@pobox.com>,
"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git <git@vger.kernel.org>, "Derrick Stolee" <stolee@gmail.com>,
dscho <johannes.schindelin@gmx.de>,
"Jonathan Tan" <jonathantanmy@google.com>
Subject: Re: Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
Date: Fri, 26 Mar 2021 11:49:52 +0800 [thread overview]
Message-ID: <552b26168de611eb8af90024e87935e7@oschina.cn> (raw)
In-Reply-To: 7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com
Thanks for your review.
--------------
lilinchao@oschina.cn
>"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.
> $ 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.
>
>> 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.
>
>> +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!
next prev parent reply other threads:[~2021-03-26 3:51 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-04 3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
2021-02-04 5:50 ` Junio C Hamano
2021-02-04 10:32 ` lilinchao
2021-02-04 18:36 ` Junio C Hamano
2021-02-04 14:00 ` Johannes Schindelin
2021-02-04 18:24 ` Junio C Hamano
2021-02-08 8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
2021-02-08 8:31 ` [PATCH v2 1/2] " lilinchao via GitGitGadget
2021-02-08 8:31 ` [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option lilinchao via GitGitGadget
2021-02-08 13:33 ` [PATCH v2 0/2] builtin/clone.c: add --no-shallow option Derrick Stolee
[not found] ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
2021-02-08 13:48 ` lilinchao
2021-02-08 14:12 ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
2021-02-09 20:32 ` Junio C Hamano
[not found] ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
2021-02-10 9:07 ` lilinchao
2021-02-10 16:27 ` Junio C Hamano
[not found] ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
2021-02-20 10:40 ` lilinchao
2021-02-21 7:05 ` [PATCH v4] " Li Linchao via GitGitGadget
2021-02-22 18:12 ` Junio C Hamano
2021-03-01 22:03 ` Jonathan Tan
2021-03-01 22:34 ` Junio C Hamano
2021-03-02 8:44 ` lilinchao
2021-03-03 23:59 ` Junio C Hamano
2021-03-04 1:53 ` Jonathan Tan
[not found] ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
2021-02-28 17:58 ` lilinchao
2021-02-28 18:06 ` [PATCH v5] " Li Linchao via GitGitGadget
2021-03-01 7:11 ` lilinchao
2021-03-01 22:40 ` Johannes Schindelin
2021-03-04 6:26 ` lilinchao
2021-03-03 23:21 ` Junio C Hamano
2021-03-04 5:50 ` lilinchao
2021-03-04 17:19 ` [PATCH v6] " Li Linchao via GitGitGadget
2021-03-12 18:25 ` lilinchao
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
2021-03-25 20:31 ` Junio C Hamano
2021-03-25 22:57 ` Junio C Hamano
[not found] ` <19c9dc128da911ebacc7d4ae5278bc1233465@pobox.com>
2021-03-26 3:34 ` lilinchao
[not found] ` <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>
2021-03-26 3:49 ` lilinchao [this message]
2021-03-29 10:19 ` [PATCH v8] " Li Linchao via GitGitGadget
2021-03-29 21:36 ` Junio C Hamano
2021-03-30 9:54 ` Johannes Schindelin
2021-03-30 17:46 ` Junio C Hamano
2021-03-31 13:30 ` Johannes Schindelin
[not found] ` <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>
2021-03-31 11:03 ` lilinchao
2021-03-31 15:51 ` [PATCH v9] " lilinchao via GitGitGadget
2021-03-31 19:14 ` Junio C Hamano
2021-03-31 22:24 ` Johannes Schindelin
2021-03-31 22:37 ` Junio C Hamano
2021-04-01 10:46 ` [PATCH v10] " Li Linchao via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=552b26168de611eb8af90024e87935e7@oschina.cn \
--to=lilinchao@oschina.cn \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=jonathantanmy@google.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).