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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2021, 23:21 UTC
Message-ID: <xmqq35xbj01p.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.865.v5.git.1614535588332.gitgitgadget@gmail.com>

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

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

Show 7 quoted lines

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

Show 14 quoted lines

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

Show 6 quoted lines

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

Show 12 quoted lines

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