git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] builtin/clone.c: add --no-shallow option
@ 2021-02-04  3:31 Li Linchao via GitGitGadget
  2021-02-04  5:50 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-02-04  3:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

This patch add a new option that reject to clone a shallow repository.
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
of repository, and want to exit the process immediately without creating
any unnecessary files.

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
    builtin/clone.c: add --no-shallow option
    
    This patch add a new option that reject to clone a shallow repository.
    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
    of repository, and want to exit the process immediately without creating
    any unnecessary files.
    
    Signed-off-by: lilinchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/865

 Documentation/config/clone.txt |  3 +++
 Documentation/git-clone.txt    |  8 +++++++-
 builtin/clone.c                | 13 +++++++++++++
 t/t5606-clone-options.sh       |  7 +++++++
 t/t5611-clone-config.sh        |  7 +++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 builtin/clone.c

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..29b779b8a825 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,6 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectshallow::
+	Reject to clone a repository if it is a shallow one.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 876aedcd472a..94bc76702757 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--no-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -145,6 +145,12 @@ 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-shallow::
+	Don't clone a shallow source repository. In some scenariors, clients
+	want the cloned repository information to be complete. Otherwise,
+	the cloning process should end immediately without creating any files,
+	which can save some disk space.
+
 --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
old mode 100644
new mode 100755
index e335734b4cfd..b07d867e6641
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ 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_no_shallow;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -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")),
@@ -858,6 +860,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_no_shallow = 1;
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -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;
@@ -1222,6 +1229,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	if (option_local > 0 && !is_local)
 		warning(_("--local is ignored"));
+
+	if (is_shallow) {
+		if (option_no_shallow)
+			die(_("source repository is shallow, reject to clone."));
+	}
+
 	transport->cloning = 1;
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
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
+
+'
+
 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 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 &&
+	test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&

base-commit: 72c4083ddf91b489b7b7b812df67ee8842177d98
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] builtin/clone.c: add --no-shallow option
  2021-02-04  3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
@ 2021-02-04  5:50 ` Junio C Hamano
  2021-02-04 10:32   ` lilinchao
  2021-02-04 14:00 ` Johannes Schindelin
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-02-04  5:50 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho, Li Linchao

"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 ;-)

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

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


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


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH] builtin/clone.c: add --no-shallow option
  2021-02-04  5:50 ` Junio C Hamano
@ 2021-02-04 10:32   ` lilinchao
  2021-02-04 18:36     ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: lilinchao @ 2021-02-04 10:32 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho


--------------
lilinchao@oschina.cn
>"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. 

Ok, will do.

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

I found an issue described like this:

    The blame information can be completely wrong when fetching it from
    a shallow clone, without errors or warnings. When the outcome is invalid
    data, it's extremely difficult to diagnose that it comes from a shallow clone.
    If a line in a file was not changed in the commits that were downloaded as
    part of the shallow fetch, git will report the first known commit as the author.
    This has a big impact on the auto-assignment of new issues.

It looks like this is another scenario that can prove this feature is necessary.

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

Oops, this happened when I edited it in VS Code, it noticed me 'permission denied' when
I want to save the file. 

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

You're right, "--reject-shallow" is much better. 
I didn't realize that bool options have default [no-] option.

>> @@ -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 ...
>
  
After I applied your review suggestions above, then we can reject a 
non-local clone from shallow repo. For now, it will clone a empty 
repo with --no-local option.

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

Thank you for so many effective suggestions, I will write test case more carefully :)

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] builtin/clone.c: add --no-shallow option
  2021-02-04  3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
  2021-02-04  5:50 ` Junio C Hamano
@ 2021-02-04 14:00 ` Johannes Schindelin
  2021-02-04 18:24   ` Junio C Hamano
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-02-04 14:00 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget
  Cc: git, Junio C Hamano, Derrick Stolee, Li Linchao, lilinchao

Hi,

in addition to what Junio said:

On Thu, 4 Feb 2021, Li Linchao via GitGitGadget wrote:

> diff --git a/builtin/clone.c b/builtin/clone.c
> index e335734b4cfd..b07d867e6641
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -858,6 +860,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_no_shallow = 1;

This needs to use `git_config_bool(k, v)` to allow for
`clone.rejectShallow = false`.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] builtin/clone.c: add --no-shallow option
  2021-02-04 14:00 ` Johannes Schindelin
@ 2021-02-04 18:24   ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:24 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, Li Linchao

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> in addition to what Junio said:
>
> On Thu, 4 Feb 2021, Li Linchao via GitGitGadget wrote:
>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index e335734b4cfd..b07d867e6641
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -858,6 +860,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_no_shallow = 1;
>
> This needs to use `git_config_bool(k, v)` to allow for
> `clone.rejectShallow = false`.

Thanks.  I completely missed that.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] builtin/clone.c: add --no-shallow option
  2021-02-04 10:32   ` lilinchao
@ 2021-02-04 18:36     ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:36 UTC (permalink / raw)
  To: lilinchao; +Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, dscho

"lilinchao@oschina.cn" <lilinchao@oschina.cn> writes:

> I found an issue described like this:
>
>     The blame information can be completely wrong when fetching it from
>     a shallow clone, without errors or warnings. When the outcome is invalid
>     data, it's extremely difficult to diagnose that it comes from a shallow clone.
>     If a line in a file was not changed in the commits that were downloaded as
>     part of the shallow fetch, git will report the first known commit as the author.
>     This has a big impact on the auto-assignment of new issues.
>
> It looks like this is another scenario that can prove this feature is necessary.

In other words:

    Users may want more history than the repository offered for
    cloning, which happens to be shallow, can give them.

And the way chosen by "--reject-shallow" is to require that the
source repository has the entire history.

I wonder if the design of this UI is flexible enough so that we can
extend it to allow "I need the history that goes back at least to X"
in the future.  "I need the history goes back to all the roots" then
becomes a narrow special case of that.

> After I applied your review suggestions above, then we can reject a 
> non-local clone from shallow repo. For now, it will clone a empty 
> repo with --no-local option.

It is my understanding that your patch with or without my suggestion
only deals with local clone and does not change anything for
non-local case.  A "local-only" solution is a good place to start
and is a good test bed to experiment with the user experience, but I
view without support for non-local clone, it would not be ready for
general use.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 0/2] builtin/clone.c: add --no-shallow option
  2021-02-04  3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
  2021-02-04  5:50 ` Junio C Hamano
  2021-02-04 14:00 ` Johannes Schindelin
@ 2021-02-08  8:31 ` Li Linchao via GitGitGadget
  2021-02-08  8:31   ` [PATCH v2 1/2] " lilinchao via GitGitGadget
                     ` (4 more replies)
  2 siblings, 5 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-02-08  8:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao

Change since v1:

 * Rename --no-shallow to --reject-shallow
 * Enable to reject a non-local clone
 * Enable --[no-]reject-shallow from CLI override configuration.
 * Add more testcases.
 * Reword commit messages and relative documentation.

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

lilinchao (2):
  builtin/clone.c: add --no-shallow option
  builtin/clone.c: add --reject-shallow option

 Documentation/config/clone.txt |  4 ++++
 Documentation/git-clone.txt    | 20 ++++++++++++++++++-
 builtin/clone.c                | 35 ++++++++++++++++++++++++++++++++-
 t/t5606-clone-options.sh       | 36 ++++++++++++++++++++++++++++++++++
 t/t5611-clone-config.sh        | 32 ++++++++++++++++++++++++++++++
 5 files changed, 125 insertions(+), 2 deletions(-)


base-commit: fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/865

Range-diff vs v1:

 1:  594323684af0 = 1:  2f9602495eb5 builtin/clone.c: add --no-shallow option
 -:  ------------ > 2:  cfcfc3ec6b37 builtin/clone.c: add --reject-shallow option

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 1/2] builtin/clone.c: add --no-shallow option
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
@ 2021-02-08  8:31   ` lilinchao via GitGitGadget
  2021-02-08  8:31   ` [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option lilinchao via GitGitGadget
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: lilinchao via GitGitGadget @ 2021-02-08  8:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

This patch add a new option that reject to clone a shallow repository.
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
of repository, and want to exit the process immediately without creating
any unnecessary files.

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
 Documentation/config/clone.txt |  3 +++
 Documentation/git-clone.txt    |  8 +++++++-
 builtin/clone.c                | 13 +++++++++++++
 t/t5606-clone-options.sh       |  7 +++++++
 t/t5611-clone-config.sh        |  7 +++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 builtin/clone.c

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..29b779b8a825 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,6 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectshallow::
+	Reject to clone a repository if it is a shallow one.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..9ce6e545577f 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--no-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -149,6 +149,12 @@ 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-shallow::
+	Don't clone a shallow source repository. In some scenariors, clients
+	want the cloned repository information to be complete. Otherwise,
+	the cloning process should end immediately without creating any files,
+	which can save some disk space.
+
 --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
old mode 100644
new mode 100755
index e335734b4cfd..b07d867e6641
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ 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_no_shallow;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -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")),
@@ -858,6 +860,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_no_shallow = 1;
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -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;
@@ -1222,6 +1229,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	if (option_local > 0 && !is_local)
 		warning(_("--local is ignored"));
+
+	if (is_shallow) {
+		if (option_no_shallow)
+			die(_("source repository is shallow, reject to clone."));
+	}
+
 	transport->cloning = 1;
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 5d6e63a841f7..e9b2bde8be8e 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -45,6 +45,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
+
+'
+
 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..68fdf5b7ce1b 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,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 &&
+	test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
  2021-02-08  8:31   ` [PATCH v2 1/2] " lilinchao via GitGitGadget
@ 2021-02-08  8:31   ` lilinchao via GitGitGadget
  2021-02-08 13:33   ` [PATCH v2 0/2] builtin/clone.c: add --no-shallow option Derrick Stolee
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: lilinchao via GitGitGadget @ 2021-02-08  8:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

In some scenarios, users may want more history than the repository
offered for cloning, which mostly to be a shallow repository, can
give them. But 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, given that 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 an unshallow repository.

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

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
 Documentation/config/clone.txt |  3 ++-
 Documentation/git-clone.txt    | 20 ++++++++++++++++----
 builtin/clone.c                | 32 ++++++++++++++++++++++++++------
 t/t5606-clone-options.sh       | 33 +++++++++++++++++++++++++++++++--
 t/t5611-clone-config.sh        | 29 +++++++++++++++++++++++++++--
 5 files changed, 102 insertions(+), 15 deletions(-)
 mode change 100755 => 100644 builtin/clone.c

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 29b779b8a825..50ebc170bb81 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -4,4 +4,5 @@ clone.defaultRemoteName::
 	option to linkgit:git-clone[1].
 
 clone.rejectshallow::
-	Reject to clone a repository if it is a shallow one.
+	Reject to clone a repository if it is a shallow one, can be overridden by
+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 9ce6e545577f..af5a97903a05 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--no-shallow]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -149,11 +149,23 @@ 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-shallow::
-	Don't clone a shallow source repository. In some scenariors, clients
+--reject-shallow::
+	Don't clone a shallow source repository. In some scenarios, clients
 	want the cloned repository information to be complete. Otherwise,
 	the cloning process should end immediately without creating any files,
-	which can save some disk space.
+	which can save some disk space. This can override `clone.rejectshallow`
+	from the configuration:
+
+	--------------------------------------------------------------------
+	$ git -c clone.rejectshallow=false clone --reject-shallow source out
+	--------------------------------------------------------------------
+
+	While there is a way to countermand a configured "I always refuse to
+	clone from a shallow repository" with "but let's allow it only this time":
+
+	----------------------------------------------------------------------
+	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
+	----------------------------------------------------------------------
 
 --bare::
 	Make a 'bare' Git repository.  That is, instead of
diff --git a/builtin/clone.c b/builtin/clone.c
old mode 100755
new mode 100644
index b07d867e6641..a9d49c8e13c2
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,7 +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_no_shallow;
+static int option_no_shallow = -1;  /* unspecified */
+static int config_shallow = -1;    /* unspecified */
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -91,7 +92,8 @@ 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(0, "reject-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")),
@@ -861,7 +863,7 @@ static int git_clone_config(const char *k, const char *v, void *cb)
 		remote_name = xstrdup(v);
 	}
 	if (!strcmp(k, "clone.rejectshallow")) {
-		option_no_shallow = 1;
+		config_shallow = git_config_bool(k, v);
 	}
 	return git_default_config(k, v, cb);
 }
@@ -1211,6 +1213,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	path = get_repo_path(remote->url[0], &is_bundle);
 	is_local = option_local != 0 && path && !is_bundle;
