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 v5] builtin/clone.c: add --reject-shallow option
Date: Thu, 4 Mar 2021 13:50:27 +0800 [thread overview]
Message-ID: <880ae0707cad11eb8ccc0024e87935e7@oschina.cn> (raw)
In-Reply-To: xmqq35xbj01p.fsf@gitster.c.googlers.com
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>> index 47de36a5fedf..50ebc170bb81 100644
>> --- a/Documentation/config/clone.txt
>> +++ b/Documentation/config/clone.txt
>> @@ -2,3 +2,7 @@ clone.defaultRemoteName::
>> The name of the remote to create when cloning a repository. Defaults to
>> `origin`, and can be overridden by passing the `--origin` command-line
>> option to linkgit:git-clone[1].
>> +
>> +clone.rejectshallow::
>> + Reject to clone a repository if it is a shallow one, can be overridden by
>> + passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>
>Let's camelCase this, i.e. "clone.rejectShallow", as this file would
>be a good candidate to be the authoritative record of canonical
>spelling of these variables.
>
>cf. https://lore.kernel.org/git/xmqq7dmy389l.fsf@gitster.g/
OK,thanks for reminding this.
>
>> +--[no-]reject-shallow::
>> + Fail if the source repository is a shallow repository. The
>> + 'clone.rejectshallow' configuration variable can be used to
>> + give the default.
>
>Let's camelCase the reference to the variable, too. Also, typeset
>in monospace, i.e.
>
> The `clone.rejectShallow` configuration variable ...
>
>> @@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
>> free(remote_name);
>> remote_name = xstrdup(v);
>> }
>> + if (!strcmp(k, "clone.rejectshallow")) {
>> + config_shallow = git_config_bool(k, v);
>> + }
>
>No need to use {} around a single-statement block, especially when
>the "if" statement does not have an "else" block.
>
>The use of strcmp() against the variable name in all lowercase is
>correct here.
>
>> @@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> */
>> git_config(git_clone_config, NULL);
>>
>> + /*
>> + * If option_shallow is specified from CLI option,
>> + * ignore config_shallow from git_clone_config.
>> + */
>> + if (config_shallow != -1) {
>> + reject_shallow = config_shallow;
>> + }
>> + if (option_shallow != -1) {
>> + reject_shallow = option_shallow;
>> + }
>
>Needless use of {} around single-statement blocks.
>
>As reject_shallow is initialized to 0, this lets the option to be of
>the most priority, then the config (presumably coming from the per-user
>or per-system configuration), by without them, defaults to false. Good.
>
>We'll have an extra git_config() call later, but that one will only
>read into config_shallow, never to be looked at because we will use
>reject_shallow variable anyway. OK.
>
>> @@ -1216,6 +1234,8 @@ 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.");
>
>With the local transport, it (hopefully) is trivial to see if the
>source is shallow. OK.
>
>> if (option_local > 0)
>> warning(_("source repository is shallow, ignoring --local"));
>> is_local = 0;
>> @@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>>
>> + if (reject_shallow)
>> + transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>> if (option_depth)
>> transport_set_option(transport, TRANS_OPT_DEPTH,
>> option_depth);
>
>OK. What is really interesting will all happen inside the transport
>layer; the caller only has to ask for it.
>
>The asymmetry with other options like "--depth" stands out and
>puzzles readers, though.
>
True, but since `reject_shallow` is not just an option from CLI, so I think
we can't just call it "option_shallow" here to order to keep symmetry
with other options below.
>Do we really want to add the clone.rejectShallow configuration?
Only when I want all my git clone in my machine can avoid cloning
shallow repo, and no need to provide the option "--reject-shallow"
every time, then this configuration would make sense.
>After all, we do not give "clone.depth = 1" etc., and that is the
>reason why we only need "if (option_depth)" here in the near-by
>code.
>
>I'd stop here for today, hoping that somebody much more familiar
>with the transport layer than I am will review and comment on the
>changes there.
>
>Thanks.
>
next prev parent reply other threads:[~2021-03-04 6:17 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 [this message]
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
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=880ae0707cad11eb8ccc0024e87935e7@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.
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).