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

["lilinchao@oschina.cn" <lilinchao@oschina.cn>]

Thanks for your suggestions!

I've combined your suggestions in this comment with Junio's which based on yours.

>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?
> 
The "Li Lilinchao" comes from my Github profile name. Actually, it contains my 
first name and last name respectively.
I will changed it to "Li Linchao" in the author info to keep these info consistent.

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