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

[Junio C Hamano <gitster@pobox.com>]

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