| Message ID | pull.865.git.1612409491842.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/clone.c: add --no-shallow option | expand |
"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.
-------------- 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 :)
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
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.
"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.
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 &&