+
+	/* Detect if the remote repository is shallow */
+	if (!access(mkpath("%s/shallow", path), F_OK)) {
+		is_shallow = 1;
+	}
+
 	if (is_local) {
 		if (option_depth)
 			warning(_("--depth is ignored in local clones; use file:// instead."));
@@ -1220,8 +1228,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
 		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 (is_shallow) {
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
 			is_local = 0;
@@ -1231,8 +1238,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		warning(_("--local is ignored"));
 
 	if (is_shallow) {
-		if (option_no_shallow)
+		int reject = 0;
+
+		/* If option_no_shallow is specified from CLI option,
+		 * ignore config_shallow from git_clone_config.
+		 */
+
+		if (config_shallow != -1) {
+			reject = config_shallow;
+		}
+		if (option_no_shallow != -1) {
+			reject = option_no_shallow;
+		}
+		if (reject) {
 			die(_("source repository is shallow, reject to clone."));
+		}
 	}
 
 	transport->cloning = 1;
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index e9b2bde8be8e..bf007448ad20 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -45,13 +45,42 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
-test_expect_success 'reject clone shallow repository' '
+test_expect_success 'fail to 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_must_fail git clone --reject-shallow shallow-repo out 2>err &&
 	test_i18ngrep -e "source repository is shallow, reject to clone." err
 
 '
 
+test_expect_success 'fail to clone 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
+
+'
+
+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
+
+'
+
 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 68fdf5b7ce1b..00789d7487d5 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,13 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
 	test_cmp expect actual
 '
 
-test_expect_success 'clone -c clone.rejectshallow' '
+test_expect_success 'clone.rejectshallow=true should fail to clone' '
 	rm -rf child &&
 	git clone --depth=1 --no-local . child &&
-	test_must_fail git clone -c clone.rejectshallow child out 2>err &&
+	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 cloning' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=false clone --no-local child out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed cloning 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 clone -c clone.rejectshallow=false  --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 &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/2] builtin/clone.c: add --no-shallow option
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
  2021-02-08  8:31   ` [PATCH v2 1/2] " lilinchao via GitGitGadget
  2021-02-08  8:31   ` [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option lilinchao via GitGitGadget
@ 2021-02-08 13:33   ` Derrick Stolee
       [not found]   ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
  2021-02-08 14:12   ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
  4 siblings, 0 replies; 31+ messages in thread
From: Derrick Stolee @ 2021-02-08 13:33 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget, git; +Cc: Junio C Hamano, dscho, Li Linchao

On 2/8/2021 3:31 AM, Li Linchao via GitGitGadget wrote:
> Range-diff vs v1:
> 
>  1:  594323684af0 = 1:  2f9602495eb5 builtin/clone.c: add --no-shallow option
>  -:  ------------ > 2:  cfcfc3ec6b37 builtin/clone.c: add --reject-shallow option

You should rewrite your history to make this one commit again. You can
do this by running 'git rebase -i HEAD~2' then editing the todo file
to 'squash' the second commit in to the first. You will be prompted to
edit the combined commit message, and you should preserve only the message
from the second commit.

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v2 0/2] builtin/clone.c: add --no-shallow option
       [not found]   ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
@ 2021-02-08 13:48     ` lilinchao
  0 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-02-08 13:48 UTC (permalink / raw)
  To: Derrick Stolee, Li Linchao via GitGitGadget, git; +Cc: Junio C Hamano, dscho

Thanks, have done. And I am waiting for all CI on Github to turn green.
--------------
lilinchao@oschina.cn
>On 2/8/2021 3:31 AM, Li Linchao via GitGitGadget wrote:
>> Range-diff vs v1:
>>
>>  1:  594323684af0 = 1:  2f9602495eb5 builtin/clone.c: add --no-shallow option
>>  -:  ------------ > 2:  cfcfc3ec6b37 builtin/clone.c: add --reject-shallow option
>
>You should rewrite your history to make this one commit again. You can
>do this by running 'git rebase -i HEAD~2' then editing the todo file
>to 'squash' the second commit in to the first. You will be prompted to
>edit the combined commit message, and you should preserve only the message
>from the second commit.
>
>Thanks,
>-Stolee
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3] builtin/clone.c: add --reject-shallow option
  2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
                     ` (3 preceding siblings ...)
       [not found]   ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
@ 2021-02-08 14:12   ` Li Linchao via GitGitGadget
  2021-02-09 20:32     ` Junio C Hamano
                       ` (2 more replies)
  4 siblings, 3 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-02-08 14:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

In some scenarios, users may want more history than the repository
offered for cloning, which mostly to be a shallow repository, can
give them. But 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, given that 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 an unshallow repository.

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

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
    builtin/clone.c: add --no-shallow option
    
    Change since v1:
    
     * Rename --no-shallow to --reject-shallow
     * Enable to reject a non-local clone
     * Enable --[no-]reject-shallow from CLI override configuration.
     * Add more testcases.
     * Reword commit messages and relative documentation.
    
    Signed-off-by: lilinchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/865

Range-diff vs v2:

 1:  2f9602495eb5 < -:  ------------ builtin/clone.c: add --no-shallow option
 2:  cfcfc3ec6b37 ! 1:  f5ed6b2e4481 builtin/clone.c: add --reject-shallow option
     @@ Commit message
      
       ## Documentation/config/clone.txt ##
      @@ Documentation/config/clone.txt: clone.defaultRemoteName::
     + 	The name of the remote to create when cloning a repository.  Defaults to
     + 	`origin`, and can be overridden by passing the `--origin` command-line
       	option to linkgit:git-clone[1].
     - 
     - clone.rejectshallow::
     --	Reject to clone a repository if it is a shallow one.
     ++
     ++clone.rejectshallow::
      +	Reject to clone a repository if it is a shallow one, can be overridden by
      +	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
      
     @@ Documentation/git-clone.txt: SYNOPSIS
       	  [--dissociate] [--separate-git-dir <git dir>]
       	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
       	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
     --	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--no-shallow]
     +-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
      +	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]
       	  [--filter=<filter>] [--] <repository>
       	  [<directory>]
     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
       --no-checkout::
       	No checkout of HEAD is performed after the clone is complete.
       
     ----no-shallow::
     --	Don't clone a shallow source repository. In some scenariors, clients
      +--reject-shallow::
      +	Don't clone a shallow source repository. In some scenarios, clients
     - 	want the cloned repository information to be complete. Otherwise,
     - 	the cloning process should end immediately without creating any files,
     --	which can save some disk space.
     ++	want the cloned repository information to be complete. Otherwise,
     ++	the cloning process should end immediately without creating any files,
      +	which can save some disk space. This can override `clone.rejectshallow`
      +	from the configuration:
      +
     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
      +	----------------------------------------------------------------------
      +	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
      +	----------------------------------------------------------------------
     - 
     ++
       --bare::
       	Make a 'bare' Git repository.  That is, instead of
     + 	creating `<directory>` and placing the administrative
      
     - ## builtin/clone.c (mode change 100755 => 100644) ##
     + ## builtin/clone.c ##
      @@ builtin/clone.c: 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_no_shallow;
      +static int option_no_shallow = -1;  /* unspecified */
      +static int config_shallow = -1;    /* unspecified */
       static int deepen;
     @@ builtin/clone.c: 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(0, "reject-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")),
      @@ builtin/clone.c: 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_no_shallow = 1;
     ++	if (!strcmp(k, "clone.rejectshallow")) {
      +		config_shallow = git_config_bool(k, v);
     - 	}
     ++	}
       	return git_default_config(k, v, cb);
       }
     + 
     +@@ builtin/clone.c: 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;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       
       	path = get_repo_path(remote->url[0], &is_bundle);
     @@ builtin/clone.c: 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 (is_shallow) {
       			if (option_local > 0)
       				warning(_("source repository is shallow, ignoring --local"));
       			is_local = 0;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	}
     + 	if (option_local > 0 && !is_local)
       		warning(_("--local is ignored"));
     - 
     - 	if (is_shallow) {
     --		if (option_no_shallow)
     ++
     ++	if (is_shallow) {
      +		int reject = 0;
      +
      +		/* If option_no_shallow is specified from CLI option,
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      +			reject = option_no_shallow;
      +		}
      +		if (reject) {
     - 			die(_("source repository is shallow, reject to clone."));
     ++			die(_("source repository is shallow, reject to clone."));
      +		}
     - 	}
     - 
     ++	}
     ++
       	transport->cloning = 1;
     + 
     + 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
      
       ## t/t5606-clone-options.sh ##
      @@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-git-dir' '
       
       '
       
     --test_expect_success 'reject clone shallow repository' '
      +test_expect_success 'fail to clone shallow repository' '
     - 	git clone --depth=1 --no-local parent shallow-repo &&
     --	test_must_fail git clone --no-shallow shallow-repo out 2>err &&
     ++	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
     - 
     - '
     - 
     ++	test_i18ngrep -e "source repository is shallow, reject to clone." err
     ++
     ++'
     ++
      +test_expect_success 'fail to clone non-local shallow repository' '
      +	rm -rf shallow-repo &&
      +	git clone --depth=1 --no-local parent shallow-repo &&
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
       	test_cmp expect actual
       '
       
     --test_expect_success 'clone -c clone.rejectshallow' '
      +test_expect_success 'clone.rejectshallow=true should fail to clone' '
     - 	rm -rf child &&
     - 	git clone --depth=1 --no-local . child &&
     --	test_must_fail git clone -c clone.rejectshallow child out 2>err &&
     ++	rm -rf child &&
     ++	git clone --depth=1 --no-local . 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_i18ngrep -e "source repository is shallow, reject to clone." err
     ++'
     ++
      +test_expect_success 'clone.rejectshallow=false should succeed cloning' '
      +	rm -rf child &&
      +	git clone --depth=1 --no-local . child &&


 Documentation/config/clone.txt |  4 ++++
 Documentation/git-clone.txt    | 20 ++++++++++++++++++-
 builtin/clone.c                | 35 ++++++++++++++++++++++++++++++++-
 t/t5606-clone-options.sh       | 36 ++++++++++++++++++++++++++++++++++
 t/t5611-clone-config.sh        | 32 ++++++++++++++++++++++++++++++
 5 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..50ebc170bb81 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectshallow::
+	Reject to clone a repository if it is a shallow one, can be overridden by
+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..af5a97903a05 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -149,6 +149,24 @@ 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.
 
+--reject-shallow::
+	Don't clone a shallow source repository. In some scenarios, clients
+	want the cloned repository information to be complete. Otherwise,
+	the cloning process should end immediately without creating any files,
+	which can save some disk space. This can override `clone.rejectshallow`
+	from the configuration:
+
+	--------------------------------------------------------------------
+	$ git -c clone.rejectshallow=false clone --reject-shallow source out
+	--------------------------------------------------------------------
+
+	While there is a way to countermand a configured "I always refuse to
+	clone from a shallow repository" with "but let's allow it only this time":
+
+	----------------------------------------------------------------------
+	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
+	----------------------------------------------------------------------
+
 --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 e335734b4cfd..a9d49c8e13c2 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_no_shallow = -1;  /* unspecified */
+static int config_shallow = -1;    /* unspecified */
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
 		 N_("force progress reporting")),
+	OPT_BOOL(0, "reject-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")),
@@ -858,6 +862,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")) {
+		config_shallow = git_config_bool(k, v);
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -963,6 +970,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;
@@ -1205,6 +1213,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	path = get_repo_path(remote->url[0], &is_bundle);
 	is_local = option_local != 0 && path && !is_bundle;
+
+	/* Detect if the remote repository is shallow */
+	if (!access(mkpath("%s/shallow", path), F_OK)) {
+		is_shallow = 1;
+	}
+
 	if (is_local) {
 		if (option_depth)
 			warning(_("--depth is ignored in local clones; use file:// instead."));
@@ -1214,7 +1228,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
 		if (filter_options.choice)
 			warning(_("--filter is ignored in local clones; use file:// instead."));
-		if (!access(mkpath("%s/shallow", path), F_OK)) {
+		if (is_shallow) {
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
 			is_local = 0;
@@ -1222,6 +1236,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	if (option_local > 0 && !is_local)
 		warning(_("--local is ignored"));
+
+	if (is_shallow) {
+		int reject = 0;
+
+		/* If option_no_shallow is specified from CLI option,
+		 * ignore config_shallow from git_clone_config.
+		 */
+
+		if (config_shallow != -1) {
+			reject = config_shallow;
+		}
+		if (option_no_shallow != -1) {
+			reject = option_no_shallow;
+		}
+		if (reject) {
+			die(_("source repository is shallow, reject to clone."));
+		}
+	}
+
 	transport->cloning = 1;
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 5d6e63a841f7..bf007448ad20 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -45,6 +45,42 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
+test_expect_success 'fail to clone shallow repository' '
+	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
+
+'
+
+test_expect_success 'fail to clone 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
+
+'
+
+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
+
+'
+
 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..00789d7487d5 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 fail to clone' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . 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 cloning' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=false clone --no-local child out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed cloning 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 clone -c clone.rejectshallow=false  --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 &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&

base-commit: fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3] builtin/clone.c: add --reject-shallow option
  2021-02-08 14:12   ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
@ 2021-02-09 20:32     ` Junio C Hamano
       [not found]     ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
  2021-02-21  7:05     ` [PATCH v4] " Li Linchao via GitGitGadget
  2 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-02-09 20:32 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho, Li Linchao

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

> From: lilinchao <lilinchao@oschina.cn>
>
> In some scenarios, users may want more history than the repository
> offered for cloning, which mostly to be a shallow repository, can
> give them.

Sorry, but I find this hard to understand.  Are you saying that most
of the repositories that users try to clone from are shallow?

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

This one on the other hand is easy to understand, but we would
probably need something like s/But/But because/.

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

Hmph, that is an interesting point.  This makes me wonder if we can
achieve the same without adding a new option at the UI level (e.g.
by allowing "--depth" to take "infinity" and reject cloning if we
find out that the origin repository is a shallow one).  But we can
worry about it later once after we get the machinery driven by the
UI right.

> In other scenarios, given that we have an API that allow us to import
> external repository, and then perform various operations on the repo.

Sorry, but I do not understand what you want to say with these two
lines ("Given that X and Y" needs to be followed by a concluding
statement, e.g. "Given that we have API to import and operate, we
can do Z"---you are missing that "we can do Z" part).

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 02d9c19cec75..af5a97903a05 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -15,7 +15,7 @@ SYNOPSIS
>  	  [--dissociate] [--separate-git-dir <git dir>]
>  	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
>  	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
> -	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
> +	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]

Isn't it "--[no-]reject-shallow"?  Offering the negation from the
command line is essential if "[clone] rejectshallow" configuration
is allowed to set the default to true.

> +--reject-shallow::
> +	Don't clone a shallow source repository. In some scenarios, clients
> +	want the cloned repository information to be complete. Otherwise,
> +	the cloning process should end immediately without creating any files,
> +	which can save some disk space. This can override `clone.rejectshallow`
> +	from the configuration:
> +
> +	--------------------------------------------------------------------
> +	$ git -c clone.rejectshallow=false clone --reject-shallow source out
> +	--------------------------------------------------------------------
> +
> +	While there is a way to countermand a configured "I always refuse to
> +	clone from a shallow repository" with "but let's allow it only this time":
> +
> +	----------------------------------------------------------------------
> +	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
> +	----------------------------------------------------------------------


This is way too verbose and gives unnecessary details that readers
already know or do not need to know (e.g. setting configuration from
the command line and immediately override it from the command line
is not something end-users would EVER need to do---only test writers
who develop Git would need it).  Something like

    Fail if the source repository is a shallow repository.  The
    `clone.rejectShallow` configuration variable can be used to
    give the default.   

would be sufficient.  All readers ought to know when a configuration
and command line option exist, the latter can be used to override
the default former gives, and it is *not* a job for the description
of an individual option to teach them to such a detail like the
above does.

> +static int option_no_shallow = -1;  /* unspecified */
> +static int config_shallow = -1;    /* unspecified */

Hmph.  I would have expected the usual "prepare a single variable
and initialize it to the default, read the config to set it, and
then parse the command line to overwrite it" sequence would suffice
so it is puzzling why we want two separate variables here.

Let's read on to find out.

> @@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
>  	OPT__VERBOSITY(&option_verbosity),
>  	OPT_BOOL(0, "progress", &option_progress,
>  		 N_("force progress reporting")),
> +	OPT_BOOL(0, "reject-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")),
> @@ -858,6 +862,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")) {
> +		config_shallow = git_config_bool(k, v);
> +	}
>  	return git_default_config(k, v, cb);
>  }

You are adding to git_clone_config(), so instead of setting the
value to config_shallow, setting the value to the same variable that
will be used in builtin_clone_options[] array should be sufficient.

cmd_clone() begins like so:


	git_config(git_clone_config, NULL);
	argc = parse_options(...);

