Re: [PATCH] builtin/clone.c: add --no-shallow option
From: Junio C Hamano <hidden>
Date: 2021-02-04 05:51:11
"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').
quoted hunk
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 ;-)
quoted hunk
@@ -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.
quoted hunk
@@ -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 ...
quoted hunk
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 hunk
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.