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

"Li Linchao via GitGitGadget" [off-list ref] writes:

From: lilinchao <redacted>

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

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

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.

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

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

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.

--------------
lilinchao@oschina.cn

"Li Linchao via GitGitGadget" [off-list ref] writes:

quoted

From: lilinchao <redacted>

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.

quoted

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.

quoted

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.

quoted

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

"scenarios" (no 'r').

quoted

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. 

quoted

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

quoted

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

quoted

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.

quoted

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