"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/ > +--[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. Do we really want to add the clone.rejectShallow configuration? 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.
