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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2021, 05:50 UTC
Message-ID: <xmqq35yc9yan.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.865.git.1612409491842.gitgitgadget@gmail.com>

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

> From: lilinchao <lilinchao@oschina.cn>
>
> This patch add a new option that reject to clone a shallow repository.

A canonical form of our log message starts by explaining the need, and then presents the solution at the end.

> Clients don't know it's a shallow repository until they download it
> locally, in some scenariors, clients just don't want to clone this kind

"scenarios". "in some scenarios" would have to be clarified a bit more to justify why it is a good idea to have such a feature.

> of repository, and want to exit the process immediately without creating
> any unnecessary files.

"clients don't know it's a shallow repository until they download" leading to "so let's reject immediately upon finding out that they are shallow" does make sense as a flow of thought, though.

> +--no-shallow::
> +	Don't clone a shallow source repository. In some scenariors, clients

"scenarios" (no 'r').

> diff --git a/builtin/clone.c b/builtin/clone.c
> old mode 100644
> new mode 100755

Unwarranted "chmod +x"; accidents do happen, but please be careful before making what you did public ;-)

Show 8 quoted lines

> @@ -90,6 +91,7 @@ static struct option builtin_clone_options[] = {
>  	OPT__VERBOSITY(&option_verbosity),
>  	OPT_BOOL(0, "progress", &option_progress,
>  		 N_("force progress reporting")),
> +	OPT_BOOL(0, "no-shallow", &option_no_shallow, N_("don't clone shallow repository")),
>  	OPT_BOOL('n', "no-checkout", &option_no_checkout,
>  		 N_("don't create a checkout")),
>  	OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),

It is a bad idea to give a name that begins with "no-" to an option whose default can be tweaked by a configuration variable [*]. If the configuration is named "rejectshallow", perhaps it is better to call it "--reject-shallow" instead.

This is because configured default must be overridable from the command line. I.e. even if you have in your ~/.gitconfig this:

    [clone]
        rejectshallow = true

you should be able to say "allow it only this time", with

    $ git clone --no-reject-shallow http://github.com/git/git/ git

and you do not want to have to say "--no-no-shallow", which sounds just silly.

	Side note. it is a bad idea in general, even if the option
	does not have corresponding configuration variable.  The
	existing "no-checkout" is a historical accident that
	happened long time ago and cannot be removed due to
	compatibility.  Let's not introduce a new option that
	follows such a bad pattern.

Show 16 quoted lines

> @@ -963,6 +968,7 @@ static int path_exists(const char *path)
>  int cmd_clone(int argc, const char **argv, const char *prefix)
>  {
>  	int is_bundle = 0, is_local;
> +	int is_shallow = 0;
>  	const char *repo_name, *repo, *work_tree, *git_dir;
>  	char *path, *dir, *display_repo = NULL;
>  	int dest_exists, real_dest_exists = 0;
> @@ -1215,6 +1221,7 @@ 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)) {
> +			is_shallow = 1;
>  			if (option_local > 0)
>  				warning(_("source repository is shallow, ignoring --local"));
>  			is_local = 0;

This change is to the local clone codepath. Cloning over the wire would not go through this part. And throughout the patch, this is the only place that sets is_shallow to 1.

Also let's note that this is after we called parse_options(), so the value of option_no_shallow is known at this point.

So, this patch does not even *need* to introduce a new "is_shallow" variable at all. It only needs to add

                        if (option_no_shallow)
                                die(...);

instead of adding "is_shallow = 1" to the above hunk.

I somehow think that this is only half a feature---wouldn't it be more useful if we also rejected a non-local clone from a shallow repository?

And for that ...

Show 15 quoted lines

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 7f082fb23b6a..9d310dbb158a 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -42,6 +42,13 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>  
>  '
>  
> +test_expect_success 'reject clone shallow repository' '
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	test_must_fail git clone --no-shallow shallow-repo out 2>err &&
> +	test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
> +'
> +

... in addition to the test for a local clone above, you'd also want to test a non-local clone, perhaps like so:

test_expect_success 'reject clone shallow repository' '
	rm -fr shallow-repo &&
	git clone --depth=1 --no-local parent shallow-repo &&
	test_must_fail git clone --no-shallow --no-local shallow-repo out 2>err &&
	test_i18ngrep -e "source repository is shallow, reject to clone." err

Ditto for the other test script.

Also, you would want to make sure that the command line overrides the configured default. I.e.

	git -c clone.rejectshallow=false clone --reject-shallow

should refuse to clone from a shallow one, while there should be a way to countermand a configured "I always refuse to clone from a shallow repository" with "but let's allow it only this time", i.e.

	git -c clone.rejectshallow=true clone --no-reject-shallow

or something along the line.

Show 12 quoted lines

> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 8e0fd398236b..3aab86ad4def 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -92,6 +92,13 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'clone -c clone.rejectshallow' '
> +	rm -rf child &&
> +	git clone --depth=1 --no-local . child &&
> +	test_must_fail git clone -c clone.rejectshallow child out 2>err &&

This is not quite right, even though it may happen to work. The "clone.rejectshallow" variable is a configuration about what should happen when creating a new repository by cloning, so letting "git clone -c var[=val]" to set the variable _in_ the resulting repository would not make much sense. Even if the clone succeeded, nobody would look at that particular configuration variable that is set in the resulting repository.

I think it would communicate to the readers better what we are trying to do, if we write

	test_must_fail git -c clone.rejectshallow=true clone child out

instead.

Thanks.