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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Mon, 29 Mar 2021, Li Linchao via GitGitGadget wrote:

> From: lilinchao <lilinchao@oschina.cn>

I see "Li Linchao" in the email, but "lilinchao" in the author
information. Maybe you want to align them? Or maybe even use Unicode to
write your non-Latinized name?

> 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
> refuse to clone this kind of repository, and may want to exit the
> process immediately without creating any unnecessary files.
>
> Althought there is an option '--depth=x' for users to decide how
> deep history they can fetch, but as the unshallow cloning's depth
> 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.
>
> In other scenarios, if we have an API that allow us to import external
> 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.
>
> This patch offers a new option '--reject-shallow' that can reject to
> clone a shallow repository.

Good.

I like most of the patch, and will only point out a couple of things that
I think can be improved even further.

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 02d9c19cec75..0adc98fa7eee 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
>  --no-checkout::
>  	No checkout of HEAD is performed after the clone is complete.
>
> +--[no-]reject-shallow::
> +	Fail if the source repository is a shallow repository.
> +	The 'clone.rejectShallow' configuration variable can be used to
> +	give the default.

I am not a native speaker, either, but I believe that it would "roll off
the tongue" a bit better to say "to specify the default".

> +
>  --bare::
>  	Make a 'bare' Git repository.  That is, instead of
>  	creating `<directory>` and placing the administrative
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 51e844a2de0a..eeddd68a51f4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
>  static int option_local = -1, option_no_hardlinks, option_shared;
>  static int option_no_tags;
>  static int option_shallow_submodules;
> +static int option_shallow = -1;    /* unspecified */
> +static int config_shallow = -1;    /* unspecified */

I would much prefer those variable names to include an indicator that this
is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.

Also, I think that we can do with just a single `option_reject_shallow`
(we do not even need that `reject_shallow` variable in `cmd_clone()`):

- in `git_clone_config()`, only override it if it is still unspecified:

	if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
		option_reject_shallow = git_config_bool(k,v);

- in `cmd_clone()`, test for a _positive_ value:

	if (option_reject_shallow > 0)
		die(_("source repository is shallow, reject to clone."));

  and

	if (option_reject_shallow > 0)
 		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");

One thing to note (in the commit message, would be my preference) is that
`cmd_clone()` is _particular_ in that it runs `git_config()` _twice_. Once
before the command-line options are parsed, and once after the new Git
repository has been initialized. Note that my suggestion still works with
that: if either the original config, or the new config set
`clone.rejectShallow`, it is picked up correctly, with the latter
overriding the former if both configs want to set it.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index fb04a76ca263..34d0c2896e2e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	if (args->deepen)
>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>  					NULL);
> -	else if (si->nr_ours || si->nr_theirs)
> +	else if (si->nr_ours || si->nr_theirs) {
> +		if (args->remote_shallow)

Even as a non-casual reader, this name `remote_shallow` leads me to assume
incorrect things. This option is not about wanting a remote shallow
repository, it is about rejecting a remote shallow repository.

Please name this attribute `reject_shallow` instead of `remote_shallow`.
That will prevent future puzzlement.

> +			die(_("source repository is shallow, reject to clone."));
>  		alternate_shallow_file = setup_temporary_shallow(si->shallow);
> -	else
> +	} else
>  		alternate_shallow_file = NULL;
>  	if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
>  		     &gitmodules_oids))
> [...]
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 428b0aac93fa..de1cd85983ed 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd

That's not good. What happens if there is no `httpd`? Then the rest of the
tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
fail. The failure is intentional, but the skipping is not. There are many
tests in t5606 that do not require a running HTTP daemon, and we should
not skip them (for example, in our CI runs, there are quite a few jobs
that run without any working `httpd`).

A much better alternative, I think, would be to move those new test cases
that require `httpd` to be running to t5601 (which _already_ calls
`start_httpd`, near the end, so as to not skip any tests that do not
require `httpd`).

>
>  test_expect_success 'setup' '
>
> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
>  '
>
> +test_expect_success 'reject cloning http shallow repository' '
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +	test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
> +	test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
> +'
> +
> +test_expect_success 'reject cloning shallow repository' '
> +	rm -rf shallow-repo &&

Should this line not come immediately after the bare clone into
<DOCUMENT_ROOT>/repo.git? Or even better, as a `test_when_finished`
command.

And maybe you want to extract this preparatory step into its own test
case:

test_expect_success 'set up shallow http repository' '
	test_when_finished "rm -rf shallow-repo" &&
	git clone --depth=1 --no-local parent shallow-repo &&
	git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
'

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

