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

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

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