* [PATCH] submodule deinit: require '--all' instead of '.' for all submodules @ 2016-05-03 22:11 Stefan Beller 2016-05-03 22:25 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Stefan Beller @ 2016-05-03 22:11 UTC (permalink / raw) To: gitster; +Cc: git, Jens.Lehmann, jrnieder, cederp, Stefan Beller The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails: > "submodule deinit ." may have "worked" in the sense that you would > have at least one path in your tree and avoided this "nothing > matches" most of the time. It would have still failed with the > exactly same error if run in an empty repository, i.e. > > $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E" > $ git init > $ rungit v2.6.6 submodule deinit . > error: pathspec '.' did not match any file(s) known to git. > Did you forget to 'git add'? > $ >file && git add file > $ rungit v2.6.6 submodule deinit . > $ echo $? > 0 So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository. There is no need to update the documentation as it did not describe the special case '.' to remove all submodules. The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in that case. [1] http://news.gmane.org/gmane.comp.version-control.git/289535 Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- This is the result of the discussion, I would think. I developed it on top of "submodule deinit test: fix broken && chain in subshell" on top of 2a4c8c36a7f6, 2016-03-24, Merge branch 'sb/submodule-module-list-pathspec-fix' but I think this would rather go in as a new feature, not on top of a bugfix topic, so I think this could go on origin/master ? Thanks, Stefan git-submodule.sh | 12 ++++++++++-- t/t7400-submodule-basic.sh | 4 ++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 43c68de..301cd0d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -521,6 +521,7 @@ cmd_init() cmd_deinit() { # parse $args after "submodule ... deinit". + deinit_all= while test $# -ne 0 do case "$1" in @@ -530,6 +531,9 @@ cmd_deinit() -q|--quiet) GIT_QUIET=1 ;; + -a|--all) + deinit_all=t + ;; --) shift break @@ -544,9 +548,13 @@ cmd_deinit() shift done - if test $# = 0 + if test -n "$deinit_all" && test "$#" -ne 0 + then + die "$(eval_gettext "'--all' is incompatible with pathspec arguments")" + fi + if test $# = 0 && test -z "$deinit_all" then - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" fi git submodule--helper list --prefix "$wt_prefix" "$@" | diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index cf06b2f..a5b0dec 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -11,6 +11,10 @@ subcommands of git submodule. . ./test-lib.sh +test_expect_success 'submodule deinit works on empty repository' ' + git submodule deinit --all +' + test_expect_success 'setup - initial commit' ' >t && git add t && -- 2.8.0.rc4.10.geb92688.dirty ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:11 [PATCH] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller @ 2016-05-03 22:25 ` Junio C Hamano 2016-05-03 22:28 ` Junio C Hamano 2016-05-04 0:33 ` [PATCHv2] " Stefan Beller 2 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2016-05-03 22:25 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Jens.Lehmann, jrnieder, cederp Stefan Beller <sbeller@google.com> writes: > ... > So instead of a path spec add a parameter '--all' to deinit all submodules > and add a test to check for the corner case of an empty repository. > > There is no need to update the documentation as it did not describe the > special case '.' to remove all submodules. Now there is, isn't it? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:11 [PATCH] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller 2016-05-03 22:25 ` Junio C Hamano @ 2016-05-03 22:28 ` Junio C Hamano 2016-05-03 22:43 ` Stefan Beller 2016-05-04 0:33 ` [PATCHv2] " Stefan Beller 2 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-05-03 22:28 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Jens.Lehmann, jrnieder, cederp Stefan Beller <sbeller@google.com> writes: > I developed it on top of > "submodule deinit test: fix broken && chain in subshell" > on top of 2a4c8c36a7f6, 2016-03-24, Merge branch > 'sb/submodule-module-list-pathspec-fix' > but I think this would rather go in as a new feature, not on top > of a bugfix topic, so I think this could go on origin/master ? I do not particularly view it as a new feature. The way the old message suggested did not work in a pathological corner case, but we wanted to keep the "force user to be explicit when doing mass destruction", and a fix we happened to chose requires addition of a new option--that would still look like a fix to me. It is not like we are forbidding the use of "submodule deinit ." that used to work in a tree with at least one tracked path. With the change, a script that has such a command will continue to work, no? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:28 ` Junio C Hamano @ 2016-05-03 22:43 ` Stefan Beller 2016-05-03 22:52 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Stefan Beller @ 2016-05-03 22:43 UTC (permalink / raw) To: Junio C Hamano Cc: git@vger.kernel.org, Jens Lehmann, Jonathan Nieder, Per Cederqvist On Tue, May 3, 2016 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> I developed it on top of >> "submodule deinit test: fix broken && chain in subshell" >> on top of 2a4c8c36a7f6, 2016-03-24, Merge branch >> 'sb/submodule-module-list-pathspec-fix' >> but I think this would rather go in as a new feature, not on top >> of a bugfix topic, so I think this could go on origin/master ? > > I do not particularly view it as a new feature. The way the old > message suggested did not work in a pathological corner case, but we > wanted to keep the "force user to be explicit when doing mass > destruction", and a fix we happened to chose requires addition of a > new option--that would still look like a fix to me. > > It is not like we are forbidding the use of "submodule deinit ." > that used to work in a tree with at least one tracked path. With > the change, a script that has such a command will continue to work, > no? Maybe. With just this patch, yes. I'd like to revert submodule-module-list-pathspec-fix partially when redoing the groups support. That would break the '.' script case. So eventually scripts want to use git submodule deinit -f --all instead of git submodule deinit -f . When implementing the groups support, I'd change module_list in a way that you can give names, paths, or labels to it. In case of a user gives 'COPYIN*' we'd want to know if that is a path (or a name, label) or bogus, so I think we'd tighten the checks there just for the functionality not just performance as originally anticipated for the order of S_ISGITLINK and match_pathspec. So eventually (i.e. after the submodule groups lands) "submodule deinit ." will start acting weird again? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:43 ` Stefan Beller @ 2016-05-03 22:52 ` Junio C Hamano 2016-05-03 23:02 ` Stefan Beller 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-05-03 22:52 UTC (permalink / raw) To: Stefan Beller Cc: git@vger.kernel.org, Jens Lehmann, Jonathan Nieder, Per Cederqvist Stefan Beller <sbeller@google.com> writes: > So eventually (i.e. after the submodule groups lands) > "submodule deinit ." will start acting weird again? It would be nice if it never acts in a weird way, but that is all future development, not related to this fix, no? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:52 ` Junio C Hamano @ 2016-05-03 23:02 ` Stefan Beller 0 siblings, 0 replies; 17+ messages in thread From: Stefan Beller @ 2016-05-03 23:02 UTC (permalink / raw) To: Junio C Hamano Cc: git@vger.kernel.org, Jens Lehmann, Jonathan Nieder, Per Cederqvist On Tue, May 3, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> So eventually (i.e. after the submodule groups lands) >> "submodule deinit ." will start acting weird again? > > It would be nice if it never acts in a weird way, but that is all > future development, not related to this fix, no? Yes, just this patch is fine. Though lacking documentation. I'll send an update. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-03 22:11 [PATCH] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller 2016-05-03 22:25 ` Junio C Hamano 2016-05-03 22:28 ` Junio C Hamano @ 2016-05-04 0:33 ` Stefan Beller 2016-05-04 0:47 ` Jonathan Nieder 2016-05-04 1:19 ` [PATCHv3] " Stefan Beller 2 siblings, 2 replies; 17+ messages in thread From: Stefan Beller @ 2016-05-04 0:33 UTC (permalink / raw) To: gitster; +Cc: Jens.Lehmann, git, Stefan Beller The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails: > "submodule deinit ." may have "worked" in the sense that you would > have at least one path in your tree and avoided this "nothing > matches" most of the time. It would have still failed with the > exactly same error if run in an empty repository, i.e. > > $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E" > $ git init > $ rungit v2.6.6 submodule deinit . > error: pathspec '.' did not match any file(s) known to git. > Did you forget to 'git add'? > $ >file && git add file > $ rungit v2.6.6 submodule deinit . > $ echo $? > 0 So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository. The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in case of "$@" being empty. [1] http://news.gmane.org/gmane.comp.version-control.git/289535 Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- Junio writes: > I do not particularly view it as a new feature. The way the old > message suggested did not work in a pathological corner case, but we > wanted to keep the "force user to be explicit when doing mass > destruction", and a fix we happened to chose requires addition of a > new option--that would still look like a fix to me. It is a fix for a corner case, but it is renaming the switch, so I would expect that we'd wait for a new version at least. > It is not like we are forbidding the use of "submodule deinit ." > that used to work in a tree with at least one tracked path. With > the change, a script that has such a command will continue to work, > no? But we are also not garantueeing it either? Before this patch "submodule deinit ." was the blessed way to deinit all submodules. Sure, `deinit "*"` may have worked as well, but that was never asserted via tests internally nor via documentation to the user. So if someone was yelling at us because of a bug, we'd fix it if it was related to "deinit .", but not if it was "deinit '*'". This change both gives guidance in the documentation on how to deinit all submodules as well as removing the tests for the '.' case (by replacing them with the 'all' case). So I'd prefer if we'd spin this as a feature rather than a bug fix. This was developed on 2a4c8c36a7f6ad, 2016-03-24 Merge branch 'sb/submodule-module-list-pathspec-fix' Thanks, Stefan Documentation/git-submodule.txt | 12 +++++++++++- git-submodule.sh | 12 ++++++++++-- t/t7400-submodule-basic.sh | 14 +++++++++----- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 1572f05..4dbf8d0 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -13,7 +13,7 @@ SYNOPSIS [--reference <repository>] [--depth <depth>] [--] <repository> [<path>] 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] 'git submodule' [--quiet] init [--] [<path>...] -'git submodule' [--quiet] deinit [-f|--force] [--] <path>... +'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>... 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--] [<path>...] @@ -144,6 +144,11 @@ deinit:: you really want to remove a submodule from the repository and commit that use linkgit:git-rm[1] instead. + + To unregister all submodules use `--all` with no path spec. In + version 2.8 and prior, you were advised to give '.' to unregister + all submodules. This may continue to work in the future, but as the + path spec '.' may include regular files, this could stop working. ++ If `--force` is specified, the submodule's work tree will be removed even if it contains local modifications. @@ -247,6 +252,11 @@ OPTIONS --quiet:: Only print error messages. +-a:: +--all:: + This option is only valid for the deinit command. Unregister all + submodules in the work tree. + -b:: --branch:: Branch of repository to add as submodule. diff --git a/git-submodule.sh b/git-submodule.sh index 43c68de..301cd0d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -521,6 +521,7 @@ cmd_init() cmd_deinit() { # parse $args after "submodule ... deinit". + deinit_all= while test $# -ne 0 do case "$1" in @@ -530,6 +531,9 @@ cmd_deinit() -q|--quiet) GIT_QUIET=1 ;; + -a|--all) + deinit_all=t + ;; --) shift break @@ -544,9 +548,13 @@ cmd_deinit() shift done - if test $# = 0 + if test -n "$deinit_all" && test "$#" -ne 0 + then + die "$(eval_gettext "'--all' is incompatible with pathspec arguments")" + fi + if test $# = 0 && test -z "$deinit_all" then - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" fi git submodule--helper list --prefix "$wt_prefix" "$@" | diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index e1abd19..6e28ea5 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -11,6 +11,10 @@ subcommands of git submodule. . ./test-lib.sh +test_expect_success 'submodule deinit works on empty repository' ' + git submodule deinit --all +' + test_expect_success 'setup - initial commit' ' >t && git add t && @@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' ' >file && git add file && git commit -m "repo should not be empty" - git submodule deinit . + git submodule deinit --all ) ' @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' ' rmdir init ' -test_expect_success 'submodule deinit . deinits all initialized submodules' ' +test_expect_success 'submodule deinit --all deinits all initialized submodules' ' git submodule update --init && git config submodule.example.foo bar && git config submodule.example2.frotz nitfol && test_must_fail git submodule deinit && - git submodule deinit . >actual && + git submodule deinit --all >actual && test -z "$(git config --get-regexp "submodule\.example\.")" && test -z "$(git config --get-regexp "submodule\.example2\.")" && test_i18ngrep "Cleared directory .init" actual && @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su git submodule deinit init >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep "Cleared directory .init" actual && - git submodule deinit . >actual && + git submodule deinit --all >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual && test_i18ngrep "Cleared directory .init" actual && - git submodule deinit . >actual && + git submodule deinit --all >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual && test_i18ngrep "Cleared directory .init" actual && -- 2.8.0.rc4.10.geb92688.dirty ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 0:33 ` [PATCHv2] " Stefan Beller @ 2016-05-04 0:47 ` Jonathan Nieder 2016-05-04 1:19 ` [PATCHv3] " Stefan Beller 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2016-05-04 0:47 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, Jens.Lehmann, git Stefan Beller wrote: [...] >> $ rungit v2.6.6 submodule deinit . >> error: pathspec '.' did not match any file(s) known to git. >> Did you forget to 'git add'? >> $ >file && git add file >> $ rungit v2.6.6 submodule deinit . >> $ echo $? >> 0 > > So instead of a path spec add a parameter '--all' to deinit all submodules > and add a test to check for the corner case of an empty repository. Makes sense. [...] > It is a fix for a corner case, but it is renaming the switch, so I would > expect that we'd wait for a new version at least. The bug was that the documentation and error message gave advice that didn't work. [...] > +++ b/Documentation/git-submodule.txt > @@ -13,7 +13,7 @@ SYNOPSIS > [--reference <repository>] [--depth <depth>] [--] <repository> [<path>] > 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] > 'git submodule' [--quiet] init [--] [<path>...] > -'git submodule' [--quiet] deinit [-f|--force] [--] <path>... > +'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>... > 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] > [-f|--force] [--rebase|--merge] [--reference <repository>] > [--depth <depth>] [--recursive] [--] [<path>...] > @@ -144,6 +144,11 @@ deinit:: > you really want to remove a submodule from the repository and commit > that use linkgit:git-rm[1] instead. > + > + To unregister all submodules use `--all` with no path spec. In > + version 2.8 and prior, you were advised to give '.' to unregister > + all submodules. This may continue to work in the future, but as the > + path spec '.' may include regular files, this could stop working. > ++ > If `--force` is specified, the submodule's work tree will be removed even if > it contains local modifications. Inconsistent indentation. Does asciidoc format this correctly? [...] > +++ b/git-submodule.sh > @@ -521,6 +521,7 @@ cmd_init() [...] > shift > done > > - if test $# = 0 > + if test -n "$deinit_all" && test "$#" -ne 0 > + then > + die "$(eval_gettext "'--all' is incompatible with pathspec arguments")" > + fi This message is giving implementation details. Most other git commands just dump usage when passed too many arguments --- perhaps we can do something more targeted like the following: die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")" Speaking of which: please also update the USAGE string at the top of git-submodule.sh. [...] > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh Makes sense. Thanks for a pleasant fix. Jonathan ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 0:33 ` [PATCHv2] " Stefan Beller 2016-05-04 0:47 ` Jonathan Nieder @ 2016-05-04 1:19 ` Stefan Beller 2016-05-04 20:44 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Stefan Beller @ 2016-05-04 1:19 UTC (permalink / raw) To: jrnieder, gitster; +Cc: git, Jens.Lehmann, Stefan Beller The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails: > "submodule deinit ." may have "worked" in the sense that you would > have at least one path in your tree and avoided this "nothing > matches" most of the time. It would have still failed with the > exactly same error if run in an empty repository, i.e. > > $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E" > $ git init > $ rungit v2.6.6 submodule deinit . > error: pathspec '.' did not match any file(s) known to git. > Did you forget to 'git add'? > $ >file && git add file > $ rungit v2.6.6 submodule deinit . > $ echo $? > 0 So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository. The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in case of "$@" being empty. [1] http://news.gmane.org/gmane.comp.version-control.git/289535 Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- Changes since v2: * dedented section in documentation for --all, as it broke asciidoc. * Using a consistent usage string in git-modules.sh (both at top as well as in the die message) as well as in the documentation. Jonathan writes: >> It is a fix for a corner case, but it is renaming the switch, so I would >> expect that we'd wait for a new version at least. > The bug was that the documentation and error message gave advice that > didn't work. By having the old way untested, we do not give guarantees any more about the '.' case, which may be used in scripts and break, no? Thanks, Stefan Documentation/git-submodule.txt | 12 +++++++++++- git-submodule.sh | 14 +++++++++++--- t/t7400-submodule-basic.sh | 14 +++++++++----- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 1572f05..24d7197 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -13,7 +13,7 @@ SYNOPSIS [--reference <repository>] [--depth <depth>] [--] <repository> [<path>] 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] 'git submodule' [--quiet] init [--] [<path>...] -'git submodule' [--quiet] deinit [-f|--force] [--] <path>... +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...) 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase|--merge] [--reference <repository>] [--depth <depth>] [--recursive] [--] [<path>...] @@ -144,6 +144,11 @@ deinit:: you really want to remove a submodule from the repository and commit that use linkgit:git-rm[1] instead. + +To unregister all submodules use `--all` with no path spec. In +version 2.8 and prior, you were advised to give '.' to unregister +all submodules. This may continue to work in the future, but as the +path spec '.' may include regular files, this could stop working. ++ If `--force` is specified, the submodule's work tree will be removed even if it contains local modifications. @@ -247,6 +252,11 @@ OPTIONS --quiet:: Only print error messages. +-a:: +--all:: + This option is only valid for the deinit command. Unregister all + submodules in the work tree. + -b:: --branch:: Branch of repository to add as submodule. diff --git a/git-submodule.sh b/git-submodule.sh index 43c68de..6dabb56 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /') USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>] or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...] or: $dashless [--quiet] init [--] [<path>...] - or: $dashless [--quiet] deinit [-f|--force] [--] <path>... + or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...) or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...] or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...] or: $dashless [--quiet] foreach [--recursive] <command> @@ -521,6 +521,7 @@ cmd_init() cmd_deinit() { # parse $args after "submodule ... deinit". + deinit_all= while test $# -ne 0 do case "$1" in @@ -530,6 +531,9 @@ cmd_deinit() -q|--quiet) GIT_QUIET=1 ;; + -a|--all) + deinit_all=t + ;; --) shift break @@ -544,9 +548,13 @@ cmd_deinit() shift done - if test $# = 0 + if test -n "$deinit_all" && test "$#" -ne 0 + then + die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")" + fi + if test $# = 0 && test -z "$deinit_all" then - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" fi git submodule--helper list --prefix "$wt_prefix" "$@" | diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index e1abd19..6e28ea5 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -11,6 +11,10 @@ subcommands of git submodule. . ./test-lib.sh +test_expect_success 'submodule deinit works on empty repository' ' + git submodule deinit --all +' + test_expect_success 'setup - initial commit' ' >t && git add t && @@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' ' >file && git add file && git commit -m "repo should not be empty" - git submodule deinit . + git submodule deinit --all ) ' @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' ' rmdir init ' -test_expect_success 'submodule deinit . deinits all initialized submodules' ' +test_expect_success 'submodule deinit --all deinits all initialized submodules' ' git submodule update --init && git config submodule.example.foo bar && git config submodule.example2.frotz nitfol && test_must_fail git submodule deinit && - git submodule deinit . >actual && + git submodule deinit --all >actual && test -z "$(git config --get-regexp "submodule\.example\.")" && test -z "$(git config --get-regexp "submodule\.example2\.")" && test_i18ngrep "Cleared directory .init" actual && @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su git submodule deinit init >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep "Cleared directory .init" actual && - git submodule deinit . >actual && + git submodule deinit --all >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual && test_i18ngrep "Cleared directory .init" actual && - git submodule deinit . >actual && + git submodule deinit --all >actual && test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual && test_i18ngrep "Cleared directory .init" actual && -- 2.8.0.rc4.9.g9db9a47 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 1:19 ` [PATCHv3] " Stefan Beller @ 2016-05-04 20:44 ` Junio C Hamano 2016-05-04 21:34 ` Stefan Beller 2016-05-04 21:49 ` Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2016-05-04 20:44 UTC (permalink / raw) To: Stefan Beller; +Cc: jrnieder, git, Jens.Lehmann Stefan Beller <sbeller@google.com> writes: > The discussion in [1] realized that '.' is a faulty suggestion as > there is a corner case where it fails: A discussion does not "realize" (you may say "the discussion made me realize" but that gets personal and subjective description that is irrelevant in the project history), and this phrase has been bothering me since the original round. Perhaps s/realized/pointed out/ or something? > >> "submodule deinit ." may have "worked" in the sense that you would >> have at least one path in your tree and avoided this "nothing >> matches" most of the time. It would have still failed with the >> exactly same error if run in an empty repository, i.e. >> >> $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E" >> $ git init >> $ rungit v2.6.6 submodule deinit . >> error: pathspec '.' did not match any file(s) known to git. >> Did you forget to 'git add'? >> $ >file && git add file >> $ rungit v2.6.6 submodule deinit . >> $ echo $? >> 0 > > So instead of a path spec add a parameter '--all' to deinit all submodules s/path spec/pathspec/; s/a parameter '--all'/the '--all' option/; > and add a test to check for the corner case of an empty repository. > > The code only needs to learn about the '--all' parameter and doesn't Likewise. > require further changes as `git submodule--helper list "$@"` will list > all submodules in case of "$@" being empty. I'd propose doing s/in case of.../when "$@" is empty./ > diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt > index 1572f05..24d7197 100644 > --- a/Documentation/git-submodule.txt > +++ b/Documentation/git-submodule.txt > @@ -13,7 +13,7 @@ SYNOPSIS > [--reference <repository>] [--depth <depth>] [--] <repository> [<path>] > 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] > 'git submodule' [--quiet] init [--] [<path>...] > -'git submodule' [--quiet] deinit [-f|--force] [--] <path>... > +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...) > 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] > [-f|--force] [--rebase|--merge] [--reference <repository>] > [--depth <depth>] [--recursive] [--] [<path>...] > @@ -144,6 +144,11 @@ deinit:: > you really want to remove a submodule from the repository and commit > that use linkgit:git-rm[1] instead. > + > +To unregister all submodules use `--all` with no path spec. In s/path spec/pathspec/; But I'd rather see something more like this instead of the first sentence: When the command is run without pathspec, it errors out, instead of deinit-ing everything, to prevent mistakes. > +version 2.8 and prior, you were advised to give '.' to unregister > +all submodules. This may continue to work in the future, but as the > +path spec '.' may include regular files, this could stop working. ... the command gave a suggestion to use '.' to unregister all submodules when it was invoked without any argument, but this suggestion did not work and gave a wrong message if you followed in pathological cases and is no longer recommended. Do not predict the future in the documentation when we ourselves have not committed to any concrete plan. > If `--force` is specified, the submodule's work tree will be removed even if > it contains local modifications. I think this sentence talks about "working tree" (as opposed to "worktree"), so s/work tree/working tree/. > @@ -247,6 +252,11 @@ OPTIONS > --quiet:: > Only print error messages. > > +-a:: > +--all:: > + This option is only valid for the deinit command. Unregister all > + submodules in the work tree. Likewise. > -b:: > --branch:: > Branch of repository to add as submodule. > diff --git a/git-submodule.sh b/git-submodule.sh > index 43c68de..6dabb56 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /') > USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>] > or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...] > or: $dashless [--quiet] init [--] [<path>...] > - or: $dashless [--quiet] deinit [-f|--force] [--] <path>... > + or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...) > or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...] > or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...] > or: $dashless [--quiet] foreach [--recursive] <command> > @@ -521,6 +521,7 @@ cmd_init() > cmd_deinit() > { > # parse $args after "submodule ... deinit". > + deinit_all= > while test $# -ne 0 > do > case "$1" in > @@ -530,6 +531,9 @@ cmd_deinit() > -q|--quiet) > GIT_QUIET=1 > ;; > + -a|--all) > + deinit_all=t > + ;; > --) > shift > break > @@ -544,9 +548,13 @@ cmd_deinit() > shift > done > > - if test $# = 0 > + if test -n "$deinit_all" && test "$#" -ne 0 > + then > + die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")" I doubt that "usage:" wants to go thru l10n. I suspect that it is more friendly to the user to say that in prose, i.e.e.g. "--all and pathspec cannot be given at the same time", than forcing them to grok the (alternative|possibilities). > + fi > + if test $# = 0 && test -z "$deinit_all" > then > - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" > + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" > fi This is good. > git submodule--helper list --prefix "$wt_prefix" "$@" | > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index e1abd19..6e28ea5 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -11,6 +11,10 @@ subcommands of git submodule. > > . ./test-lib.sh > > +test_expect_success 'submodule deinit works on empty repository' ' > + git submodule deinit --all > +' > + > test_expect_success 'setup - initial commit' ' > >t && > git add t && > @@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' ' > >file && > git add file && > git commit -m "repo should not be empty" > - git submodule deinit . > + git submodule deinit --all > ) > ' > > @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' ' > rmdir init > ' > > -test_expect_success 'submodule deinit . deinits all initialized submodules' ' > +test_expect_success 'submodule deinit --all deinits all initialized submodules' ' > git submodule update --init && > git config submodule.example.foo bar && > git config submodule.example2.frotz nitfol && > test_must_fail git submodule deinit && > - git submodule deinit . >actual && > + git submodule deinit --all >actual && > test -z "$(git config --get-regexp "submodule\.example\.")" && > test -z "$(git config --get-regexp "submodule\.example2\.")" && > test_i18ngrep "Cleared directory .init" actual && > @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su > git submodule deinit init >actual && > test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && > test_i18ngrep "Cleared directory .init" actual && > - git submodule deinit . >actual && > + git submodule deinit --all >actual && > test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && > test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual && > test_i18ngrep "Cleared directory .init" actual && > - git submodule deinit . >actual && > + git submodule deinit --all >actual && > test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual && > test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual && > test_i18ngrep "Cleared directory .init" actual && I would have expected that we'd be testing both '.' and '--all', by keeping the '.' tests as they were and adding tests for '--all'. It is not like we are discouraging use of '.' when the repository is known to have submodule(s) and '.' is expected to match. Other than that, looks good to me. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 20:44 ` Junio C Hamano @ 2016-05-04 21:34 ` Stefan Beller 2016-05-04 21:43 ` Junio C Hamano 2016-05-04 21:49 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Stefan Beller @ 2016-05-04 21:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann On Wed, May 4, 2016 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote: > I think this sentence talks about "working tree" (as opposed to > "worktree"), so s/work tree/working tree/. I'll fix this up in a resend, though it may be a fix on its own. So the two "official" terms are working tree (files on your disk) and worktree (the command) and we don't want to have anything in between? (e.g. work tree for working tree?) Or as `grep -r "work tree"` puts it, we may want to have an extra cleanup patch and not do it here for this single occurrence. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 21:34 ` Stefan Beller @ 2016-05-04 21:43 ` Junio C Hamano 2016-05-04 21:47 ` Stefan Beller 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-05-04 21:43 UTC (permalink / raw) To: Stefan Beller; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann Stefan Beller <sbeller@google.com> writes: > (e.g. work tree for working tree?) This was probably primarily my fault, not just because I've written more than my share of documentation (compared to the code that touched), but I was deliberately writing "work tree" when both "work tree" and "working tree" terms meant the same thing. Compared to the length of the timeperiod, the newcomer who is now known as "worktree" has only lived a very short period of time, so it is not surprising to see remaining "work tree" in our documentation set. I think some attempts like 06cdac5a (git-reset.txt: use "working tree" consistently, 2010-09-15) were made to clean things up even before "worktree" started to mean an entirely different thing, and we shouldn't make things worse by adding mentions of "work tree" when we mean "working tree". ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 21:43 ` Junio C Hamano @ 2016-05-04 21:47 ` Stefan Beller 2016-05-04 21:51 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Stefan Beller @ 2016-05-04 21:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann On Wed, May 4, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> (e.g. work tree for working tree?) > > This was probably primarily my fault, not just because I've written > more than my share of documentation (compared to the code that > touched), but I was deliberately writing "work tree" when both "work > tree" and "working tree" terms meant the same thing. Compared to > the length of the timeperiod, the newcomer who is now known as > "worktree" has only lived a very short period of time, so it is not > surprising to see remaining "work tree" in our documentation set. Sure. > > I think some attempts like 06cdac5a (git-reset.txt: use "working > tree" consistently, 2010-09-15) were made to clean things up even > before "worktree" started to mean an entirely different thing, and > we shouldn't make things worse by adding mentions of "work tree" > when we mean "working tree". Ok. I'll see if I can send a similar patch like that. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 21:47 ` Stefan Beller @ 2016-05-04 21:51 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2016-05-04 21:51 UTC (permalink / raw) To: Stefan Beller; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann Stefan Beller <sbeller@google.com> writes: >> I think some attempts like 06cdac5a (git-reset.txt: use "working >> tree" consistently, 2010-09-15) were made to clean things up even >> before "worktree" started to mean an entirely different thing, and >> we shouldn't make things worse by adding mentions of "work tree" >> when we mean "working tree". > > Ok. I'll see if I can send a similar patch like that. Please don't churn the documentation especially when there are more important changes in flight that deserve larger share of reviewer bandwidth. My review comment on the documentation part of the patch was primarily not to make things worse when you do not have to. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 20:44 ` Junio C Hamano 2016-05-04 21:34 ` Stefan Beller @ 2016-05-04 21:49 ` Junio C Hamano 2016-05-04 21:57 ` Stefan Beller 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2016-05-04 21:49 UTC (permalink / raw) To: Stefan Beller; +Cc: jrnieder, git, Jens.Lehmann Junio C Hamano <gitster@pobox.com> writes: >> + if test -n "$deinit_all" && test "$#" -ne 0 >> + then >> + die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")" > > I doubt that "usage:" wants to go thru l10n. > > I suspect that it is more friendly to the user to say that in prose, > i.e.e.g. "--all and pathspec cannot be given at the same time", than > forcing them to grok the (alternative|possibilities). > >> + fi >> + if test $# = 0 && test -z "$deinit_all" >> then >> - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" >> + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" >> fi > > This is good. By the way, while it is a very good idea to die upon $ git submodule deinit --all no-only-this-one it may not be too bad if we demoted the output to "info" with clean no-op exit when the user said $ git submodule deinit IOW, the latter part _might_ be better if it were if test $# = 0 && test -z "$deinit_all" then echo >&2 "info: not deinitializing anything." echo >&2 "info: Use --all to deinitialize all submodules." exit 0 fi given that this is really about preventing mistakes from doing mass destruction. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 21:49 ` Junio C Hamano @ 2016-05-04 21:57 ` Stefan Beller 2016-05-04 22:06 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Stefan Beller @ 2016-05-04 21:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann On Wed, May 4, 2016 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> + if test -n "$deinit_all" && test "$#" -ne 0 >>> + then >>> + die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")" >> >> I doubt that "usage:" wants to go thru l10n. >> >> I suspect that it is more friendly to the user to say that in prose, >> i.e.e.g. "--all and pathspec cannot be given at the same time", than >> forcing them to grok the (alternative|possibilities). >> >>> + fi >>> + if test $# = 0 && test -z "$deinit_all" >>> then >>> - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" >>> + die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")" >>> fi >> >> This is good. > > By the way, while it is a very good idea to die upon > > $ git submodule deinit --all no-only-this-one > > it may not be too bad if we demoted the output to "info" with clean > no-op exit when the user said > > $ git submodule deinit > > IOW, the latter part _might_ be better if it were > > if test $# = 0 && test -z "$deinit_all" > then > echo >&2 "info: not deinitializing anything." > echo >&2 "info: Use --all to deinitialize all submodules." > exit 0 > fi > > given that this is really about preventing mistakes from doing mass > destruction. Demote to a new class in a class of its own? * grep -r "info:" gives no match for user facing code, so it's a prefix you made up now. * I assume we want to translate it? * This would not respect the --quiet option? * returning 0 as in "everyting is fine", while nothing is fine. There are two cases: - Either the user knows what they are doing (See Per complaining about this) A very deliberate "I want it all gone or error out if you cannot remove it all" - The user has no idea what they are doing. Exiting non zero doesn't do any harm. Maybe: > - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" > + usage() instead? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules 2016-05-04 21:57 ` Stefan Beller @ 2016-05-04 22:06 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2016-05-04 22:06 UTC (permalink / raw) To: Stefan Beller; +Cc: Jonathan Nieder, git@vger.kernel.org, Jens Lehmann Stefan Beller <sbeller@google.com> writes: >> IOW, the latter part _might_ be better if it were >> >> if test $# = 0 && test -z "$deinit_all" >> then >> echo >&2 "info: not deinitializing anything." >> echo >&2 "info: Use --all to deinitialize all submodules." >> exit 0 >> fi >> >> given that this is really about preventing mistakes from doing mass >> destruction. > ... > Maybe: > >> - die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" >> + usage() > > instead? I was primarily concerned about die giving a non-zero exit status when "git submodule deinit" did not find anything worthwhile to do (because we specified nothing on the command line to deinit). IOW, it was an attempt to do "You asked me a no-op, so I am doing a no-op, but if I stayed silent, you wouldn't even notice it, and you might have meant to deinit all, so I am telling you I didn't do anything, and advising how to say 'deinit all' to me." But what we see in the patch under discussion is perfectly fine; it behaves just like "$ rm<RET>" and "$ git add<RET>" that give an error message and exit with a non-zero status. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-05-04 22:06 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-03 22:11 [PATCH] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller 2016-05-03 22:25 ` Junio C Hamano 2016-05-03 22:28 ` Junio C Hamano 2016-05-03 22:43 ` Stefan Beller 2016-05-03 22:52 ` Junio C Hamano 2016-05-03 23:02 ` Stefan Beller 2016-05-04 0:33 ` [PATCHv2] " Stefan Beller 2016-05-04 0:47 ` Jonathan Nieder 2016-05-04 1:19 ` [PATCHv3] " Stefan Beller 2016-05-04 20:44 ` Junio C Hamano 2016-05-04 21:34 ` Stefan Beller 2016-05-04 21:43 ` Junio C Hamano 2016-05-04 21:47 ` Stefan Beller 2016-05-04 21:51 ` Junio C Hamano 2016-05-04 21:49 ` Junio C Hamano 2016-05-04 21:57 ` Stefan Beller 2016-05-04 22:06 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).