From: lilinchao@oschina.cn Date: Thu, 04 Feb 2021 10:32:52 GMT Subject: Re: Re: [PATCH] builtin/clone.c: add --no-shallow option Message-ID: <57d5526c66d411eb81800024e87935e7@oschina.cn> In-Reply-To: -------------- lilinchao@oschina.cn >"Li Linchao via GitGitGadget" writes: > >> From: lilinchao >> >> 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..fetch= --origin=' ' >>  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 :)