>"Li Linchao via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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. >
