Re: Re: [PATCH v5] builtin/clone.c: add --reject-shallow option

"Li Linchao via GitGitGadget" [off-list ref] writes:

quoted

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)

OK，thanks for reminding this.

quoted

+--[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

@@ -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

@@ -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

@@ -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

 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.