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>
Subject: Re: Re: [PATCH] builtin/clone.c: add --no-shallow option
Date: Thu, 4 Feb 2021 18:32:52 +0800 [thread overview]
Message-ID: <57d5526c66d411eb81800024e87935e7@oschina.cn> (raw)
In-Reply-To: xmqq35yc9yan.fsf@gitster.c.googlers.com
--------------
lilinchao@oschina.cn
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: lilinchao <lilinchao@oschina.cn>
>>
>> This patch add a new option that reject to clone a shallow repository.
>
>A canonical form of our log message starts by explaining the need,
>and then presents the solution at the end.
Ok, will do.
>
>> Clients don't know it's a shallow repository until they download it
>> locally, in some scenariors, clients just don't want to clone this kind
>
>"scenarios". "in some scenarios" would have to be clarified a bit
>more to justify why it is a good idea to have such a feature.
I found an issue described like this:
The blame information can be completely wrong when fetching it from
a shallow clone, without errors or warnings. When the outcome is invalid
data, it's extremely difficult to diagnose that it comes from a shallow clone.
If a line in a file was not changed in the commits that were downloaded as
part of the shallow fetch, git will report the first known commit as the author.
This has a big impact on the auto-assignment of new issues.
It looks like this is another scenario that can prove this feature is necessary.
>
>> of repository, and want to exit the process immediately without creating
>> any unnecessary files.
>
>"clients don't know it's a shallow repository until they download"
>leading to "so let's reject immediately upon finding out that they
>are shallow" does make sense as a flow of thought, though.
>
>> +--no-shallow::
>> + Don't clone a shallow source repository. In some scenariors, clients
>
>"scenarios" (no 'r').
>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> old mode 100644
>> new mode 100755
>
>Unwarranted "chmod +x"; accidents do happen, but please be careful
>before making what you did public ;-)
Oops, this happened when I edited it in VS Code, it noticed me 'permission denied' when
I want to save the file.
>
>> @@ -90,6 +91,7 @@ static struct option builtin_clone_options[] = {
>> OPT__VERBOSITY(&option_verbosity),
>> OPT_BOOL(0, "progress", &option_progress,
>> N_("force progress reporting")),
>> + OPT_BOOL(0, "no-shallow", &option_no_shallow, N_("don't clone shallow repository")),
>> OPT_BOOL('n', "no-checkout", &option_no_checkout,
>> N_("don't create a checkout")),
>> OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
>
>It is a bad idea to give a name that begins with "no-" to an option
>whose default can be tweaked by a configuration variable [*]. If
>the configuration is named "rejectshallow", perhaps it is better to
>call it "--reject-shallow" instead.
>
>This is because configured default must be overridable from the
>command line. I.e. even if you have in your ~/.gitconfig this:
>
> [clone]
> rejectshallow = true
>
>you should be able to say "allow it only this time", with
>
> $ git clone --no-reject-shallow http://github.com/git/git/ git
>
>and you do not want to have to say "--no-no-shallow", which sounds
>just silly.
>
> Side note. it is a bad idea in general, even if the option
> does not have corresponding configuration variable. The
> existing "no-checkout" is a historical accident that
> happened long time ago and cannot be removed due to
> compatibility. Let's not introduce a new option that
> follows such a bad pattern.
>
You're right, "--reject-shallow" is much better.
I didn't realize that bool options have default [no-] option.
>> @@ -963,6 +968,7 @@ static int path_exists(const char *path)
>> int cmd_clone(int argc, const char **argv, const char *prefix)
>> {
>> int is_bundle = 0, is_local;
>> + int is_shallow = 0;
>> const char *repo_name, *repo, *work_tree, *git_dir;
>> char *path, *dir, *display_repo = NULL;
>> int dest_exists, real_dest_exists = 0;
>> @@ -1215,6 +1221,7 @@ 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)) {
>> + is_shallow = 1;
>> if (option_local > 0)
>> warning(_("source repository is shallow, ignoring --local"));
>> is_local = 0;
>
>This change is to the local clone codepath. Cloning over the wire
>would not go through this part. And throughout the patch, this is
>the only place that sets is_shallow to 1.
>
>Also let's note that this is after we called parse_options(), so the
>value of option_no_shallow is known at this point.
>
>So, this patch does not even *need* to introduce a new "is_shallow"
>variable at all. It only needs to add
>
> if (option_no_shallow)
> die(...);
>
>instead of adding "is_shallow = 1" to the above hunk.
>
>I somehow think that this is only half a feature---wouldn't it be
>more useful if we also rejected a non-local clone from a shallow
>repository?
>
>And for that ...
>
After I applied your review suggestions above, then we can reject a
non-local clone from shallow repo. For now, it will clone a empty
repo with --no-local option.
>
>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index 7f082fb23b6a..9d310dbb158a 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -42,6 +42,13 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>>
>> '
>>
>> +test_expect_success 'reject clone shallow repository' '
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + test_must_fail git clone --no-shallow shallow-repo out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +
>> +'
>> +
>
>... in addition to the test for a local clone above, you'd also want
>to test a non-local clone, perhaps like so:
>
>test_expect_success 'reject clone shallow repository' '
> rm -fr shallow-repo &&
> git clone --depth=1 --no-local parent shallow-repo &&
> test_must_fail git clone --no-shallow --no-local shallow-repo out 2>err &&
> test_i18ngrep -e "source repository is shallow, reject to clone." err
>
>'
>
>Ditto for the other test script.
>
>Also, you would want to make sure that the command line overrides
>the configured default. I.e.
>
> git -c clone.rejectshallow=false clone --reject-shallow
>
>should refuse to clone from a shallow one, while there should be a
>way to countermand a configured "I always refuse to clone from a
>shallow repository" with "but let's allow it only this time", i.e.
>
> git -c clone.rejectshallow=true clone --no-reject-shallow
>
>or something along the line.
>
>
>> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
>> index 8e0fd398236b..3aab86ad4def 100755
>> --- a/t/t5611-clone-config.sh
>> +++ b/t/t5611-clone-config.sh
>> @@ -92,6 +92,13 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'clone -c clone.rejectshallow' '
>> + rm -rf child &&
>> + git clone --depth=1 --no-local . child &&
>> + test_must_fail git clone -c clone.rejectshallow child out 2>err &&
>
>This is not quite right, even though it may happen to work. The
>"clone.rejectshallow" variable is a configuration about what should
>happen when creating a new repository by cloning, so letting "git
>clone -c var[=val]" to set the variable _in_ the resulting repository
>would not make much sense. Even if the clone succeeded, nobody would
>look at that particular configuration variable that is set in the
>resulting repository.
>
>I think it would communicate to the readers better what we are
>trying to do, if we write
>
> test_must_fail git -c clone.rejectshallow=true clone child out
>
>instead.
>
>Thanks.
Thank you for so many effective suggestions, I will write test case more carefully :)
next prev parent reply other threads:[~2021-02-04 11:07 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 [this message]
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
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57d5526c66d411eb81800024e87935e7@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=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.
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 NNTP newsgroup(s).