git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Mar 30, 2021, 09:54 UTC
Message-ID
<nycvar.QRO.7.76.6.2103301110040.52@tvgsbejvaqbjf.bet>
In-Reply-To
<pull.865.v8.git.1617013145206.gitgitgadget@gmail.com>
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?

Show 21 quoted lines
> 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.

Show 12 quoted lines
> 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".

Show 14 quoted lines
> +
>  --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.

Show 11 quoted lines
> 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.

Show 18 quoted lines
> +			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`).

Show 17 quoted lines
>
>  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).

Show 8 quoted lines
> +
> +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.

Show 22 quoted lines
> +
> +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.
Show 14 quoted lines
>  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`.

Show 9 quoted lines
> +	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).
Show 20 quoted lines
> +
> +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.

Show 13 quoted lines
>  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

Previous: Junio C HamanoNext: Junio C Hamano
Message 33 of 48 in “builtin/clone.c: add --no-shallow option”
  1. builtin/clone.c: add --no-shallow optionLi Linchao via GitGitGadget, Feb 4, 2021
  2. Junio C HamanoFeb 4, 2021
  3. lilinchao@oschina.cnFeb 4, 2021
  4. Junio C HamanoFeb 4, 2021
  5. Johannes SchindelinFeb 4, 2021
  6. Junio C HamanoFeb 4, 2021
  7. 0/2 builtin/clone.c: add --no-shallow optionLi Linchao via GitGitGadget, Feb 8, 2021
  8. 1/2 builtin/clone.c: add --no-shallow optionlilinchao via GitGitGadget, Feb 8, 2021
  9. 2/2 builtin/clone.c: add --reject-shallow optionlilinchao via GitGitGadget, Feb 8, 2021
  10. Derrick StoleeFeb 8, 2021
  11. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Feb 8, 2021
  12. Junio C HamanoFeb 9, 2021
  13. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Feb 21, 2021
  14. Junio C HamanoFeb 22, 2021
  15. Jonathan TanMar 1, 2021
  16. Junio C HamanoMar 1, 2021
  17. lilinchao@oschina.cnMar 2, 2021
  18. Junio C HamanoMar 3, 2021
  19. Jonathan TanMar 4, 2021
  20. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Feb 28, 2021
  21. lilinchao@oschina.cnMar 1, 2021
  22. Johannes SchindelinMar 1, 2021
  23. lilinchao@oschina.cnMar 4, 2021
  24. Junio C HamanoMar 3, 2021
  25. lilinchao@oschina.cnMar 4, 2021
  26. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Mar 4, 2021
  27. lilinchao@oschina.cnMar 12, 2021
  28. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Mar 25, 2021
  29. Junio C HamanoMar 25, 2021
  30. Junio C HamanoMar 25, 2021
  31. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Mar 29, 2021
  32. Junio C HamanoMar 29, 2021
  33. Johannes SchindelinMar 30, 2021
  34. Junio C HamanoMar 30, 2021
  35. Johannes SchindelinMar 31, 2021
  36. builtin/clone.c: add --reject-shallow optionlilinchao via GitGitGadget, Mar 31, 2021
  37. Junio C HamanoMar 31, 2021
  38. Johannes SchindelinMar 31, 2021
  39. Junio C HamanoMar 31, 2021
  40. builtin/clone.c: add --reject-shallow optionLi Linchao via GitGitGadget, Apr 1, 2021
  41. lilinchao@oschina.cnFeb 8, 2021
  42. lilinchao@oschina.cnFeb 10, 2021
  43. Junio C HamanoFeb 10, 2021
  44. lilinchao@oschina.cnFeb 20, 2021
  45. lilinchao@oschina.cnFeb 28, 2021
  46. lilinchao@oschina.cnMar 26, 2021
  47. lilinchao@oschina.cnMar 26, 2021
  48. lilinchao@oschina.cnMar 31, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.