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

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

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/

Let's camelCase the reference to the variable, too.  Also, typeset
in monospace, i.e.

	The `clone.rejectShallow` configuration variable ...

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.

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.

With the local transport, it (hopefully) is trivial to see if the
source is shallow.  OK.

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.

"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/

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.