which means that single variable (let's call it reject_shallow)
can (1) stay to be its initial value if no config or option is
given, (2) if there is config, the git_config() call will cause
that variable assigned, (3) if there is option, parse_options()
call will cause that variable assigned, possibly overwriting the
value taken from the config.

Which is exactly what we want.  So in short, declare just a single

    static int reject_shallow; /* default to false */
    
instead of "option_no_shallow" and "config_shallow", and use it in
both builtin_clone_options[] given to parse_options, and
git_clone_config() that is given to git_config(), and you'd be fine,
I think.

> @@ -963,6 +970,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;
> @@ -1205,6 +1213,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	path = get_repo_path(remote->url[0], &is_bundle);
>  	is_local = option_local != 0 && path && !is_bundle;
> +
> +	/* Detect if the remote repository is shallow */
> +	if (!access(mkpath("%s/shallow", path), F_OK)) {
> +		is_shallow = 1;
> +	}

This is only for cloning from a local repository, no?  IOW, path at
this point may even be "git://github.com/git/git/" and checking with
access() does not make sense.

Ah, it is even worse.  get_repo_path() can return NULL, so mkpath()
will crash in such a case.  This must be at least

	if (path && !access(mkpath("%s/shallow", path), F_OK))
		is_shallow = 1;

but I think the logic fits better in the body of "if (is_Local)"
thing that immediately follows.  It is specific to the case where
cloning from a local repository and access(mkpath()) that is about
the local filesystem (as opposed to going through the transport
layer) belongs there.

>  	if (is_local) {
>  		if (option_depth)
>  			warning(_("--depth is ignored in local clones; use file:// instead."));
> @@ -1214,7 +1228,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
>  		if (filter_options.choice)
>  			warning(_("--filter is ignored in local clones; use file:// instead."));
> -		if (!access(mkpath("%s/shallow", path), F_OK)) {
> +		if (is_shallow) {
>  			if (option_local > 0)
>  				warning(_("source repository is shallow, ignoring --local"));
>  			is_local = 0;

So, I think the above two hunks are making the code worse.  If we
are to detect and reject cloning from the shallow repository when
going through the transport layer (i.e. "--no-local" or cloning from
"git://github.com/git/git", or "https://github.com/git/git", if it
were a shallow repository), that must be handled separately.

> @@ -1222,6 +1236,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	}
>  	if (option_local > 0 && !is_local)
>  		warning(_("--local is ignored"));
> +
> +	if (is_shallow) {
> +		int reject = 0;
> +
> +		/* If option_no_shallow is specified from CLI option,
> +		 * ignore config_shallow from git_clone_config.
> +		 */
> +
> +		if (config_shallow != -1) {
> +			reject = config_shallow;
> +		}
> +		if (option_no_shallow != -1) {
> +			reject = option_no_shallow;
> +		}

I do not think any of the above is necessary with just a single
reject_shallow variable that is initialized to 0, can be set by
git_config() callback, and can further be set by parse_options().

> +		if (reject) {
> +			die(_("source repository is shallow, reject to clone."));
> +		}

> +	}
> +
>  	transport->cloning = 1;
>  
>  	transport_set_option(transport, TRANS_OPT_KEEP, "yes");

I do not see how this change would allow users to reject cloning
http://github.com/git/git, if that repository were shallow, though.
I think that would need changes to the code that interacts with
these transport_* functions we see later part of this functrion.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v3] builtin/clone.c: add --reject-shallow option
       [not found]     ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
@ 2021-02-10  9:07       ` lilinchao
  2021-02-10 16:27         ` Junio C Hamano
       [not found]         ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
  0 siblings, 2 replies; 31+ messages in thread
From: lilinchao @ 2021-02-10  9:07 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho

--------------
lilinchao@oschina.cn
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: lilinchao <lilinchao@oschina.cn>
>>
>> In some scenarios, users may want more history than the repository
>> offered for cloning, which mostly to be a shallow repository, can
>> give them.
>
>Sorry, but I find this hard to understand.  Are you saying that most
>of the repositories that users try to clone from are shallow? 
> 
Oh, sorry, it shoud be "which happens to be a shallow repository".

>> But 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.
>
>This one on the other hand is easy to understand, but we would
>probably need something like s/But/But because/.
> 
Ok, I will substitute it.

>> 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.
>
>Hmph, that is an interesting point.  This makes me wonder if we can
>achieve the same without adding a new option at the UI level (e.g.
>by allowing "--depth" to take "infinity" and reject cloning if we
>find out that the origin repository is a shallow one).  But we can
>worry about it later once after we get the machinery driven by the
>UI right.
> 
Please let me explain the purpose of this patch in another way:
In practice, I may need a filter in clone command to filter out remote
shallow repository that may appear. I think a new option like '--reject-shallow',
or '--filter-shallow', or something like that, has a clearer purpose for users, they
don't have to come up with a specific depth number to achieve the same purpose.
A literal filter, or option is just ok, I think.

>> In other scenarios, given that we have an API that allow us to import
>> external repository, and then perform various operations on the repo.
>
>Sorry, but I do not understand what you want to say with these two
>lines ("Given that X and Y" needs to be followed by a concluding
>statement, e.g. "Given that we have API to import and operate, we
>can do Z"---you are missing that "we can do Z" part).
> 
Forgive my crappy English :-(
What I want to express is "if we have an API to do sth, then do sth".

>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>> index 02d9c19cec75..af5a97903a05 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -15,7 +15,7 @@ SYNOPSIS
>>    [--dissociate] [--separate-git-dir <git dir>]
>>    [--depth <depth>] [--[no-]single-branch] [--no-tags]
>>    [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
>> -	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
>> +	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]
>
>Isn't it "--[no-]reject-shallow"?  Offering the negation from the
>command line is essential if "[clone] rejectshallow" configuration
>is allowed to set the default to true.
> 
Ok, that makes sense.

>> +--reject-shallow::
>> +	Don't clone a shallow source repository. In some scenarios, clients
>> +	want the cloned repository information to be complete. Otherwise,
>> +	the cloning process should end immediately without creating any files,
>> +	which can save some disk space. This can override `clone.rejectshallow`
>> +	from the configuration:
>> +
>> +	--------------------------------------------------------------------
>> +	$ git -c clone.rejectshallow=false clone --reject-shallow source out
>> +	--------------------------------------------------------------------
>> +
>> +	While there is a way to countermand a configured "I always refuse to
>> +	clone from a shallow repository" with "but let's allow it only this time":
>> +
>> +	----------------------------------------------------------------------
>> +	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
>> +	----------------------------------------------------------------------
>
>
>This is way too verbose and gives unnecessary details that readers
>already know or do not need to know (e.g. setting configuration from
>the command line and immediately override it from the command line
>is not something end-users would EVER need to do---only test writers
>who develop Git would need it).  Something like
>
>    Fail if the source repository is a shallow repository.  The
>    `clone.rejectShallow` configuration variable can be used to
>    give the default.  
>
>would be sufficient.  All readers ought to know when a configuration
>and command line option exist, the latter can be used to override
>the default former gives, and it is *not* a job for the description
>of an individual option to teach them to such a detail like the
>above does.
> 
>> +static int option_no_shallow = -1;  /* unspecified */
>> +static int config_shallow = -1;    /* unspecified */
>
>Hmph.  I would have expected the usual "prepare a single variable
>and initialize it to the default, read the config to set it, and
>then parse the command line to overwrite it" sequence would suffice
>so it is puzzling why we want two separate variables here.
>
>Let's read on to find out.
>
>> @@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
>>  OPT__VERBOSITY(&option_verbosity),
>>  OPT_BOOL(0, "progress", &option_progress,
>>  N_("force progress reporting")),
>> +	OPT_BOOL(0, "reject-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")),
>> @@ -858,6 +862,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")) {
>> +	config_shallow = git_config_bool(k, v);
>> +	}
>>  return git_default_config(k, v, cb);
>>  }
>
>You are adding to git_clone_config(), so instead of setting the
>value to config_shallow, setting the value to the same variable that
>will be used in builtin_clone_options[] array should be sufficient.
>
>cmd_clone() begins like so:
>
>
>	git_config(git_clone_config, NULL);
>	argc = parse_options(...);
>
>which means that single variable (let's call it reject_shallow)
>can (1) stay to be its initial value if no config or option is
>given, (2) if there is config, the git_config() call will cause
>that variable assigned, (3) if there is option, parse_options()
>call will cause that variable assigned, possibly overwriting the
>value taken from the config.
> 
Sorry, you may forget there is a re-read git-config under these lines
(around at line 1160):
    	/*
	 * additional config can be injected with -c, make sure it's included
	 * after init_db, which clears the entire config environment.
	 */
	write_config(&option_config);

	/*
	 * re-read config after init_db and write_config to pick up any config
	 * injected by --template and --config, respectively.
	 */
	git_config(git_clone_config, NULL);

so, what I can think of is introducing a new variable for git_clone_config,
and I find that in the other place, "define a new config_xxx variable for
git-config" is usual.

>Which is exactly what we want.  So in short, declare just a single
>
>    static int reject_shallow; /* default to false */
>   
>instead of "option_no_shallow" and "config_shallow", and use it in
>both builtin_clone_options[] given to parse_options, and
>git_clone_config() that is given to git_config(), and you'd be fine,
>I think.
>
>> @@ -963,6 +970,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;
>> @@ -1205,6 +1213,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> 
>>  path = get_repo_path(remote->url[0], &is_bundle);
>>  is_local = option_local != 0 && path && !is_bundle;
>> +
>> +	/* Detect if the remote repository is shallow */
>> +	if (!access(mkpath("%s/shallow", path), F_OK)) {
>> +	is_shallow = 1;
>> +	}
>
>This is only for cloning from a local repository, no?  IOW, path at
>this point may even be "git://github.com/git/git/" and checking with
>access() does not make sense.
>
>Ah, it is even worse.  get_repo_path() can return NULL, so mkpath()
>will crash in such a case.  This must be at least
>
>	if (path && !access(mkpath("%s/shallow", path), F_OK))
>	is_shallow = 1;
>
>but I think the logic fits better in the body of "if (is_Local)"
>thing that immediately follows.  It is specific to the case where
>cloning from a local repository and access(mkpath()) that is about
>the local filesystem (as opposed to going through the transport
>layer) belongs there.
>
>>  if (is_local) {
>>  if (option_depth)
>>  warning(_("--depth is ignored in local clones; use file:// instead."));
>> @@ -1214,7 +1228,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
>>  if (filter_options.choice)
>>  warning(_("--filter is ignored in local clones; use file:// instead."));
>> -	if (!access(mkpath("%s/shallow", path), F_OK)) {
>> +	if (is_shallow) {
>>  if (option_local > 0)
>>  warning(_("source repository is shallow, ignoring --local"));
>>  is_local = 0;
>
>So, I think the above two hunks are making the code worse.  If we
>are to detect and reject cloning from the shallow repository when
>going through the transport layer (i.e. "--no-local" or cloning from
>"git://github.com/git/git", or "https://github.com/git/git", if it
>were a shallow repository), that must be handled separately.
> 
Sorry, I made the question simple. Reject cloning a shallow repository
should apply to all four type transport protocols. There still a bunch of
work to be done.

>> @@ -1222,6 +1236,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  }
>>  if (option_local > 0 && !is_local)
>>  warning(_("--local is ignored"));
>> +
>> +	if (is_shallow) {
>> +	int reject = 0;
>> +
>> +	/* If option_no_shallow is specified from CLI option,
>> +	* ignore config_shallow from git_clone_config.
>> +	*/
>> +
>> +	if (config_shallow != -1) {
>> +	reject = config_shallow;
>> +	}
>> +	if (option_no_shallow != -1) {
>> +	reject = option_no_shallow;
>> +	}
>
>I do not think any of the above is necessary with just a single
>reject_shallow variable that is initialized to 0, can be set by
>git_config() callback, and can further be set by parse_options().
>
>> +	if (reject) {
>> +	die(_("source repository is shallow, reject to clone."));
>> +	}
>
>> +	}
>> +
>>  transport->cloning = 1;
>> 
>>  transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>
>I do not see how this change would allow users to reject cloning
>http://github.com/git/git, if that repository were shallow, though. 
>I think that would need changes to the code that interacts with
>these transport_* functions we see later part of this functrion.
>
>Thanks. 

On the whole, thank you for all your patience and suggestions!
I will dig into the code, and base on your suggestions to figure it out.

Finally, I wish you a Happy Lunar New Year! ^-^

-Lilinchao

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3] builtin/clone.c: add --reject-shallow option
  2021-02-10  9:07       ` lilinchao
@ 2021-02-10 16:27         ` Junio C Hamano
       [not found]         ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
  1 sibling, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-02-10 16:27 UTC (permalink / raw)
  To: lilinchao; +Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, dscho

"lilinchao@oschina.cn" <lilinchao@oschina.cn> writes:

> Sorry, you may forget there is a re-read git-config under these lines

Ahhhh, thanks.  You're right that "clone" is "special" in that way
and requires unusual order---I forgot about that.

>>This is only for cloning from a local repository, no?  IOW, path at
>>this point may even be "git://github.com/git/git/" and checking with
>>access() does not make sense.
>>
>>Ah, it is even worse.  get_repo_path() can return NULL, so mkpath()
>>will crash in such a case.  This must be at least
>>
>>	if (path && !access(mkpath("%s/shallow", path), F_OK))
>>	is_shallow = 1;
>> ...
>>
>>So, I think the above two hunks are making the code worse.  If we
>>are to detect and reject cloning from the shallow repository when
>>going through the transport layer (i.e. "--no-local" or cloning from
>>"git://github.com/git/git", or "https://github.com/git/git", if it
>>were a shallow repository), that must be handled separately.
>> 
> Sorry, I made the question simple. Reject cloning a shallow repository
> should apply to all four type transport protocols. There still a bunch of
> work to be done.

Sorry, but I didn't realize that this is just a work-in-progress
that shows an early "only local transport is covered" preview.  I
think, modulo that the access(mkpath()) thing should be in the "if
(is_local)" block, the patch covers the case of local transport well
enough.  I think by "four types of transport" you mean ssh://, git://
https:// etc., but they all go through the same transport API, so
hopefully once one of them, say, git://, is covered, that would take
us a long way.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v3] builtin/clone.c: add --reject-shallow option
       [not found]         ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
@ 2021-02-20 10:40           ` lilinchao
  0 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-02-20 10:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, dscho




--------------
lilinchao@oschina.cn
>"lilinchao@oschina.cn" <lilinchao@oschina.cn> writes:
>
>> Sorry, you may forget there is a re-read git-config under these lines
>
>Ahhhh, thanks.  You're right that "clone" is "special" in that way
>and requires unusual order---I forgot about that.
>
>>>This is only for cloning from a local repository, no?  IOW, path at
>>>this point may even be "git://github.com/git/git/" and checking with
>>>access() does not make sense.
>>>
>>>Ah, it is even worse.  get_repo_path() can return NULL, so mkpath()
>>>will crash in such a case.  This must be at least
>>>
>>>	if (path && !access(mkpath("%s/shallow", path), F_OK))
>>>	is_shallow = 1;
>>> ...
>>>
>>>So, I think the above two hunks are making the code worse.  If we
>>>are to detect and reject cloning from the shallow repository when
>>>going through the transport layer (i.e. "--no-local" or cloning from
>>>"git://github.com/git/git", or "https://github.com/git/git", if it
>>>were a shallow repository), that must be handled separately.
>>>
>> Sorry, I made the question simple. Reject cloning a shallow repository
>> should apply to all four type transport protocols. There still a bunch of
>> work to be done.
>
>Sorry, but I didn't realize that this is just a work-in-progress
>that shows an early "only local transport is covered" preview.  I
>think, modulo that the access(mkpath()) thing should be in the "if
>(is_local)" block, the patch covers the case of local transport well
>enough.  I think by "four types of transport" you mean ssh://, git://
>https:// etc., but they all go through the same transport API, so
>hopefully once one of them, say, git://, is covered, that would take
>us a long way.
> 
Hi, after some exploration, I found that the following changes
can achieve the goal:
(around at line 1386 in builtin/clone.c)
    /* below part is my changes */
            if (reject_shallow) {
                    if (local_shallow || is_repository_shallow(the_repository)) {
                            die(_("source repository is shallow, reject to clone."));
                    }
            }
    /* end */
            update_remote_refs(refs, mapped_refs, remote_head_points_at,
                               branch_top.buf, reflog_msg.buf, transport,
                               !is_local);

as it shows, using existing method "is_repository_shallow" to detect
whether the repo is shallow or not, before "update_remote_refs"
I made some tests based on this change, and it seems ok.
what do you think about this?


>Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-02-08 14:12   ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
  2021-02-09 20:32     ` Junio C Hamano
       [not found]     ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
@ 2021-02-21  7:05     ` Li Linchao via GitGitGadget
  2021-02-22 18:12       ` Junio C Hamano
                         ` (2 more replies)
  2 siblings, 3 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-02-21  7:05 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

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.

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
    builtin/clone.c: add --reject-shallow option
    
    Changes since v1:
    
     * Rename --no-shallow to --reject-shallow
     * Enable to reject a non-local clone
     * Enable --[no-]reject-shallow from CLI override configuration.
     * Add more testcases.
     * Reword commit messages and relative documentation.
    
    Changes since v3:
    
     * Add support to reject clone shallow repo over https protocol
     * Add testcase to reject clone shallow repo over https:// transport
     * Reword commit messages and relative documentation according
       suggestions from Junio.
    
    Signed-off-by: lilinchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/865

Range-diff vs v3:

 1:  f5ed6b2e4481 ! 1:  ee4fb840a32f builtin/clone.c: add --reject-shallow option
     @@ Commit message
          builtin/clone.c: add --reject-shallow option
      
          In some scenarios, users may want more history than the repository
     -    offered for cloning, which mostly to be a shallow repository, can
     -    give them. But 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.
     +    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
     @@ Commit message
          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, given that we have an API that allow us to import
     -    external repository, and then perform various operations on the repo.
     +    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 an unshallow repository.
     +    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.
     @@ Documentation/git-clone.txt: SYNOPSIS
       	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
       	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
      -	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
     -+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--reject-shallow]
     ++	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
       	  [--filter=<filter>] [--] <repository>
       	  [<directory>]
       
     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
       --no-checkout::
       	No checkout of HEAD is performed after the clone is complete.
       
     -+--reject-shallow::
     -+	Don't clone a shallow source repository. In some scenarios, clients
     -+	want the cloned repository information to be complete. Otherwise,
     -+	the cloning process should end immediately without creating any files,
     -+	which can save some disk space. This can override `clone.rejectshallow`
     -+	from the configuration:
     -+
     -+	--------------------------------------------------------------------
     -+	$ git -c clone.rejectshallow=false clone --reject-shallow source out
     -+	--------------------------------------------------------------------
     -+
     -+	While there is a way to countermand a configured "I always refuse to
     -+	clone from a shallow repository" with "but let's allow it only this time":
     -+
     -+	----------------------------------------------------------------------
     -+	$ git -c clone.rejectshallow=true clone --no-reject-shallow source out
     -+	----------------------------------------------------------------------
     ++--[no-]reject-shallow::
     ++	Fail if the source repository is a shallow repository. The
     ++	'clone.rejectshallow' configuration variable can be used to
     ++	give the default.
      +
       --bare::
       	Make a 'bare' Git repository.  That is, instead of
       	creating `<directory>` and placing the administrative
      
       ## builtin/clone.c ##
     +@@
     + #include "connected.h"
     + #include "packfile.h"
     + #include "list-objects-filter-options.h"
     ++#include "shallow.h"
     + 
     + /*
     +  * Overall FIXMEs:
      @@ builtin/clone.c: 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_no_shallow = -1;  /* unspecified */
     ++static int option_shallow = -1;    /* unspecified */
      +static int config_shallow = -1;    /* unspecified */
       static int deepen;
       static char *option_template, *option_depth, *option_since;
     @@ builtin/clone.c: static struct option builtin_clone_options[] = {
       	OPT__VERBOSITY(&option_verbosity),
       	OPT_BOOL(0, "progress", &option_progress,
       		 N_("force progress reporting")),
     -+	OPT_BOOL(0, "reject-shallow", &option_no_shallow,
     ++	OPT_BOOL(0, "reject-shallow", &option_shallow,
      +		 N_("don't clone shallow repository")),
       	OPT_BOOL('n', "no-checkout", &option_no_checkout,
       		 N_("don't create a checkout")),
     @@ builtin/clone.c: 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;
     ++	int local_shallow = 0;
     ++	int reject_shallow = 0;
       	const char *repo_name, *repo, *work_tree, *git_dir;
       	char *path, *dir, *display_repo = NULL;
       	int dest_exists, real_dest_exists = 0;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	 */
     + 	git_config(git_clone_config, NULL);
       
     - 	path = get_repo_path(remote->url[0], &is_bundle);
     - 	is_local = option_local != 0 && path && !is_bundle;
     -+
     -+	/* Detect if the remote repository is shallow */
     -+	if (!access(mkpath("%s/shallow", path), F_OK)) {
     -+		is_shallow = 1;
     ++	/*
     ++	 * If option_shallow is specified from CLI option,
     ++	 * ignore config_shallow from git_clone_config.
     ++	 */
     ++	if (config_shallow != -1) {
     ++		reject_shallow = config_shallow;
      +	}
     -+
     - 	if (is_local) {
     - 		if (option_depth)
     - 			warning(_("--depth is ignored in local clones; use file:// instead."));
     ++	if (option_shallow != -1) {
     ++		reject_shallow = option_shallow;
     ++	}
     + 	/*
     + 	 * apply the remote name provided by --origin only after this second
     + 	 * call to git_config, to ensure it overrides all config-based values.
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 			warning(_("--shallow-exclude is ignored in local clones; use file:// instead."));
       		if (filter_options.choice)
       			warning(_("--filter is ignored in local clones; use file:// instead."));
     --		if (!access(mkpath("%s/shallow", path), F_OK)) {
     -+		if (is_shallow) {
     + 		if (!access(mkpath("%s/shallow", path), F_OK)) {
     ++			local_shallow = 1;
       			if (option_local > 0)
       				warning(_("source repository is shallow, ignoring --local"));
       			is_local = 0;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 			goto cleanup;
       	}
     - 	if (option_local > 0 && !is_local)
     - 		warning(_("--local is ignored"));
     -+
     -+	if (is_shallow) {
     -+		int reject = 0;
     -+
     -+		/* If option_no_shallow is specified from CLI option,
     -+		 * ignore config_shallow from git_clone_config.
     -+		 */
     -+
     -+		if (config_shallow != -1) {
     -+			reject = config_shallow;
     -+		}
     -+		if (option_no_shallow != -1) {
     -+			reject = option_no_shallow;
     -+		}
     -+		if (reject) {
     + 
     ++	if (reject_shallow) {
     ++		if (local_shallow || is_repository_shallow(the_repository)) {
      +			die(_("source repository is shallow, reject to clone."));
      +		}
      +	}
      +
     - 	transport->cloning = 1;
     - 
     - 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
     + 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
     + 			   branch_top.buf, reflog_msg.buf, transport,
     + 			   !is_local);
      
       ## t/t5606-clone-options.sh ##
     +@@ t/t5606-clone-options.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
     + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     + 
     + . ./test-lib.sh
     ++. "$TEST_DIRECTORY"/lib-httpd.sh
     ++start_httpd
     + 
     + test_expect_success 'setup' '
     + 
      @@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-git-dir' '
       
       '
       
     ++test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
     ++	rm -rf shallow-repo &&
      +	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
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
      +	test_i18ngrep -e "source repository is shallow, reject to clone." err
      +'
      +
     -+test_expect_success 'clone.rejectshallow=false should succeed cloning' '
     ++test_expect_success 'clone.rejectshallow=false should succeed' '
      +	rm -rf child &&
      +	git clone --depth=1 --no-local . child &&
      +	git -c clone.rejectshallow=false clone --no-local child out
      +'
      +
     -+test_expect_success 'clone.rejectshallow=true should succeed cloning normal repo' '
     ++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
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
      +test_expect_success 'option --reject-shallow override clone.rejectshallow' '
      +	rm -rf child out &&
      +	git clone --depth=1 --no-local . child &&
     -+	test_must_fail git clone -c clone.rejectshallow=false  --reject-shallow --no-local child out 2>err &&
     ++	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' '
     ++test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
      +	rm -rf child &&
      +	git clone --depth=1 --no-local . child &&
      +	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out


 Documentation/config/clone.txt |  4 +++
 Documentation/git-clone.txt    |  7 ++++-
 builtin/clone.c                | 27 +++++++++++++++++++
 t/t5606-clone-options.sh       | 47 ++++++++++++++++++++++++++++++++++
 t/t5611-clone-config.sh        | 32 +++++++++++++++++++++++
 5 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..50ebc170bb81 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectshallow::
+	Reject to clone a repository if it is a shallow one, can be overridden by
+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..cb458123eef6 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -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.
+
 --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..6807eb954366 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -32,6 +32,7 @@
 #include "connected.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "shallow.h"
 
 /*
  * Overall FIXMEs:
@@ -50,6 +51,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 */
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -90,6 +93,8 @@ static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
 		 N_("force progress reporting")),
+	OPT_BOOL(0, "reject-shallow", &option_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")),
@@ -858,6 +863,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")) {
+		config_shallow = git_config_bool(k, v);
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -963,6 +971,8 @@ 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 local_shallow = 0;
+	int reject_shallow = 0;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1166,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 */
 	git_config(git_clone_config, NULL);
 
+	/*
+	 * If option_shallow is specified from CLI option,
+	 * ignore config_shallow from git_clone_config.
+	 */
+	if (config_shallow != -1) {
+		reject_shallow = config_shallow;
+	}
+	if (option_shallow != -1) {
+		reject_shallow = option_shallow;
+	}
 	/*
 	 * apply the remote name provided by --origin only after this second
 	 * call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1236,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)) {
+			local_shallow = 1;
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
 			is_local = 0;
@@ -1363,6 +1384,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			goto cleanup;
 	}
 
+	if (reject_shallow) {
+		if (local_shallow || is_repository_shallow(the_repository)) {
+			die(_("source repository is shallow, reject to clone."));
+		}
+	}
+
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local);
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb050..6170d0513227 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
 
 test_expect_success 'setup' '
 
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
+test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
+	rm -rf shallow-repo &&
+	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
+
+'
+
+test_expect_success 'fail to clone 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
+
+'
+
+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
+
+'
+
 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..18268256bfe0 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 fail to clone' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . 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 &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=false clone --no-local child out
+'
+
+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 &&
+	git clone --depth=1 --no-local . child &&
+	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&

base-commit: 2283e0e9af55689215afa39c03beb2315ce18e83
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-02-21  7:05     ` [PATCH v4] " Li Linchao via GitGitGadget
@ 2021-02-22 18:12       ` Junio C Hamano
  2021-03-01 22:03         ` Jonathan Tan
       [not found]       ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
  2021-02-28 18:06       ` [PATCH v5] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-02-22 18:12 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget
  Cc: git, Derrick Stolee, dscho, Li Linchao, Jonathan Tan

[jc: I've CC'ed Jonathan Tan, who is much more knowledgeable than I
am on the transport layer issues, to sanity check my assumption]

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

> @@ -1363,6 +1384,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			goto cleanup;
>  	}
>  
> +	if (reject_shallow) {
> +		if (local_shallow || is_repository_shallow(the_repository)) {

This may reject to clone from a shallow repository, but at this
point the bulk of the tranfer from the origin repository has already
happened, no?  Rejecting after transferring many megabytes feels a
bit too late.  That is one of the reasons why I kept hinting that
the transport layer needs to be taught an option to reject talking
to a shallow counterpart if we want to add this feature [*1*].

Also, wouldn't "clone --depth=1 --reject-shallow" from a repository
that is not shallow make the_repository a shallow one at this point
and makes it fail?  If the goal of the --reject-shallow option were
to make sure the resulting repository is not shallow, then that is a
technically correct implementation (even though it is wasteful to
transfer a full tree worth of megabytes and then abort), but is the
feature is explained to reject cloning from a shallow one, then
users would be suprised to see ...

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

... this message, when cloning from well known publich repositories
that are not shallow.

I think cloning with --depth=<n> when the source repository is deep
enough, should be allowed, so the cleanest solution for the latter
may be to notice the combination of options that make the resulting
repository shallow (I mentioned --depth=<n>, but there may be others)
and the --reject-shallow option and error out before even talking
to the other side at the time we parse the command line.

Thanks.


[Footnote]

*1* Looking at Documentation/technical/pack-protocol.txt, "git
    fetch" seem to learn if the repository is shallow immediately
    upon contacting "upload-pack" during the Reference Discovery
    phase (we'd see 'shallow' packets if they are shallow). I
    suspect that the right solution would be to teach the codepath
    on the "git fetch" side that accepts, parses, and acts on this
    packet to optionally stop communication and error out when the
    caller asks not to talk with a shallow repository.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
       [not found]       ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
@ 2021-02-28 17:58         ` lilinchao
  0 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-02-28 17:58 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget
  Cc: git, Derrick Stolee, dscho, Jonathan Tan


--------------
lilinchao@oschina.cn
>[jc: I've CC'ed Jonathan Tan, who is much more knowledgeable than I
>am on the transport layer issues, to sanity check my assumption]
>
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -1363,6 +1384,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  goto cleanup;
>>  }
>> 
>> +	if (reject_shallow) {
>> +	if (local_shallow || is_repository_shallow(the_repository)) {
>
>This may reject to clone from a shallow repository, but at this
>point the bulk of the tranfer from the origin repository has already
>happened, no?  Rejecting after transferring many megabytes feels a
>bit too late.  That is one of the reasons why I kept hinting that
>the transport layer needs to be taught an option to reject talking
>to a shallow counterpart if we want to add this feature [*1*].
>
>Also, wouldn't "clone --depth=1 --reject-shallow" from a repository
>that is not shallow make the_repository a shallow one at this point
>and makes it fail?  If the goal of the --reject-shallow option were
>to make sure the resulting repository is not shallow, then that is a
>technically correct implementation (even though it is wasteful to
>transfer a full tree worth of megabytes and then abort), but is the
>feature is explained to reject cloning from a shallow one, then
>users would be suprised to see ...
>
>> +	die(_("source repository is shallow, reject to clone."));
>
>... this message, when cloning from well known publich repositories
>that are not shallow.
> 
Uh, IMO the goal of this new option is not to make sure the cloned repo
is not shallow, but to prevent(just as optional) the remote repo is shallow, 
we still allow the resulting repo is shallow by using "--depth" option.
so, if we apply "clone --depth=1 --reject-shallow=true" to a clone process,
the expected result is a shallow repo.
Oh, wait, what if we apply "--depth=1" to a remote shallow repo, in other word,
shallow a remote shallow repo? then the result will not be what we expected.
This can be confusing.

>I think cloning with --depth=<n> when the source repository is deep
>enough, should be allowed, so the cleanest solution for the latter
>may be to notice the combination of options that make the resulting
>repository shallow (I mentioned --depth=<n>, but there may be others)
>and the --reject-shallow option and error out before even talking
>to the other side at the time we parse the command line.
>
>Thanks.
>
>
>[Footnote]
>
>*1* Looking at Documentation/technical/pack-protocol.txt, "git
>    fetch" seem to learn if the repository is shallow immediately
>    upon contacting "upload-pack" during the Reference Discovery
>    phase (we'd see 'shallow' packets if they are shallow). I
>    suspect that the right solution would be to teach the codepath
>    on the "git fetch" side that accepts, parses, and acts on this
>    packet to optionally stop communication and error out when the
>    caller asks not to talk with a shallow repository. 

I took the time to update the patch as you suggested there, it may look
imperfect, I only tested the local protocol, and the smart protocols,
not dumb protocol, and not protocol version1 yet. 
Hope to get some suggestions from you.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-02-21  7:05     ` [PATCH v4] " Li Linchao via GitGitGadget
  2021-02-22 18:12       ` Junio C Hamano
       [not found]       ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
@ 2021-02-28 18:06       ` Li Linchao via GitGitGadget
  2021-03-01  7:11         ` lilinchao
                           ` (2 more replies)
  2 siblings, 3 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-02-28 18:06 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee, dscho, Li Linchao, lilinchao

From: lilinchao <lilinchao@oschina.cn>

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.

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
    builtin/clone.c: add --reject-shallow option
    
    Changes since v1:
    
     * Rename --no-shallow to --reject-shallow
     * Enable to reject a non-local clone
     * Enable --[no-]reject-shallow from CLI override configuration.
     * Add more testcases.
     * Reword commit messages and relative documentation.
    
    Changes since v3:
    
     * Add support to reject clone shallow repo over https protocol
     * Add testcase to reject clone shallow repo over https:// transport
     * Reword commit messages and relative documentation according
       suggestions from Junio.
    
    Signed-off-by: lilinchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/865

Range-diff vs v4:

 1:  ee4fb840a32f ! 1:  3f765e49e4a7 builtin/clone.c: add --reject-shallow option
     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
       	creating `<directory>` and placing the administrative
      
       ## builtin/clone.c ##
     -@@
     - #include "connected.h"
     - #include "packfile.h"
     - #include "list-objects-filter-options.h"
     -+#include "shallow.h"
     - 
     - /*
     -  * Overall FIXMEs:
      @@ builtin/clone.c: 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;
     @@ builtin/clone.c: 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 local_shallow = 0;
      +	int reject_shallow = 0;
       	const char *repo_name, *repo, *work_tree, *git_dir;
       	char *path, *dir, *display_repo = NULL;
     @@ builtin/clone.c: 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)) {
     -+			local_shallow = 1;
     ++			if (reject_shallow)
     ++				die("source repository is shallow, reject to clone.");
       			if (option_local > 0)
       				warning(_("source repository is shallow, ignoring --local"));
       			is_local = 0;
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 			goto cleanup;
     - 	}
       
     -+	if (reject_shallow) {
     -+		if (local_shallow || is_repository_shallow(the_repository)) {
     -+			die(_("source repository is shallow, reject to clone."));
     -+		}
     -+	}
     + 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
     + 
     ++	if (reject_shallow)
     ++		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
     + 	if (option_depth)
     + 		transport_set_option(transport, TRANS_OPT_DEPTH,
     + 				     option_depth);
     +
     + ## fetch-pack.c ##
     +@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     + 
     + 	if (args->stateless_rpc)
     + 		packet_flush(fd[1]);
     ++
     ++	if (!args->deepen && args->remote_shallow)
     ++		die("source repository is shallow, reject to clone.");
      +
     - 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
     - 			   branch_top.buf, reflog_msg.buf, transport,
     - 			   !is_local);
     + 	if (args->deepen)
     + 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
     + 					NULL);
     +@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
     + 		 * shallow. In v0, remote refs that reach these objects are
     + 		 * rejected (unless --update-shallow is set); do the same.
     + 		 */
     ++		if (args->remote_shallow)
     ++			die("source repository is shallow, reject to clone.");
     + 		prepare_shallow_info(si, shallows);
     + 		if (si->nr_ours || si->nr_theirs)
     + 			alternate_shallow_file =
     +
     + ## fetch-pack.h ##
     +@@ fetch-pack.h: struct fetch_pack_args {
     + 	unsigned self_contained_and_connected:1;
     + 	unsigned cloning:1;
     + 	unsigned update_shallow:1;
     ++	unsigned remote_shallow:1;
     + 	unsigned deepen:1;
     + 
     + 	/*
      
       ## t/t5606-clone-options.sh ##
      @@ t/t5606-clone-options.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
      +'
      +
      +test_expect_success 'clone.rejectshallow=false should succeed' '
     -+	rm -rf child &&
     ++	rm -rf child out &&
      +	git clone --depth=1 --no-local . child &&
      +	git -c clone.rejectshallow=false clone --no-local child out
      +'
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
      +'
      +
      +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
     -+	rm -rf child &&
     ++	rm -rf child out &&
      +	git clone --depth=1 --no-local . child &&
      +	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
      +'
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
       test_expect_success MINGW 'clone -c core.hideDotFiles' '
       	test_commit attributes .gitattributes "" &&
       	rm -rf child &&
     +
     + ## transport-helper.c ##
     +@@ transport-helper.c: static const char *boolean_options[] = {
     + 	TRANS_OPT_THIN,
     + 	TRANS_OPT_KEEP,
     + 	TRANS_OPT_FOLLOWTAGS,
     +-	TRANS_OPT_DEEPEN_RELATIVE
     ++	TRANS_OPT_DEEPEN_RELATIVE,
     ++	TRANS_OPT_REJECT_SHALLOW
     + 	};
     + 
     + static int strbuf_set_helper_option(struct helper_data *data,
     +
     + ## transport.c ##
     +@@ transport.c: 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;
     ++		return 0;
     + 	}
     + 	return 1;
     + }
     +@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
     + 	args.stateless_rpc = transport->stateless_rpc;
     + 	args.server_options = transport->server_options;
     + 	args.negotiation_tips = data->options.negotiation_tips;
     ++	args.remote_shallow = transport->smart_options->reject_shallow;
     + 
     + 	if (!data->got_remote_heads) {
     + 		int i;
     +
     + ## transport.h ##
     +@@ transport.h: struct git_transport_options {
     + 	unsigned check_self_contained_and_connected : 1;
     + 	unsigned self_contained_and_connected : 1;
     + 	unsigned update_shallow : 1;
     ++	unsigned reject_shallow : 1;
     + 	unsigned deepen_relative : 1;
     + 
     + 	/* see documentation of corresponding flag in fetch-pack.h */
     +@@ transport.h: void transport_check_allowed(const char *type);
     + /* Aggressively fetch annotated tags if possible */
     + #define TRANS_OPT_FOLLOWTAGS "followtags"
     + 
     ++/* reject shallow repo transport  */
     ++#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
     ++
     + /* Accept refs that may update .git/shallow without --depth */
     + #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
     + 


 Documentation/config/clone.txt |  4 +++
 Documentation/git-clone.txt    |  7 ++++-
 builtin/clone.c                | 22 ++++++++++++++++
 fetch-pack.c                   |  6 +++++
 fetch-pack.h                   |  1 +
 t/t5606-clone-options.sh       | 47 ++++++++++++++++++++++++++++++++++
 t/t5611-clone-config.sh        | 32 +++++++++++++++++++++++
 transport-helper.c             |  3 ++-
 transport.c                    |  4 +++
 transport.h                    |  4 +++
 10 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..50ebc170bb81 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectshallow::
+	Reject to clone a repository if it is a shallow one, can be overridden by
+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..cb458123eef6 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -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.
+
 --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..70d8ca77988f 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 */
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
 		 N_("force progress reporting")),
+	OPT_BOOL(0, "reject-shallow", &option_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")),
@@ -858,6 +862,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")) {
+		config_shallow = git_config_bool(k, v);
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -963,6 +970,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 reject_shallow = 0;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 */
 	git_config(git_clone_config, NULL);
 
+	/*
+	 * If option_shallow is specified from CLI option,
+	 * ignore config_shallow from git_clone_config.
+	 */
+	if (config_shallow != -1) {
+		reject_shallow = config_shallow;
+	}
+	if (option_shallow != -1) {
+		reject_shallow = option_shallow;
+	}
 	/*
 	 * apply the remote name provided by --origin only after this second
 	 * call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1234,8 @@ 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)) {
+			if (reject_shallow)
+				die("source repository is shallow, reject to clone.");
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
 			is_local = 0;
@@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
+	if (reject_shallow)
+		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
 	if (option_depth)
 		transport_set_option(transport, TRANS_OPT_DEPTH,
 				     option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index 1eaedcb5dc2e..41ef700bde38 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1071,6 +1071,10 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if (args->stateless_rpc)
 		packet_flush(fd[1]);
+
+	if (!args->deepen && args->remote_shallow)
+		die("source repository is shallow, reject to clone.");
+
 	if (args->deepen)
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		 * shallow. In v0, remote refs that reach these objects are
 		 * rejected (unless --update-shallow is set); do the same.
 		 */
+		if (args->remote_shallow)
+			die("source repository is shallow, reject to clone.");
 		prepare_shallow_info(si, shallows);
 		if (si->nr_ours || si->nr_theirs)
 			alternate_shallow_file =
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..6e4f8f0d738c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
 	unsigned self_contained_and_connected:1;
 	unsigned cloning:1;
 	unsigned update_shallow:1;
+	unsigned remote_shallow:1;
 	unsigned deepen:1;
 
 	/*
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb050..6170d0513227 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
 
 test_expect_success 'setup' '
 
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
+test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
+	rm -rf shallow-repo &&
+	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
+
+'
+
+test_expect_success 'fail to clone 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
+
+'
+
+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
+
+'
+
 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..da10d3f10352 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 fail to clone' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . 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
+'
+
+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
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&
diff --git a/transport-helper.c b/transport-helper.c
index 49b7fb4dcb9a..b57b55c8180c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -265,7 +265,8 @@ static const char *boolean_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP,
 	TRANS_OPT_FOLLOWTAGS,
-	TRANS_OPT_DEEPEN_RELATIVE
+	TRANS_OPT_DEEPEN_RELATIVE,
+	TRANS_OPT_REJECT_SHALLOW
 	};
 
 static int strbuf_set_helper_option(struct helper_data *data,
diff --git a/transport.c b/transport.c
index b13fab5dc3b1..34fe01221ee0 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;
+		return 0;
 	}
 	return 1;
 }
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.stateless_rpc = transport->stateless_rpc;
 	args.server_options = transport->server_options;
 	args.negotiation_tips = data->options.negotiation_tips;
+	args.remote_shallow = transport->smart_options->reject_shallow;
 
 	if (!data->got_remote_heads) {
 		int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..f6273d268b09 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
+	unsigned reject_shallow : 1;
 	unsigned deepen_relative : 1;
 
 	/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
+/* reject shallow repo transport  */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
 /* Accept refs that may update .git/shallow without --depth */
 #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
 

base-commit: 225365fb5195e804274ab569ac3cc4919451dc7f
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-02-28 18:06       ` [PATCH v5] " Li Linchao via GitGitGadget
@ 2021-03-01  7:11         ` lilinchao
  2021-03-01 22:40           ` Johannes Schindelin
  2021-03-03 23:21         ` Junio C Hamano
  2021-03-04 17:19         ` [PATCH v6] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 31+ messages in thread
From: lilinchao @ 2021-03-01  7:11 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget, git; +Cc: Junio C Hamano, Derrick Stolee, dscho




--------------
lilinchao@oschina.cn
>From: lilinchao <lilinchao@oschina.cn>
>
>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.
>
>Signed-off-by: lilinchao <lilinchao@oschina.cn>
>---
>    builtin/clone.c: add --reject-shallow option
>   
>    Changes since v1:
>   
>     * Rename --no-shallow to --reject-shallow
>     * Enable to reject a non-local clone
>     * Enable --[no-]reject-shallow from CLI override configuration.
>     * Add more testcases.
>     * Reword commit messages and relative documentation.
>   
>    Changes since v3:
>   
>     * Add support to reject clone shallow repo over https protocol
> * Add testcase to reject clone shallow repo over https:// transport
>     * Reword commit messages and relative documentation according
>       suggestions from Junio.
>   
>    Signed-off-by: lilinchao lilinchao@oschina.cn
>
>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v5
>Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v5
>Pull-Request: https://github.com/gitgitgadget/git/pull/865
>
>Range-diff vs v4:
>
> 1:  ee4fb840a32f ! 1:  3f765e49e4a7 builtin/clone.c: add --reject-shallow option
>     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
>       creating `<directory>` and placing the administrative
>     
>       ## builtin/clone.c ##
>     -@@
>     - #include "connected.h"
>     - #include "packfile.h"
>     - #include "list-objects-filter-options.h"
>     -+#include "shallow.h"
>     -
>     - /*
>     -  * Overall FIXMEs:
>      @@ builtin/clone.c: 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;
>     @@ builtin/clone.c: 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 local_shallow = 0;
>      +	int reject_shallow = 0;
>       const char *repo_name, *repo, *work_tree, *git_dir;
>       char *path, *dir, *display_repo = NULL;
>     @@ builtin/clone.c: 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)) {
>     -+	local_shallow = 1;
>     ++	if (reject_shallow)
>     ++	die("source repository is shallow, reject to clone.");
>       if (option_local > 0)
>       warning(_("source repository is shallow, ignoring --local"));
>       is_local = 0;
>      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
>     - goto cleanup;
>     - }
>      
>     -+	if (reject_shallow) {
>     -+	if (local_shallow || is_repository_shallow(the_repository)) {
>     -+	die(_("source repository is shallow, reject to clone."));
>     -+	}
>     -+	}
>     + transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>     +
>     ++	if (reject_shallow)
>     ++	transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>     + if (option_depth)
>     + transport_set_option(transport, TRANS_OPT_DEPTH,
>     +      option_depth);
>     +
>     + ## fetch-pack.c ##
>     +@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>     +
>     + if (args->stateless_rpc)
>     + packet_flush(fd[1]);
>     ++
>     ++	if (!args->deepen && args->remote_shallow)
>     ++	die("source repository is shallow, reject to clone.");
>      +
>     - update_remote_refs(refs, mapped_refs, remote_head_points_at,
>     -    branch_top.buf, reflog_msg.buf, transport,
>     -    !is_local);
>     + if (args->deepen)
>     + setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>     + NULL);
>     +@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
>     + * shallow. In v0, remote refs that reach these objects are
>     + * rejected (unless --update-shallow is set); do the same.
>     + */
>     ++	if (args->remote_shallow)
>     ++	die("source repository is shallow, reject to clone.");
>     + prepare_shallow_info(si, shallows);
>     + if (si->nr_ours || si->nr_theirs)
>     + alternate_shallow_file =
>     +
>     + ## fetch-pack.h ##
>     +@@ fetch-pack.h: struct fetch_pack_args {
>     + unsigned self_contained_and_connected:1;
>     + unsigned cloning:1;
>     + unsigned update_shallow:1;
>     ++	unsigned remote_shallow:1;
>     + unsigned deepen:1;
>     +
>     + /*
>     
>       ## t/t5606-clone-options.sh ##
>      @@ t/t5606-clone-options.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
>      +'
>      +
>      +test_expect_success 'clone.rejectshallow=false should succeed' '
>     -+	rm -rf child &&
>     ++	rm -rf child out &&
>      +	git clone --depth=1 --no-local . child &&
>      +	git -c clone.rejectshallow=false clone --no-local child out
>      +'
>     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
>      +'
>      +
>      +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
>     -+	rm -rf child &&
>     ++	rm -rf child out &&
>      +	git clone --depth=1 --no-local . child &&
>      +	git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
>      +'
>     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
>       test_expect_success MINGW 'clone -c core.hideDotFiles' '
>       test_commit attributes .gitattributes "" &&
>       rm -rf child &&
>     +
>     + ## transport-helper.c ##
>     +@@ transport-helper.c: static const char *boolean_options[] = {
>     + TRANS_OPT_THIN,
>     + TRANS_OPT_KEEP,
>     + TRANS_OPT_FOLLOWTAGS,
>     +-	TRANS_OPT_DEEPEN_RELATIVE
>     ++	TRANS_OPT_DEEPEN_RELATIVE,
>     ++	TRANS_OPT_REJECT_SHALLOW
>     + };
>     +
>     + static int strbuf_set_helper_option(struct helper_data *data,
>     +
>     + ## transport.c ##
>     +@@ transport.c: 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;
>     ++	return 0;
>     + }
>     + return 1;
>     + }
>     +@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
>     + args.stateless_rpc = transport->stateless_rpc;
>     + args.server_options = transport->server_options;
>     + args.negotiation_tips = data->options.negotiation_tips;
>     ++	args.remote_shallow = transport->smart_options->reject_shallow;
>     +
>     + if (!data->got_remote_heads) {
>     + int i;
>     +
>     + ## transport.h ##
>     +@@ transport.h: struct git_transport_options {
>     + unsigned check_self_contained_and_connected : 1;
>     + unsigned self_contained_and_connected : 1;
>     + unsigned update_shallow : 1;
>     ++	unsigned reject_shallow : 1;
>     + unsigned deepen_relative : 1;
>     +
>     + /* see documentation of corresponding flag in fetch-pack.h */
>     +@@ transport.h: void transport_check_allowed(const char *type);
>     + /* Aggressively fetch annotated tags if possible */
>     + #define TRANS_OPT_FOLLOWTAGS "followtags"
>     +
>     ++/* reject shallow repo transport  */
>     ++#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
>     ++
>     + /* Accept refs that may update .git/shallow without --depth */
>     + #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
>     +
>
>
> Documentation/config/clone.txt |  4 +++
> Documentation/git-clone.txt    |  7 ++++-
> builtin/clone.c                | 22 ++++++++++++++++
> fetch-pack.c                   |  6 +++++
> fetch-pack.h                   |  1 +
> t/t5606-clone-options.sh       | 47 ++++++++++++++++++++++++++++++++++
> t/t5611-clone-config.sh        | 32 +++++++++++++++++++++++
> transport-helper.c             |  3 ++-
> transport.c                    |  4 +++
> transport.h                    |  4 +++
> 10 files changed, 128 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>index 47de36a5fedf..50ebc170bb81 100644
>--- a/Documentation/config/clone.txt
>+++ b/Documentation/config/clone.txt
>@@ -2,3 +2,7 @@ clone.defaultRemoteName::
> The name of the remote to create when cloning a repository.  Defaults to
> `origin`, and can be overridden by passing the `--origin` command-line
> option to linkgit:git-clone[1].
>+
>+clone.rejectshallow::
>+	Reject to clone a repository if it is a shallow one, can be overridden by
>+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>index 02d9c19cec75..cb458123eef6 100644
>--- a/Documentation/git-clone.txt
>+++ b/Documentation/git-clone.txt
>@@ -15,7 +15,7 @@ SYNOPSIS
>   [--dissociate] [--separate-git-dir <git dir>]
>   [--depth <depth>] [--[no-]single-branch] [--no-tags]
>   [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
>-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
>+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
>   [--filter=<filter>] [--] <repository>
>   [<directory>]
>
>@@ -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.
>+
> --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..70d8ca77988f 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 */
> static int deepen;
> static char *option_template, *option_depth, *option_since;
> static char *option_origin = NULL;
>@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
> OPT__VERBOSITY(&option_verbosity),
> OPT_BOOL(0, "progress", &option_progress,
> N_("force progress reporting")),
>+	OPT_BOOL(0, "reject-shallow", &option_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")),
>@@ -858,6 +862,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")) {
>+	config_shallow = git_config_bool(k, v);
>+	}
> return git_default_config(k, v, cb);
> }
>
>@@ -963,6 +970,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 reject_shallow = 0;
> const char *repo_name, *repo, *work_tree, *git_dir;
> char *path, *dir, *display_repo = NULL;
> int dest_exists, real_dest_exists = 0;
>@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> */
> git_config(git_clone_config, NULL);
>
>+	/*
>+	* If option_shallow is specified from CLI option,
>+	* ignore config_shallow from git_clone_config.
>+	*/
>+	if (config_shallow != -1) {
>+	reject_shallow = config_shallow;
>+	}
>+	if (option_shallow != -1) {
>+	reject_shallow = option_shallow;
>+	}
> /*
> * apply the remote name provided by --origin only after this second
> * call to git_config, to ensure it overrides all config-based values.
>@@ -1216,6 +1234,8 @@ 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)) {
>+	if (reject_shallow)
>+	die("source repository is shallow, reject to clone.");
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
>@@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>
>+	if (reject_shallow)
>+	transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
> if (option_depth)
> transport_set_option(transport, TRANS_OPT_DEPTH,
>      option_depth);
>diff --git a/fetch-pack.c b/fetch-pack.c
>index 1eaedcb5dc2e..41ef700bde38 100644
>--- a/fetch-pack.c
>+++ b/fetch-pack.c
>@@ -1071,6 +1071,10 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>
> if (args->stateless_rpc)
> packet_flush(fd[1]);
>+
>+	if (!args->deepen && args->remote_shallow)
>+	die("source repository is shallow, reject to clone.");
>+ 
I am not sure if we should apply this to non-protocol-v2.

> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
>@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> * shallow. In v0, remote refs that reach these objects are
> * rejected (unless --update-shallow is set); do the same.
> */
>+	if (args->remote_shallow)
>+	die("source repository is shallow, reject to clone."); 

I just found that Johannes Schindelin wrote a document 14 year ago
in Documentation/technical/shallow.txt:

"There are some unfinished ends of the whole shallow business:

A special handling of a shallow upstream is needed. At some stage,
upload-pack has to check if it sends a shallow commit, and it should
send that information early (or fail, if the client does not support 
shallow repositories). There is no support at all for this in this patch
series."

It seems that my patch can sovle his worry in some degree,
and maybe we could warn client in fetch-pack stage, if we don't
choose to reject shallow cloning.

		if (args->remote_shallow)
			die("source repository is shallow, reject to clone.");
		else
			warning("remote source repository is shallow.");

> prepare_shallow_info(si, shallows);
> if (si->nr_ours || si->nr_theirs)
> alternate_shallow_file =
>diff --git a/fetch-pack.h b/fetch-pack.h
>index 736a3dae467a..6e4f8f0d738c 100644
>--- a/fetch-pack.h
>+++ b/fetch-pack.h
>@@ -39,6 +39,7 @@ struct fetch_pack_args {
> unsigned self_contained_and_connected:1;
> unsigned cloning:1;
> unsigned update_shallow:1;
>+	unsigned remote_shallow:1;
> unsigned deepen:1;
>
> /*
>diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>index 52e5789fb050..6170d0513227 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
>
> test_expect_success 'setup' '
>
>@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
> '
>
>+test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
>+	rm -rf shallow-repo &&
>+	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
>+
>+'
>+
>+test_expect_success 'fail to clone 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
>+
>+'
>+
>+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
>+
>+'
>+
> 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..da10d3f10352 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 fail to clone' '
>+	rm -rf child &&
>+	git clone --depth=1 --no-local . 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
>+'
>+
>+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
>+'
>+
> test_expect_success MINGW 'clone -c core.hideDotFiles' '
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
>diff --git a/transport-helper.c b/transport-helper.c
>index 49b7fb4dcb9a..b57b55c8180c 100644
>--- a/transport-helper.c
>+++ b/transport-helper.c
>@@ -265,7 +265,8 @@ static const char *boolean_options[] = {
> TRANS_OPT_THIN,
> TRANS_OPT_KEEP,
> TRANS_OPT_FOLLOWTAGS,
>-	TRANS_OPT_DEEPEN_RELATIVE
>+	TRANS_OPT_DEEPEN_RELATIVE,
>+	TRANS_OPT_REJECT_SHALLOW
> };
>
> static int strbuf_set_helper_option(struct helper_data *data,
>diff --git a/transport.c b/transport.c
>index b13fab5dc3b1..34fe01221ee0 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;
>+	return 0;
> }
> return 1;
> }
>@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> args.stateless_rpc = transport->stateless_rpc;
> args.server_options = transport->server_options;
> args.negotiation_tips = data->options.negotiation_tips;
>+	args.remote_shallow = transport->smart_options->reject_shallow;
>
> if (!data->got_remote_heads) {
> int i;
>diff --git a/transport.h b/transport.h
>index 24e15799e714..f6273d268b09 100644
>--- a/transport.h
>+++ b/transport.h
>@@ -14,6 +14,7 @@ struct git_transport_options {
> unsigned check_self_contained_and_connected : 1;
> unsigned self_contained_and_connected : 1;
> unsigned update_shallow : 1;
>+	unsigned reject_shallow : 1;
> unsigned deepen_relative : 1;
>
> /* see documentation of corresponding flag in fetch-pack.h */
>@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
> /* Aggressively fetch annotated tags if possible */
> #define TRANS_OPT_FOLLOWTAGS "followtags"
>
>+/* reject shallow repo transport  */
>+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
>+
> /* Accept refs that may update .git/shallow without --depth */
> #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
>
>
>base-commit: 225365fb5195e804274ab569ac3cc4919451dc7f
>--
>gitgitgadget
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-02-22 18:12       ` Junio C Hamano
@ 2021-03-01 22:03         ` Jonathan Tan
  2021-03-01 22:34           ` Junio C Hamano
                             ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Jonathan Tan @ 2021-03-01 22:03 UTC (permalink / raw)
  To: gitster
  Cc: gitgitgadget, git, stolee, johannes.schindelin, lilinchao, jonathantanmy

> This may reject to clone from a shallow repository, but at this
> point the bulk of the tranfer from the origin repository has already
> happened, no?  Rejecting after transferring many megabytes feels a
> bit too late.  That is one of the reasons why I kept hinting that
> the transport layer needs to be taught an option to reject talking
> to a shallow counterpart if we want to add this feature [*1*].

Extending the transport layer in this way might not be too difficult in
the case of native (SSH, git://) protocols and using protocol v0, since
handshake() in transport.c (called indirectly from
transport_get_remote_refs()) writes shallow information to a data
structure that we could potentially expose for the caller to use (before
it calls transport_fetch_refs(). I couldn't see how remote-using
protocols (e.g. HTTP) communicate shallow information, though
(remote-curl.c seems to just keep it for itself), so that will be a more
difficult task. And of course there's the matter of protocol v2, which I
discuss below.

> [Footnote]
> 
> *1* Looking at Documentation/technical/pack-protocol.txt, "git
>     fetch" seem to learn if the repository is shallow immediately
>     upon contacting "upload-pack" during the Reference Discovery
>     phase (we'd see 'shallow' packets if they are shallow). I
>     suspect that the right solution would be to teach the codepath
>     on the "git fetch" side that accepts, parses, and acts on this
>     packet to optionally stop communication and error out when the
>     caller asks not to talk with a shallow repository.

This is true with protocol v0, but protocol v2 bundles all shallow
information (whether coming from the fact that the remote is shallow or
the fact that the fetcher specified --depth etc.) and sends them
together with the packfile. It may be possible to stop packfile download
(saving bandwidth on the client side, at least) once such information is
returned, though.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-03-01 22:03         ` Jonathan Tan
@ 2021-03-01 22:34           ` Junio C Hamano
  2021-03-02  8:44           ` lilinchao
  2021-03-03 23:59           ` Junio C Hamano
  2 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-03-01 22:34 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: gitgitgadget, git, stolee, johannes.schindelin, lilinchao

Jonathan Tan <jonathantanmy@google.com> writes:

>> This may reject to clone from a shallow repository, but at this
>> point the bulk of the tranfer from the origin repository has already
>> happened, no?  Rejecting after transferring many megabytes feels a
>> bit too late.  That is one of the reasons why I kept hinting that
>> the transport layer needs to be taught an option to reject talking
>> to a shallow counterpart if we want to add this feature [*1*].
>
> Extending the transport layer in this way might not be too difficult in
> the case of native (SSH, git://) protocols and using protocol v0, since
> handshake() in transport.c (called indirectly from
> transport_get_remote_refs()) writes shallow information to a data
> structure that we could potentially expose for the caller to use (before
> it calls transport_fetch_refs(). I couldn't see how remote-using
> protocols (e.g. HTTP) communicate shallow information, though
> (remote-curl.c seems to just keep it for itself), so that will be a more
> difficult task. And of course there's the matter of protocol v2, which I
> discuss below.
>
>> [Footnote]
>> 
>> *1* Looking at Documentation/technical/pack-protocol.txt, "git
>>     fetch" seem to learn if the repository is shallow immediately
>>     upon contacting "upload-pack" during the Reference Discovery
>>     phase (we'd see 'shallow' packets if they are shallow). I
>>     suspect that the right solution would be to teach the codepath
>>     on the "git fetch" side that accepts, parses, and acts on this
>>     packet to optionally stop communication and error out when the
>>     caller asks not to talk with a shallow repository.
>
> This is true with protocol v0, but protocol v2 bundles all shallow
> information (whether coming from the fact that the remote is shallow or
> the fact that the fetcher specified --depth etc.) and sends them
> together with the packfile. It may be possible to stop packfile download
> (saving bandwidth on the client side, at least) once such information is
> returned, though.

So in short, the "we do not want to clone from a shallow upstream"
would not be possible to implement sensibly without significantly
cleaning up the protocol layers first, which makes the whole thing
pretty much moot.

Thanks for a review and insight.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-03-01  7:11         ` lilinchao
@ 2021-03-01 22:40           ` Johannes Schindelin
  2021-03-04  6:26             ` lilinchao
  0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-03-01 22:40 UTC (permalink / raw)
  To: lilinchao
  Cc: Li Linchao via GitGitGadget, git, Junio C Hamano, Derrick Stolee

[-- Attachment #1: Type: text/plain, Size: 8366 bytes --]

Hi,

On Mon, 1 Mar 2021, lilinchao@oschina.cn wrote:

> >@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> > * shallow. In v0, remote refs that reach these objects are
> > * rejected (unless --update-shallow is set); do the same.
> > */
> >+	if (args->remote_shallow)
> >+	die("source repository is shallow, reject to clone.");
>
> I just found that Johannes Schindelin wrote a document 14 year ago
> in Documentation/technical/shallow.txt:
>
> "There are some unfinished ends of the whole shallow business:
>
> A special handling of a shallow upstream is needed. At some stage,
> upload-pack has to check if it sends a shallow commit, and it should
> send that information early (or fail, if the client does not support 
> shallow repositories). There is no support at all for this in this patch
> series."

Oh wow, what a blast from the past.

I do agree that your patch is an improvement over the current situation.

Thanks,
Johannes

> It seems that my patch can sovle his worry in some degree,
> and maybe we could warn client in fetch-pack stage, if we don't
> choose to reject shallow cloning.
>
> 		if (args->remote_shallow)
> 			die("source repository is shallow, reject to clone.");
> 		else
> 			warning("remote source repository is shallow.");
>
> > prepare_shallow_info(si, shallows);
> > if (si->nr_ours || si->nr_theirs)
> > alternate_shallow_file =
> >diff --git a/fetch-pack.h b/fetch-pack.h
> >index 736a3dae467a..6e4f8f0d738c 100644
> >--- a/fetch-pack.h
> >+++ b/fetch-pack.h
> >@@ -39,6 +39,7 @@ struct fetch_pack_args {
> > unsigned self_contained_and_connected:1;
> > unsigned cloning:1;
> > unsigned update_shallow:1;
> >+	unsigned remote_shallow:1;
> > unsigned deepen:1;
> >
> > /*
> >diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> >index 52e5789fb050..6170d0513227 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
> >
> > test_expect_success 'setup' '
> >
> >@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
> >
> > '
> >
> >+test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
> >+	rm -rf shallow-repo &&
> >+	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
> >+
> >+'
> >+
> >+test_expect_success 'fail to clone 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
> >+
> >+'
> >+
> >+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
> >+
> >+'
> >+
> > 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..da10d3f10352 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 fail to clone' '
> >+	rm -rf child &&
> >+	git clone --depth=1 --no-local . 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
> >+'
> >+
> >+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
> >+'
> >+
> > test_expect_success MINGW 'clone -c core.hideDotFiles' '
> > test_commit attributes .gitattributes "" &&
> > rm -rf child &&
> >diff --git a/transport-helper.c b/transport-helper.c
> >index 49b7fb4dcb9a..b57b55c8180c 100644
> >--- a/transport-helper.c
> >+++ b/transport-helper.c
> >@@ -265,7 +265,8 @@ static const char *boolean_options[] = {
> > TRANS_OPT_THIN,
> > TRANS_OPT_KEEP,
> > TRANS_OPT_FOLLOWTAGS,
> >-	TRANS_OPT_DEEPEN_RELATIVE
> >+	TRANS_OPT_DEEPEN_RELATIVE,
> >+	TRANS_OPT_REJECT_SHALLOW
> > };
> >
> > static int strbuf_set_helper_option(struct helper_data *data,
> >diff --git a/transport.c b/transport.c
> >index b13fab5dc3b1..34fe01221ee0 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;
> >+	return 0;
> > }
> > return 1;
> > }
> >@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> > args.stateless_rpc = transport->stateless_rpc;
> > args.server_options = transport->server_options;
> > args.negotiation_tips = data->options.negotiation_tips;
> >+	args.remote_shallow = transport->smart_options->reject_shallow;
> >
> > if (!data->got_remote_heads) {
> > int i;
> >diff --git a/transport.h b/transport.h
> >index 24e15799e714..f6273d268b09 100644
> >--- a/transport.h
> >+++ b/transport.h
> >@@ -14,6 +14,7 @@ struct git_transport_options {
> > unsigned check_self_contained_and_connected : 1;
> > unsigned self_contained_and_connected : 1;
> > unsigned update_shallow : 1;
> >+	unsigned reject_shallow : 1;
> > unsigned deepen_relative : 1;
> >
> > /* see documentation of corresponding flag in fetch-pack.h */
> >@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
> > /* Aggressively fetch annotated tags if possible */
> > #define TRANS_OPT_FOLLOWTAGS "followtags"
> >
> >+/* reject shallow repo transport  */
> >+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
> >+
> > /* Accept refs that may update .git/shallow without --depth */
> > #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
> >
> >
> >base-commit: 225365fb5195e804274ab569ac3cc4919451dc7f
> >--
> >gitgitgadget
> >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-03-01 22:03         ` Jonathan Tan
  2021-03-01 22:34           ` Junio C Hamano
@ 2021-03-02  8:44           ` lilinchao
  2021-03-03 23:59           ` Junio C Hamano
  2 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-03-02  8:44 UTC (permalink / raw)
  To: Jonathan Tan, Junio C Hamano
  Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, dscho, Jonathan Tan


--------------
lilinchao
>> This may reject to clone from a shallow repository, but at this
>> point the bulk of the tranfer from the origin repository has already
>> happened, no?  Rejecting after transferring many megabytes feels a
>> bit too late.  That is one of the reasons why I kept hinting that
>> the transport layer needs to be taught an option to reject talking
>> to a shallow counterpart if we want to add this feature [*1*].
>
>Extending the transport layer in this way might not be too difficult in
>the case of native (SSH, git://) protocols and using protocol v0, since
>handshake() in transport.c (called indirectly from
>transport_get_remote_refs()) writes shallow information to a data
>structure that we could potentially expose for the caller to use (before
>it calls transport_fetch_refs(). I couldn't see how remote-using
>protocols (e.g. HTTP) communicate shallow information, though
>(remote-curl.c seems to just keep it for itself), so that will be a more
>difficult task. And of course there's the matter of protocol v2, which I
>discuss below.
> 
These discussions were based on PATCH_v4, which is quiet immature then,
In the latest PATCH_v5, for the case of native protocols(ssh, git, file://), they
will eventually call do_fetch_pack_v2() if it's protocol v2,  and then will call
receive_shallow_info() for the case FETCH_GET_PACK, so this is the place
I made the change.
As for HTTPs protocl, as long as it's still smart protocol, which means do not
fallback to dumb protocol, it will also call do_fetch_pack_v2(), and go to 
"check shallow info" trigger in receive_shallow_info().

So, base on PATCH_V5, I have tested protocol v2, which goes like this:

First, I created a shallow repo on gitlab and gitee respectively.

Then tried to clone them in my terminal, (in order not to look too verbose,
I ommited the result when GIT_TRACE_PACKET is on).

$ ./git-clone --reject-shallow https://gitlab.com/Cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

$ ./git-clone --reject-shallow ssh://gitlab.com/Cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

$ ./git-clone --reject-shallow git@gitlab.com:Cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

$ ./git-clone --reject-shallow https://gitee.com/cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

$ ./git-clone --reject-shallow ssh://gitee.com/cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

$ ./git-clone --reject-shallow git@gitee.com:cactusinhand/rugged.git
Cloning into 'rugged'...
fatal: source repository is shallow, reject to clone.

I haven't tested protocol v1, but I've made the change in do_fetch_pack(),
which is for protocol version 0 and version 1.

I hope you can review the latest patch, and give me some suggestions.

Thanks!

>> [Footnote]
>>
>> *1* Looking at Documentation/technical/pack-protocol.txt, "git
>>     fetch" seem to learn if the repository is shallow immediately
>>     upon contacting "upload-pack" during the Reference Discovery
>>     phase (we'd see 'shallow' packets if they are shallow). I
>>     suspect that the right solution would be to teach the codepath
>>     on the "git fetch" side that accepts, parses, and acts on this
>>     packet to optionally stop communication and error out when the
>>     caller asks not to talk with a shallow repository.
>
>This is true with protocol v0, but protocol v2 bundles all shallow
>information (whether coming from the fact that the remote is shallow or
>the fact that the fetcher specified --depth etc.) and sends them
>together with the packfile. It may be possible to stop packfile download
>(saving bandwidth on the client side, at least) once such information is
>returned, though.
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-02-28 18:06       ` [PATCH v5] " Li Linchao via GitGitGadget
  2021-03-01  7:11         ` lilinchao
@ 2021-03-03 23:21         ` Junio C Hamano
  2021-03-04  5:50           ` lilinchao
  2021-03-04 17:19         ` [PATCH v6] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-03-03 23:21 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho, Li Linchao

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

> diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
> index 47de36a5fedf..50ebc170bb81 100644
> --- a/Documentation/config/clone.txt
> +++ b/Documentation/config/clone.txt
> @@ -2,3 +2,7 @@ clone.defaultRemoteName::
>  	The name of the remote to create when cloning a repository.  Defaults to
>  	`origin`, and can be overridden by passing the `--origin` command-line
>  	option to linkgit:git-clone[1].
> +
> +clone.rejectshallow::
> +	Reject to clone a repository if it is a shallow one, can be overridden by
> +	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]

Let's camelCase this, i.e. "clone.rejectShallow", as this file would
be a good candidate to be the authoritative record of canonical
spelling of these variables.

cf. https://lore.kernel.org/git/xmqq7dmy389l.fsf@gitster.g/

> +--[no-]reject-shallow::
> +	Fail if the source repository is a shallow repository. The
> +	'clone.rejectshallow' configuration variable can be used to
> +	give the default.

Let's camelCase the reference to the variable, too.  Also, typeset
in monospace, i.e.

	The `clone.rejectShallow` configuration variable ...

> @@ -858,6 +862,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")) {
> +		config_shallow = git_config_bool(k, v);
> +	}

No need to use {} around a single-statement block, especially when
the "if" statement does not have an "else" block.

The use of strcmp() against the variable name in all lowercase is
correct here.

> @@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	 */
>  	git_config(git_clone_config, NULL);
>  
> +	/*
> +	 * If option_shallow is specified from CLI option,
> +	 * ignore config_shallow from git_clone_config.
> +	 */
> +	if (config_shallow != -1) {
> +		reject_shallow = config_shallow;
> +	}
> +	if (option_shallow != -1) {
> +		reject_shallow = option_shallow;
> +	}

Needless use of {} around single-statement blocks.

As reject_shallow is initialized to 0, this lets the option to be of
the most priority, then the config (presumably coming from the per-user
or per-system configuration), by without them, defaults to false.  Good.

We'll have an extra git_config() call later, but that one will only
read into config_shallow, never to be looked at because we will use
reject_shallow variable anyway.  OK.

> @@ -1216,6 +1234,8 @@ 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)) {
> +			if (reject_shallow)
> +				die("source repository is shallow, reject to clone.");

With the local transport, it (hopefully) is trivial to see if the
source is shallow.  OK.

>  			if (option_local > 0)
>  				warning(_("source repository is shallow, ignoring --local"));
>  			is_local = 0;
> @@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>  
> +	if (reject_shallow)
> +		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>  	if (option_depth)
>  		transport_set_option(transport, TRANS_OPT_DEPTH,
>  				     option_depth);

OK.  What is really interesting will all happen inside the transport
layer; the caller only has to ask for it.

The asymmetry with other options like "--depth" stands out and
puzzles readers, though.

Do we really want to add the clone.rejectShallow configuration?
After all, we do not give "clone.depth = 1" etc., and that is the
reason why we only need "if (option_depth)" here in the near-by
code.

I'd stop here for today, hoping that somebody much more familiar
with the transport layer than I am will review and comment on the
changes there.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-03-01 22:03         ` Jonathan Tan
  2021-03-01 22:34           ` Junio C Hamano
  2021-03-02  8:44           ` lilinchao
@ 2021-03-03 23:59           ` Junio C Hamano
  2021-03-04  1:53             ` Jonathan Tan
  2 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-03-03 23:59 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: gitgitgadget, git, stolee, johannes.schindelin, lilinchao

Jonathan Tan <jonathantanmy@google.com> writes:

> This is true with protocol v0, but protocol v2 bundles all shallow
> information (whether coming from the fact that the remote is shallow or
> the fact that the fetcher specified --depth etc.) and sends them
> together with the packfile.

By the above do you mean what happens in FETCH_GET_PACK arm where
receive_shallow_info() is called when "shallow-info" header is seen,
before the code continues to process wanted-refs, packfile-uris and
then finally the packfile?

I do not think it makes much sense to ask any option to make us
shallow (like --depth=<n>) while --reject-shallow is in use (after
all, if the other side is deep enough to make us <n> commits deep,
there is no reason to reject the other side as the source), so your
"whether coming from the fact ..." part, while is a valid
observation, can be ignored in practice (meaning: it is OK to make
"--reject-shallow" be in effect only when we are trying to make a
full clone, and reject combinations of it with --depth=<n> and such
at the command parsing time).

> It may be possible to stop packfile download (saving bandwidth on
> the client side, at least) once such information is returned,
> though.

Just like "upload-pack" does not get upset by a client that comes
only for the initial refs advertisement and disconnects without
asking for any "want" (aka "ls-remote"), the server side should be
prepared to see if the other side cuts off after seeing the
"shallow-info" section header or after seeing the the whole
"shallow-info" section, so we should be able to leave early without
having to download the bulk data.  If the "upload-pack" spends
unnecessary cycles when it happens, then we need to fix that.  Even
if the "fetch" client does not willingly disconnect in the middle,
the network disconnect may happen at any point in the exchange, and
we'd need to be prepared for it.

Do we need to read and parse the "shallow-info" section, or would
the mere presense of the section mean the other side knows this side
needs to futz with the .git/shallow information (either because we
asked to be made shallow with --depth and the like, or because we
tried to clone from them and they are shallow)?

Thanks.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4] builtin/clone.c: add --reject-shallow option
  2021-03-03 23:59           ` Junio C Hamano
@ 2021-03-04  1:53             ` Jonathan Tan
  0 siblings, 0 replies; 31+ messages in thread
From: Jonathan Tan @ 2021-03-04  1:53 UTC (permalink / raw)
  To: gitster
  Cc: jonathantanmy, gitgitgadget, git, stolee, johannes.schindelin, lilinchao

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > This is true with protocol v0, but protocol v2 bundles all shallow
> > information (whether coming from the fact that the remote is shallow or
> > the fact that the fetcher specified --depth etc.) and sends them
> > together with the packfile.
> 
> By the above do you mean what happens in FETCH_GET_PACK arm where
> receive_shallow_info() is called when "shallow-info" header is seen,
> before the code continues to process wanted-refs, packfile-uris and
> then finally the packfile?

Yes.

> I do not think it makes much sense to ask any option to make us
> shallow (like --depth=<n>) while --reject-shallow is in use (after
> all, if the other side is deep enough to make us <n> commits deep,
> there is no reason to reject the other side as the source), so your
> "whether coming from the fact ..." part, while is a valid
> observation, can be ignored in practice (meaning: it is OK to make
> "--reject-shallow" be in effect only when we are trying to make a
> full clone, and reject combinations of it with --depth=<n> and such
> at the command parsing time).
> 
> > It may be possible to stop packfile download (saving bandwidth on
> > the client side, at least) once such information is returned,
> > though.
> 
> Just like "upload-pack" does not get upset by a client that comes
> only for the initial refs advertisement and disconnects without
> asking for any "want" (aka "ls-remote"), the server side should be
> prepared to see if the other side cuts off after seeing the
> "shallow-info" section header or after seeing the the whole
> "shallow-info" section, so we should be able to leave early without
> having to download the bulk data.  If the "upload-pack" spends
> unnecessary cycles when it happens, then we need to fix that.  Even
> if the "fetch" client does not willingly disconnect in the middle,
> the network disconnect may happen at any point in the exchange, and
> we'd need to be prepared for it.
> 
> Do we need to read and parse the "shallow-info" section, or would
> the mere presense of the section mean the other side knows this side
> needs to futz with the .git/shallow information (either because we
> asked to be made shallow with --depth and the like, or because we
> tried to clone from them and they are shallow)?

Reading the documentation, the mere presence should be enough. Yes, I
think upload-pack will spend unnecessary cycles if the channel is
terminated halfway (and I don't know if we can prevent spending these
cycles, since I/O can be buffered). I think it should be possible for
the client to cut off when it sees shallow-info (besides the possible
wastage of cycles and I/O on the server's end).

Having said that, I think this different from the ls-remote case. There,
the server is awaiting another request from the user before sending more
information, but here, the server intends to send everything at once.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-03-03 23:21         ` Junio C Hamano
@ 2021-03-04  5:50           ` lilinchao
  0 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-03-04  5:50 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho

>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>> index 47de36a5fedf..50ebc170bb81 100644
>> --- a/Documentation/config/clone.txt
>> +++ b/Documentation/config/clone.txt
>> @@ -2,3 +2,7 @@ clone.defaultRemoteName::
>>  The name of the remote to create when cloning a repository.  Defaults to
>>  `origin`, and can be overridden by passing the `--origin` command-line
>>  option to linkgit:git-clone[1].
>> +
>> +clone.rejectshallow::
>> +	Reject to clone a repository if it is a shallow one, can be overridden by
>> +	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>
>Let's camelCase this, i.e. "clone.rejectShallow", as this file would
>be a good candidate to be the authoritative record of canonical
>spelling of these variables.
>
>cf. https://lore.kernel.org/git/xmqq7dmy389l.fsf@gitster.g/

OK,thanks for reminding this.

>
>> +--[no-]reject-shallow::
>> +	Fail if the source repository is a shallow repository. The
>> +	'clone.rejectshallow' configuration variable can be used to
>> +	give the default.
>
>Let's camelCase the reference to the variable, too.  Also, typeset
>in monospace, i.e.
>
>	The `clone.rejectShallow` configuration variable ...
>
>> @@ -858,6 +862,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")) {
>> +	config_shallow = git_config_bool(k, v);
>> +	}
>
>No need to use {} around a single-statement block, especially when
>the "if" statement does not have an "else" block.
>
>The use of strcmp() against the variable name in all lowercase is
>correct here.
>
>> @@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  */
>>  git_config(git_clone_config, NULL);
>> 
>> +	/*
>> +	* If option_shallow is specified from CLI option,
>> +	* ignore config_shallow from git_clone_config.
>> +	*/
>> +	if (config_shallow != -1) {
>> +	reject_shallow = config_shallow;
>> +	}
>> +	if (option_shallow != -1) {
>> +	reject_shallow = option_shallow;
>> +	}
>
>Needless use of {} around single-statement blocks.
>
>As reject_shallow is initialized to 0, this lets the option to be of
>the most priority, then the config (presumably coming from the per-user
>or per-system configuration), by without them, defaults to false.  Good.
>
>We'll have an extra git_config() call later, but that one will only
>read into config_shallow, never to be looked at because we will use
>reject_shallow variable anyway.  OK.
>
>> @@ -1216,6 +1234,8 @@ 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)) {
>> +	if (reject_shallow)
>> +	die("source repository is shallow, reject to clone.");
>
>With the local transport, it (hopefully) is trivial to see if the
>source is shallow.  OK.
>
>>  if (option_local > 0)
>>  warning(_("source repository is shallow, ignoring --local"));
>>  is_local = 0;
>> @@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> 
>>  transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>> 
>> +	if (reject_shallow)
>> +	transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>>  if (option_depth)
>>  transport_set_option(transport, TRANS_OPT_DEPTH,
>>       option_depth);
>
>OK.  What is really interesting will all happen inside the transport
>layer; the caller only has to ask for it.
>
>The asymmetry with other options like "--depth" stands out and
>puzzles readers, though.
> 

True, but since `reject_shallow` is not just an option from CLI, so I think
we can't just call it "option_shallow" here to order to keep symmetry
with other options below.

>Do we really want to add the clone.rejectShallow configuration? 

Only when I want all my git clone in my machine can avoid cloning
shallow repo, and no need to provide the option "--reject-shallow"
every time, then this configuration would make sense.

>After all, we do not give "clone.depth = 1" etc., and that is the
>reason why we only need "if (option_depth)" here in the near-by
>code.
>
>I'd stop here for today, hoping that somebody much more familiar
>with the transport layer than I am will review and comment on the
>changes there.
>
>Thanks.
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
  2021-03-01 22:40           ` Johannes Schindelin
@ 2021-03-04  6:26             ` lilinchao
  0 siblings, 0 replies; 31+ messages in thread
From: lilinchao @ 2021-03-04  6:26 UTC (permalink / raw)
  To: dscho; +Cc: Li Linchao via GitGitGadget, git, Junio C Hamano, Derrick Stolee

>Hi,
>
>On Mon, 1 Mar 2021, lilinchao@oschina.cn wrote:
>
>> >@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>> > * shallow. In v0, remote refs that reach these objects are
>> > * rejected (unless --update-shallow is set); do the same.
>> > */
>> >+	if (args->remote_shallow)
>> >+	die("source repository is shallow, reject to clone.");
>>
>> I just found that Johannes Schindelin wrote a document 14 year ago
>> in Documentation/technical/shallow.txt:
>>
>> "There are some unfinished ends of the whole shallow business:
>>
>> A special handling of a shallow upstream is needed. At some stage,
>> upload-pack has to check if it sends a shallow commit, and it should
>> send that information early (or fail, if the client does not support 
>> shallow repositories). There is no support at all for this in this patch
>> series."
>
>Oh wow, what a blast from the past.
>
>I do agree that your patch is an improvement over the current situation. 
> 

Thanks. Glad to hear the voice from you, hope I can get some suggestions
from you too, especially on the transport part.

>Thanks,
>Johannes
>
>> It seems that my patch can sovle his worry in some degree,
>> and maybe we could warn client in fetch-pack stage, if we don't
>> choose to reject shallow cloning.
>>
>> if (args->remote_shallow)
>> die("source repository is shallow, reject to clone.");
>> else
>> warning("remote source repository is shallow.");
>> 




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v6] builtin/clone.c: add --reject-shallow option
  2021-02-28 18:06       ` [PATCH v5] " Li Linchao via GitGitGadget
  2021-03-01  7:11         ` lilinchao
  2021-03-03 23:21         ` Junio C Hamano
@ 2021-03-04 17:19         ` Li Linchao via GitGitGadget
  2 siblings, 0 replies; 31+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-03-04 17:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, Li Linchao,
	lilinchao

From: lilinchao <lilinchao@oschina.cn>

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.

Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
    builtin/clone.c: add --reject-shallow option
    
    Changes since v1:
    
     * Rename --no-shallow to --reject-shallow
     * Enable to reject a non-local clone
     * Enable --[no-]reject-shallow from CLI override configuration.
     * Add more testcases.
     * Reword commit messages and relative documentation.
    
    Changes since v3:
    
     * Add support to reject clone shallow repo over https protocol
     * Add testcase to reject clone shallow repo over https:// transport
     * Reword commit messages and relative documentation according
       suggestions from Junio.
    
    Changes since v5:
    
     * camelcase config variable
     * warning client that source repo is shallow
     * better support ssh:// and git:// protocol v1, v2
    
    Signed-off-by: lilinchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/865

Range-diff vs v5:

 1:  3f765e49e4a7 ! 1:  953122588ca8 builtin/clone.c: add --reject-shallow option
     @@ Documentation/config/clone.txt: clone.defaultRemoteName::
       	`origin`, and can be overridden by passing the `--origin` command-line
       	option to linkgit:git-clone[1].
      +
     -+clone.rejectshallow::
     ++clone.rejectShallow::
      +	Reject to clone a repository if it is a shallow one, can be overridden by
      +	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
      
     @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
       	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
     ++	Fail if the source repository is a shallow repository.
     ++	The 'clone.rejectShallow' configuration variable can be used to
      +	give the default.
      +
       --bare::
     @@ builtin/clone.c: static int git_clone_config(const char *k, const char *v, void
       		free(remote_name);
       		remote_name = xstrdup(v);
       	}
     -+	if (!strcmp(k, "clone.rejectshallow")) {
     ++	if (!strcmp(k, "clone.rejectshallow"))
      +		config_shallow = git_config_bool(k, v);
     -+	}
     ++
       	return git_default_config(k, v, cb);
       }
       
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      +	 * If option_shallow is specified from CLI option,
      +	 * ignore config_shallow from git_clone_config.
      +	 */
     -+	if (config_shallow != -1) {
     ++	if (config_shallow != -1)
      +		reject_shallow = config_shallow;
     -+	}
     -+	if (option_shallow != -1) {
     ++
     ++	if (option_shallow != -1)
      +		reject_shallow = option_shallow;
     -+	}
     ++
       	/*
       	 * apply the remote name provided by --origin only after this second
       	 * call to git_config, to ensure it overrides all config-based values.
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       		if (!access(mkpath("%s/shallow", path), F_OK)) {
      +			if (reject_shallow)
      +				die("source repository is shallow, reject to clone.");
     ++			else
     ++				warning("source repository is shallow.");
       			if (option_local > 0)
       				warning(_("source repository is shallow, ignoring --local"));
       			is_local = 0;
     @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
       
       	if (args->stateless_rpc)
       		packet_flush(fd[1]);
     -+
     -+	if (!args->deepen && args->remote_shallow)
     -+		die("source repository is shallow, reject to clone.");
      +
       	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)
     ++			die("source repository is shallow, reject to clone.");
     ++		else
     ++			warning("source repository is shallow.");
     + 		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))
      @@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
     - 		 * shallow. In v0, remote refs that reach these objects are
       		 * rejected (unless --update-shallow is set); do the same.
       		 */
     -+		if (args->remote_shallow)
     -+			die("source repository is shallow, reject to clone.");
       		prepare_shallow_info(si, shallows);
     - 		if (si->nr_ours || si->nr_theirs)
     +-		if (si->nr_ours || si->nr_theirs)
     ++		if (si->nr_ours || si->nr_theirs) {
     ++			if (args->remote_shallow)
     ++				die("source repository is shallow, reject to clone.");
     ++			else
     ++				warning("source repository is shallow.");
       			alternate_shallow_file =
     + 				setup_temporary_shallow(si->shallow);
     +-		else
     ++		} else
     + 			alternate_shallow_file = NULL;
     + 	} else {
     + 		alternate_shallow_file = NULL;
      
       ## fetch-pack.h ##
      @@ fetch-pack.h: struct fetch_pack_args {
     @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
       	test_commit attributes .gitattributes "" &&
       	rm -rf child &&
      
     - ## transport-helper.c ##
     -@@ transport-helper.c: static const char *boolean_options[] = {
     - 	TRANS_OPT_THIN,
     - 	TRANS_OPT_KEEP,
     - 	TRANS_OPT_FOLLOWTAGS,
     --	TRANS_OPT_DEEPEN_RELATIVE
     -+	TRANS_OPT_DEEPEN_RELATIVE,
     -+	TRANS_OPT_REJECT_SHALLOW
     - 	};
     - 
     - static int strbuf_set_helper_option(struct helper_data *data,
     -
       ## transport.c ##
      @@ transport.c: static int set_git_option(struct git_transport_options *opts,
       		list_objects_filter_die_if_populated(&opts->filter_options);
     @@ transport.h: void transport_check_allowed(const char *type);
       /* Aggressively fetch annotated tags if possible */
       #define TRANS_OPT_FOLLOWTAGS "followtags"
       
     -+/* reject shallow repo transport  */
     ++/* Reject shallow repo transport */
      +#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
      +
       /* Accept refs that may update .git/shallow without --depth */


 Documentation/config/clone.txt |  4 +++
 Documentation/git-clone.txt    |  7 ++++-
 builtin/clone.c                | 24 +++++++++++++++++
 fetch-pack.c                   | 17 +++++++++---
 fetch-pack.h                   |  1 +
 t/t5606-clone-options.sh       | 47 ++++++++++++++++++++++++++++++++++
 t/t5611-clone-config.sh        | 32 +++++++++++++++++++++++
 transport.c                    |  4 +++
 transport.h                    |  4 +++
 9 files changed, 135 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ clone.defaultRemoteName::
 	The name of the remote to create when cloning a repository.  Defaults to
 	`origin`, and can be overridden by passing the `--origin` command-line
 	option to linkgit:git-clone[1].
+
+clone.rejectShallow::
+	Reject to clone a repository if it is a shallow one, can be overridden by
+	passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
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
@@ -15,7 +15,7 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
 	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
-	  [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
 	  [--filter=<filter>] [--] <repository>
 	  [<directory>]
 
@@ -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.
+
 --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..5c64837e8f7b 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 */
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
 		 N_("force progress reporting")),
+	OPT_BOOL(0, "reject-shallow", &option_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")),
@@ -858,6 +862,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"))
+		config_shallow = git_config_bool(k, v);
+
 	return git_default_config(k, v, cb);
 }
 
@@ -963,6 +970,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 reject_shallow = 0;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 */
 	git_config(git_clone_config, NULL);
 
+	/*
+	 * If option_shallow is specified from CLI option,
+	 * ignore config_shallow from git_clone_config.
+	 */
+	if (config_shallow != -1)
+		reject_shallow = config_shallow;
+
+	if (option_shallow != -1)
+		reject_shallow = option_shallow;
+
 	/*
 	 * apply the remote name provided by --origin only after this second
 	 * call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1234,10 @@ 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)) {
+			if (reject_shallow)
+				die("source repository is shallow, reject to clone.");
+			else
+				warning("source repository is shallow.");
 			if (option_local > 0)
 				warning(_("source repository is shallow, ignoring --local"));
 			is_local = 0;
@@ -1227,6 +1249,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
+	if (reject_shallow)
+		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
 	if (option_depth)
 		transport_set_option(transport, TRANS_OPT_DEPTH,
 				     option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index 0cb59acc4866..860ff45d46e7 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1126,12 +1126,17 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 
 	if (args->stateless_rpc)
 		packet_flush(fd[1]);
+
 	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)
+			die("source repository is shallow, reject to clone.");
+		else
+			warning("source repository is shallow.");
 		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))
@@ -1498,10 +1503,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 		 * rejected (unless --update-shallow is set); do the same.
 		 */
 		prepare_shallow_info(si, shallows);
-		if (si->nr_ours || si->nr_theirs)
+		if (si->nr_ours || si->nr_theirs) {
+			if (args->remote_shallow)
+				die("source repository is shallow, reject to clone.");
+			else
+				warning("source repository is shallow.");
 			alternate_shallow_file =
 				setup_temporary_shallow(si->shallow);
-		else
+		} else
 			alternate_shallow_file = NULL;
 	} else {
 		alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..6e4f8f0d738c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
 	unsigned self_contained_and_connected:1;
 	unsigned cloning:1;
 	unsigned update_shallow:1;
+	unsigned remote_shallow:1;
 	unsigned deepen:1;
 
 	/*
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb050..6170d0513227 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
 
 test_expect_success 'setup' '
 
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
+test_expect_success 'fail to clone 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 'fail to clone shallow repository' '
+	rm -rf shallow-repo &&
+	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
+
+'
+
+test_expect_success 'fail to clone 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
+
+'
+
+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
+
+'
+
 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..da10d3f10352 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 fail to clone' '
+	rm -rf child &&
+	git clone --depth=1 --no-local . 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
+'
+
+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
+'
+
 test_expect_success MINGW 'clone -c core.hideDotFiles' '
 	test_commit attributes .gitattributes "" &&
 	rm -rf child &&
diff --git a/transport.c b/transport.c
index b13fab5dc3b1..34fe01221ee0 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;
+		return 0;
 	}
 	return 1;
 }
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.stateless_rpc = transport->stateless_rpc;
 	args.server_options = transport->server_options;
 	args.negotiation_tips = data->options.negotiation_tips;
+	args.remote_shallow = transport->smart_options->reject_shallow;
 
 	if (!data->got_remote_heads) {
 		int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
+	unsigned reject_shallow : 1;
 	unsigned deepen_relative : 1;
 
 	/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
 /* Aggressively fetch annotated tags if possible */
 #define TRANS_OPT_FOLLOWTAGS "followtags"
 
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
 /* Accept refs that may update .git/shallow without --depth */
 #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
 

base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2021-03-04 17:21 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-04  3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
2021-02-04  5:50 ` Junio C Hamano
2021-02-04 10:32   ` lilinchao
2021-02-04 18:36     ` Junio C Hamano
2021-02-04 14:00 ` Johannes Schindelin
2021-02-04 18:24   ` Junio C Hamano
2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
2021-02-08  8:31   ` [PATCH v2 1/2] " lilinchao via GitGitGadget
2021-02-08  8:31   ` [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option lilinchao via GitGitGadget
2021-02-08 13:33   ` [PATCH v2 0/2] builtin/clone.c: add --no-shallow option Derrick Stolee
     [not found]   ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
2021-02-08 13:48     ` lilinchao
2021-02-08 14:12   ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
2021-02-09 20:32     ` Junio C Hamano
     [not found]     ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
2021-02-10  9:07       ` lilinchao
2021-02-10 16:27         ` Junio C Hamano
     [not found]         ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
2021-02-20 10:40           ` lilinchao
2021-02-21  7:05     ` [PATCH v4] " Li Linchao via GitGitGadget
2021-02-22 18:12       ` Junio C Hamano
2021-03-01 22:03         ` Jonathan Tan
2021-03-01 22:34           ` Junio C Hamano
2021-03-02  8:44           ` lilinchao
2021-03-03 23:59           ` Junio C Hamano
2021-03-04  1:53             ` Jonathan Tan
     [not found]       ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
2021-02-28 17:58         ` lilinchao
2021-02-28 18:06       ` [PATCH v5] " Li Linchao via GitGitGadget
2021-03-01  7:11         ` lilinchao
2021-03-01 22:40           ` Johannes Schindelin
2021-03-04  6:26             ` lilinchao
2021-03-03 23:21         ` Junio C Hamano
2021-03-04  5:50           ` lilinchao
2021-03-04 17:19         ` [PATCH v6] " Li Linchao via GitGitGadget

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git