Please remove the extra empty line. (This goes for at least a couple test
cases added by this patch.)

> +'

This test case does not require `start_httpd`, and should therefore come
before the test cases that do require it (actually, it should come before
the `start_httpd` call, even).

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

Hmm. Reading through three test cases that all create `shallow-repo` in
the same way, I wonder whether we should not simply set it up once, and
then not even bother removing it. I think that would simplify not only the
patch, it would also simplify debugging later on.

> +
> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
> +	rm -rf shallow-repo &&
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	git clone --no-reject-shallow --no-local shallow-repo clone-repo
> +
> +'
> +
> +test_expect_success 'clone normal repository with --reject-shallow' '
> +	rm -rf clone-repo &&
> +	git clone --no-local parent normal-repo &&
> +	git clone --reject-shallow --no-local normal-repo clone-repo
> +
> +'
> +
> +test_expect_success 'unspecified any configs or options' '
> +	rm -rf shallow-repo clone-repo &&
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	git clone shallow-repo clone-repo
> +
> +'
> +

Having read through these test cases, I think they can be simplified by

- first setting up `shallow-repo`

- then testing in the same test case whether `--reject-shallow` fails and
  `--no-reject-shallow` succeeds, without `--no-local`

- then testing the same _with_ `--no-local`

These can go to t5606, no problem.

Then, in t5601, after the `start_httpd` call, add a single test case that

- sets up the shallow clone _directly_, i.e.

	git clone --bare --no-local --depth=1 parent \
		"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"

- verifies that `--reject-shallow` fails as expected, and

- verifies that `--no-reject-shallow` works as expected.

>  test_expect_success 'uses "origin" for default remote name' '
>
>  	git clone parent clone-default-origin &&
> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 9f555b87ecdf..adf873f60300 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'clone.rejectshallow=true should reject cloning' '
> +	rm -rf child &&
> +	git clone --depth=1 --no-local . child &&

In the following, this shallow repository is needed a couple of times.
Better set it up once, in a dedicated `set up shallow repository` test
case.

And `shallow-repo` would probably make for a much better name than
`child`.

> +	test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
> +	test_i18ngrep -e "source repository is shallow, reject to clone." err
> +'
> +
> +test_expect_success 'clone.rejectshallow=false should succeed' '
> +	rm -rf child out &&
> +	git clone --depth=1 --no-local . child &&
> +	git -c clone.rejectshallow=false clone --no-local child out
> +'

These two can be combined (and should, if you ask me, to simplify things).

> +
> +test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
> +	rm -rf child out &&
> +	git clone --no-local . child &&
> +	git -c clone.rejectshallow=true clone --no-local child out
> +'
> +
> +test_expect_success 'option --reject-shallow override clone.rejectshallow' '
> +	rm -rf child out &&
> +	git clone --depth=1 --no-local . child &&
> +	test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
> +	test_i18ngrep -e "source repository is shallow, reject to clone." err
> +'
> +
> +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
> +	rm -rf child out &&
> +	git clone --depth=1 --no-local . child &&
> +	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
> +'
> +

Personally, I think this is overkill. What I would do is to have a single
test case that verifies that

- `clone.rejectShallow=true` fails as expected,

- `clone.rejectShallow=false [...] --reject-shallow` fails as expected, and

- `clone.rejectShallow=false` succeeds.

If we do this, we do not even need a preparatory test case setting up the
shallow repository.

>  test_expect_success MINGW 'clone -c core.hideDotFiles' '
>  	test_commit attributes .gitattributes "" &&
>  	rm -rf child &&
> diff --git a/transport.c b/transport.c
> index 1c4ab676d1b1..a6b9f404d86a 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
>  		list_objects_filter_die_if_populated(&opts->filter_options);
>  		parse_list_objects_filter(&opts->filter_options, value);
>  		return 0;
> +	} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
> +		opts->reject_shallow = !!value;

I see that this is the established pattern (I am so grateful that I have
https://github.com/gitgitgadget/git/pull/865/files to look at the context,
something with which a pure mail-only patch contribution would not bless
me!), that those Boolean options are `NULL` vs non-`NULL`. So while you
pass `"1"` as the `value` parameter to `set_git_option()`, the parameter
`"0"` would _enable that option just the same_, you would have to pass
`NULL` to turn it off. I find that highly unintuitive, but that's not the
fault of your patch. The pattern is established, and you did the right
thing by following it.

> +		return 0;
>  	}
>  	return 1;
>  }

As I said, the rest of the patch looks good to me. With the few
suggestions I offered, I would be totally fine with this patch entering
`next`.

Thank you,
Dscho