Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
flat view
From: Junio C Hamano <hidden>
Date: 2021-03-04 00:26:53
"Li Linchao via GitGitGadget" [off-list ref] writes:
quoted hunk
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/ (local)
quoted hunk
+--[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 ...
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
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.