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

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

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

> From: lilinchao <lilinchao@oschina.cn>

This is not corrected, and no longer matches your sign-off.

I can edit your 'From:' to match this time, but please make sure
they match for the next time.

> In some scenarios, users may want more history than the repository
> offered for cloning, which happens to be a shallow repository, can
> give them. But because users don't know it is a shallow repository
> until they download it to local, users should have the option to

I find the "should" too strong, but let's keep reading.

> refuse to clone this kind of repository, and may want to exit the
> process immediately without creating any unnecessary files.

Yes, that is too strong and also redundant.

> Althought there is an option '--depth=x' for users to decide how
> deep history they can fetch, but as the unshallow cloning's depth

"Although"; if you begin with "although", you shouldn't write "but".

> is INFINITY, we can't know exactly the minimun 'x' value that can
> satisfy the minimum integrity,
> so we can't pass 'x' value to --depth,
> and expect this can obtain a complete history of a repository.

If the argument were "we might start with depth x, and the source
may be deep enough to give us x right now, but we may want to deepen
more than they can offer, and such a user would want to be able to
say 'I want depth=x now, but make sure they are not shallow'", I
would understand it, but I do not see where the "minimum integrity"
comes from---there doesn't appear to be anything related to
integrity here.

> In other scenarios, if we have an API that allow us to import external

"allows"

> repository, and then perform various operations on the repo.
> But if the imported is a shallow one(which is actually possible), it
> will affect the subsequent operations. So we can choose to refuse to
> clone, and let's just import a normal repository.

I'd suggest dropping this entire paragraph.  That is not any new
scenario at all.  API or not, you essentially just said "you can do
various things on your clone once you have it, but some things you
may want to do you would want a full history".  That is what you
started the whole discussion above and does not add any new
information.

> This patch offers a new option '--reject-shallow' that can reject to
> clone a shallow repository.

	Teach '--reject-shallow' option to "git clone" to abort as
	soon as we find out that we are cloning from a shallow
	repository.

perhaps?  cf. Documentation/SubmittingPatches[[describe-changes]]
especially [[imperative-mood]].


> Signed-off-by: Li Linchao <lilinchao@oschina.cn>
> ---
>     builtin/clone.c: add --reject-shallow option
>     
>     Changes since v1:
>...
>     Changes since v6:
>     
>      * use _() for warning/die statement

What are the more recent changes?

>     Signed-off-by: lilinchao lilinchao@oschina.cn

> @@ -858,6 +861,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") && option_reject_shallow < 0)
> +		option_reject_shallow = git_config_bool(k, v);

Does this "single variable is enough" really work?

Imagine that the first time around we'd read from $HOME/.gitconfig
that says true (flips the variable from "unspecified").  Further
imagine that we are running "git clone -c config.rejectShallow=no"
to countermand the global config.  We call write_config() to write
the extra configuration value out, and then call git_config() to
read from the repository configuration again.

Because of the value taken from $HOME/.gitconfig, however, the
attempt to override is silently ignored, isn't it?

Other than that, the changes to the code from the previous round
looked sensible.