{"thread":{"id":"42212","subject":"[PATCH] submodule deinit: require '--all' instead of '.' for all submodules","startedAt":"2016-05-03T22:11:39Z","lastAt":"2016-05-04T22:06:01Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285387","messageId":"1462313499-6760-1-git-send-email-sbeller@google.com","threadId":"42212","inReplyTo":null,"subject":"[PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T22:11:39Z","receivedAt":"2016-05-03T22:11:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The discussion in [1] realized that '.' is a faulty suggestion as\nthere is a corner case where it fails:\n\n> \"submodule deinit .\" may have \"worked\" in the sense that you would\n> have at least one path in your tree and avoided this \"nothing\n> matches\" most of the time.  It would have still failed with the\n> exactly same error if run in an empty repository, i.e.\n>\n>        $ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n>        $ git init\n>        $ rungit v2.6.6 submodule deinit .\n>        error: pathspec '.' did not match any file(s) known to git.\n>        Did you forget to 'git add'?\n>        $ >file && git add file\n>        $ rungit v2.6.6 submodule deinit .\n>        $ echo $?\n>        0\n\nSo instead of a path spec add a parameter '--all' to deinit all submodules\nand add a test to check for the corner case of an empty repository.\n\nThere is no need to update the documentation as it did not describe the\nspecial case '.' to remove all submodules.\n\nThe code only needs to learn about the '--all' parameter and doesn't\nrequire further changes as `git submodule--helper list \"$@\"` will list\nall submodules in that case.\n\n[1] http://news.gmane.org/gmane.comp.version-control.git/289535\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis is the result of the discussion, I would think.\n\nI developed it on top of \n    \"submodule deinit test: fix broken && chain in subshell\"\n    on top of 2a4c8c36a7f6, 2016-03-24, Merge branch\n    'sb/submodule-module-list-pathspec-fix'\nbut I think this would rather go in as a new feature, not on top\nof a bugfix topic, so I think this could go on origin/master ?\n\nThanks,\nStefan\n\n git-submodule.sh           | 12 ++++++++++--\n t/t7400-submodule-basic.sh |  4 ++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..301cd0d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -521,6 +521,7 @@ cmd_init()\n cmd_deinit()\n {\n \t# parse $args after \"submodule ... deinit\".\n+\tdeinit_all=\n \twhile test $# -ne 0\n \tdo\n \t\tcase \"$1\" in\n@@ -530,6 +531,9 @@ cmd_deinit()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n+\t\t-a|--all)\n+\t\t\tdeinit_all=t\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -544,9 +548,13 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tif test $# = 0\n+\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n+\tthen\n+\t\tdie \"$(eval_gettext \"'--all' is incompatible with pathspec arguments\")\"\n+\tfi\n+\tif test $# = 0 && test -z \"$deinit_all\"\n \tthen\n-\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n+\t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n \tfi\n \n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cf06b2f..a5b0dec 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -11,6 +11,10 @@ subcommands of git submodule.\n \n . ./test-lib.sh\n \n+test_expect_success 'submodule deinit works on empty repository' '\n+\tgit submodule deinit --all\n+'\n+\n test_expect_success 'setup - initial commit' '\n \t>t &&\n \tgit add t &&\n-- \n2.8.0.rc4.10.geb92688.dirty\n"},{"id":"285390","messageId":"xmqq37pylqxo.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"1462313499-6760-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T22:25:39Z","receivedAt":"2016-05-03T22:25:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> ...\n> So instead of a path spec add a parameter '--all' to deinit all submodules\n> and add a test to check for the corner case of an empty repository.\n>\n> There is no need to update the documentation as it did not describe the\n> special case '.' to remove all submodules.\n\nNow there is, isn't it?\n"},{"id":"285391","messageId":"xmqqy47qkc87.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"1462313499-6760-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T22:28:40Z","receivedAt":"2016-05-03T22:28:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I developed it on top of \n>     \"submodule deinit test: fix broken && chain in subshell\"\n>     on top of 2a4c8c36a7f6, 2016-03-24, Merge branch\n>     'sb/submodule-module-list-pathspec-fix'\n> but I think this would rather go in as a new feature, not on top\n> of a bugfix topic, so I think this could go on origin/master ?\n\nI do not particularly view it as a new feature.  The way the old\nmessage suggested did not work in a pathological corner case, but we\nwanted to keep the \"force user to be explicit when doing mass\ndestruction\", and a fix we happened to chose requires addition of a\nnew option--that would still look like a fix to me.\n\nIt is not like we are forbidding the use of \"submodule deinit .\"\nthat used to work in a tree with at least one tracked path.  With\nthe change, a script that has such a command will continue to work,\nno?\n"},{"id":"285393","messageId":"CAGZ79ka8RQmNzzKJ9r_GsLkSCK5Thewsz9scZvwpJb0Pf_+NRg@mail.gmail.com","threadId":"42212","inReplyTo":"xmqqy47qkc87.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T22:43:32Z","receivedAt":"2016-05-03T22:43:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I developed it on top of\n>>     \"submodule deinit test: fix broken && chain in subshell\"\n>>     on top of 2a4c8c36a7f6, 2016-03-24, Merge branch\n>>     'sb/submodule-module-list-pathspec-fix'\n>> but I think this would rather go in as a new feature, not on top\n>> of a bugfix topic, so I think this could go on origin/master ?\n>\n> I do not particularly view it as a new feature.  The way the old\n> message suggested did not work in a pathological corner case, but we\n> wanted to keep the \"force user to be explicit when doing mass\n> destruction\", and a fix we happened to chose requires addition of a\n> new option--that would still look like a fix to me.\n>\n> It is not like we are forbidding the use of \"submodule deinit .\"\n> that used to work in a tree with at least one tracked path.  With\n> the change, a script that has such a command will continue to work,\n> no?\n\nMaybe.\n\nWith just this patch, yes.\n\nI'd like to revert submodule-module-list-pathspec-fix partially\nwhen redoing the groups support. That would break the '.' script\ncase. So eventually scripts want to use\n\n    git submodule deinit -f --all\n\ninstead of\n\n    git submodule deinit -f .\n\nWhen implementing the groups support, I'd change module_list\nin a way that you can give names, paths, or labels to it. In case of\na user gives 'COPYIN*' we'd want to know if that is a path (or a name,\nlabel) or bogus, so I think we'd tighten the checks there just for the\nfunctionality not just performance as originally anticipated for the\norder of S_ISGITLINK and match_pathspec.\n\nSo eventually (i.e. after the submodule groups lands)\n\"submodule deinit .\" will start acting weird again?\n"},{"id":"285397","messageId":"xmqqoa8mkb4l.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"CAGZ79ka8RQmNzzKJ9r_GsLkSCK5Thewsz9scZvwpJb0Pf_+NRg@mail.gmail.com","subject":"Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T22:52:26Z","receivedAt":"2016-05-03T22:52:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So eventually (i.e. after the submodule groups lands)\n> \"submodule deinit .\" will start acting weird again?\n\nIt would be nice if it never acts in a weird way, but that is all\nfuture development, not related to this fix, no?\n"},{"id":"285399","messageId":"CAGZ79kaHs6TgxQpPUOKA7NQ8k0r80RrGg7aTtxN90FXwLBsVeA@mail.gmail.com","threadId":"42212","inReplyTo":"xmqqoa8mkb4l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T23:02:27Z","receivedAt":"2016-05-03T23:02:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> So eventually (i.e. after the submodule groups lands)\n>> \"submodule deinit .\" will start acting weird again?\n>\n> It would be nice if it never acts in a weird way, but that is all\n> future development, not related to this fix, no?\n\nYes, just this patch is fine. Though lacking documentation.\nI'll send an update.\n"},{"id":"285404","messageId":"1462321992-15153-1-git-send-email-sbeller@google.com","threadId":"42212","inReplyTo":"1462313499-6760-1-git-send-email-sbeller@google.com","subject":"[PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T00:33:12Z","receivedAt":"2016-05-04T00:33:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The discussion in [1] realized that '.' is a faulty suggestion as\nthere is a corner case where it fails:\n\n> \"submodule deinit .\" may have \"worked\" in the sense that you would\n> have at least one path in your tree and avoided this \"nothing\n> matches\" most of the time.  It would have still failed with the\n> exactly same error if run in an empty repository, i.e.\n>\n>        $ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n>        $ git init\n>        $ rungit v2.6.6 submodule deinit .\n>        error: pathspec '.' did not match any file(s) known to git.\n>        Did you forget to 'git add'?\n>        $ >file && git add file\n>        $ rungit v2.6.6 submodule deinit .\n>        $ echo $?\n>        0\n\nSo instead of a path spec add a parameter '--all' to deinit all submodules\nand add a test to check for the corner case of an empty repository.\n\nThe code only needs to learn about the '--all' parameter and doesn't\nrequire further changes as `git submodule--helper list \"$@\"` will list\nall submodules in case of \"$@\" being empty.\n\n[1] http://news.gmane.org/gmane.comp.version-control.git/289535\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nJunio writes:\n> I do not particularly view it as a new feature.  The way the old\n> message suggested did not work in a pathological corner case, but we\n> wanted to keep the \"force user to be explicit when doing mass\n> destruction\", and a fix we happened to chose requires addition of a\n> new option--that would still look like a fix to me.\n\nIt is a fix for a corner case, but it is renaming the switch, so I would\nexpect that we'd wait for a new version at least.\n\n> It is not like we are forbidding the use of \"submodule deinit .\"\n> that used to work in a tree with at least one tracked path.  With\n> the change, a script that has such a command will continue to work,\n> no?\n\nBut we are also not garantueeing it either? Before this patch\n\"submodule deinit .\" was the blessed way to deinit all submodules.\nSure, `deinit \"*\"` may have worked as well, but that was never asserted\nvia tests internally nor via documentation to the user.\n\nSo if someone was yelling at us because of a bug, we'd fix it if it was\nrelated to \"deinit .\", but not if it was \"deinit '*'\".\n\nThis change both gives guidance in the documentation on how to deinit\nall submodules as well as removing the tests for the '.' case (by \nreplacing them with the 'all' case).\n\nSo I'd prefer if we'd spin this as a feature rather than a bug fix.\n\nThis was developed on 2a4c8c36a7f6ad, 2016-03-24\nMerge branch 'sb/submodule-module-list-pathspec-fix'\n\nThanks,\nStefan\n\n Documentation/git-submodule.txt | 12 +++++++++++-\n git-submodule.sh                | 12 ++++++++++--\n t/t7400-submodule-basic.sh      | 14 +++++++++-----\n 3 files changed, 30 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 1572f05..4dbf8d0 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]\n 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n 'git submodule' [--quiet] init [--] [<path>...]\n-'git submodule' [--quiet] deinit [-f|--force] [--] <path>...\n+'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n \t      [--depth <depth>] [--recursive] [--] [<path>...]\n@@ -144,6 +144,11 @@ deinit::\n \tyou really want to remove a submodule from the repository and commit\n \tthat use linkgit:git-rm[1] instead.\n +\n+\tTo unregister all submodules use `--all` with no path spec. In\n+\tversion 2.8 and prior, you were advised to give '.' to unregister\n+\tall submodules. This may continue to work in the future, but as the\n+\tpath spec '.' may include regular files, this could stop working.\n++\n If `--force` is specified, the submodule's work tree will be removed even if\n it contains local modifications.\n \n@@ -247,6 +252,11 @@ OPTIONS\n --quiet::\n \tOnly print error messages.\n \n+-a::\n+--all::\n+\tThis option is only valid for the deinit command. Unregister all\n+\tsubmodules in the work tree.\n+\n -b::\n --branch::\n \tBranch of repository to add as submodule.\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..301cd0d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -521,6 +521,7 @@ cmd_init()\n cmd_deinit()\n {\n \t# parse $args after \"submodule ... deinit\".\n+\tdeinit_all=\n \twhile test $# -ne 0\n \tdo\n \t\tcase \"$1\" in\n@@ -530,6 +531,9 @@ cmd_deinit()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n+\t\t-a|--all)\n+\t\t\tdeinit_all=t\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -544,9 +548,13 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tif test $# = 0\n+\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n+\tthen\n+\t\tdie \"$(eval_gettext \"'--all' is incompatible with pathspec arguments\")\"\n+\tfi\n+\tif test $# = 0 && test -z \"$deinit_all\"\n \tthen\n-\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n+\t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n \tfi\n \n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex e1abd19..6e28ea5 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -11,6 +11,10 @@ subcommands of git submodule.\n \n . ./test-lib.sh\n \n+test_expect_success 'submodule deinit works on empty repository' '\n+\tgit submodule deinit --all\n+'\n+\n test_expect_success 'setup - initial commit' '\n \t>t &&\n \tgit add t &&\n@@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n \t\t>file &&\n \t\tgit add file &&\n \t\tgit commit -m \"repo should not be empty\"\n-\t\tgit submodule deinit .\n+\t\tgit submodule deinit --all\n \t)\n '\n \n@@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '\n \trmdir init\n '\n \n-test_expect_success 'submodule deinit . deinits all initialized submodules' '\n+test_expect_success 'submodule deinit --all deinits all initialized submodules' '\n \tgit submodule update --init &&\n \tgit config submodule.example.foo bar &&\n \tgit config submodule.example2.frotz nitfol &&\n \ttest_must_fail git submodule deinit &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\" &&\n \ttest -z \"$(git config --get-regexp \"submodule\\.example2\\.\")\" &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n@@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \tgit submodule deinit init >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep ! \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-- \n2.8.0.rc4.10.geb92688.dirty\n"},{"id":"285405","messageId":"20160504004759.GB395@google.com","threadId":"42212","inReplyTo":"1462321992-15153-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-05-04T00:47:59Z","receivedAt":"2016-05-04T00:47:59Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n[...]\n>>        $ rungit v2.6.6 submodule deinit .\n>>        error: pathspec '.' did not match any file(s) known to git.\n>>        Did you forget to 'git add'?\n>>        $ >file && git add file\n>>        $ rungit v2.6.6 submodule deinit .\n>>        $ echo $?\n>>        0\n>\n> So instead of a path spec add a parameter '--all' to deinit all submodules\n> and add a test to check for the corner case of an empty repository.\n\nMakes sense.\n\n[...]\n> It is a fix for a corner case, but it is renaming the switch, so I would\n> expect that we'd wait for a new version at least.\n\nThe bug was that the documentation and error message gave advice that\ndidn't work.\n\n[...]\n> +++ b/Documentation/git-submodule.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n>  \t      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]\n>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n>  'git submodule' [--quiet] init [--] [<path>...]\n> -'git submodule' [--quiet] deinit [-f|--force] [--] <path>...\n> +'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>...\n>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n>  \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n>  \t      [--depth <depth>] [--recursive] [--] [<path>...]\n> @@ -144,6 +144,11 @@ deinit::\n>  \tyou really want to remove a submodule from the repository and commit\n>  \tthat use linkgit:git-rm[1] instead.\n>  +\n> +\tTo unregister all submodules use `--all` with no path spec. In\n> +\tversion 2.8 and prior, you were advised to give '.' to unregister\n> +\tall submodules. This may continue to work in the future, but as the\n> +\tpath spec '.' may include regular files, this could stop working.\n> ++\n>  If `--force` is specified, the submodule's work tree will be removed even if\n>  it contains local modifications.\n\nInconsistent indentation.  Does asciidoc format this correctly?\n\n[...]\n> +++ b/git-submodule.sh\n> @@ -521,6 +521,7 @@ cmd_init()\n[...]\n>  \t\tshift\n>  \tdone\n>  \n> -\tif test $# = 0\n> +\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n> +\tthen\n> +\t\tdie \"$(eval_gettext \"'--all' is incompatible with pathspec arguments\")\"\n> +\tfi\n\nThis message is giving implementation details.  Most other git\ncommands just dump usage when passed too many arguments --- perhaps we\ncan do something more targeted like the following:\n\n\tdie \"$(eval_gettext \"usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\")\"\n\nSpeaking of which: please also update the USAGE string at the top of\ngit-submodule.sh.\n\n\n[...]\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n\nMakes sense.\n\nThanks for a pleasant fix.\n\nJonathan\n"},{"id":"285407","messageId":"1462324785-26389-1-git-send-email-sbeller@google.com","threadId":"42212","inReplyTo":"1462321992-15153-1-git-send-email-sbeller@google.com","subject":"[PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T01:19:45Z","receivedAt":"2016-05-04T01:19:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The discussion in [1] realized that '.' is a faulty suggestion as\nthere is a corner case where it fails:\n\n> \"submodule deinit .\" may have \"worked\" in the sense that you would\n> have at least one path in your tree and avoided this \"nothing\n> matches\" most of the time.  It would have still failed with the\n> exactly same error if run in an empty repository, i.e.\n>\n>        $ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n>        $ git init\n>        $ rungit v2.6.6 submodule deinit .\n>        error: pathspec '.' did not match any file(s) known to git.\n>        Did you forget to 'git add'?\n>        $ >file && git add file\n>        $ rungit v2.6.6 submodule deinit .\n>        $ echo $?\n>        0\n\nSo instead of a path spec add a parameter '--all' to deinit all submodules\nand add a test to check for the corner case of an empty repository.\n\nThe code only needs to learn about the '--all' parameter and doesn't\nrequire further changes as `git submodule--helper list \"$@\"` will list\nall submodules in case of \"$@\" being empty.\n\n[1] http://news.gmane.org/gmane.comp.version-control.git/289535\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nChanges since v2:\n* dedented section in documentation for --all, as it broke asciidoc.\n* Using a consistent usage string in git-modules.sh (both at top as well as in\n  the die message) as well as in the documentation.\n\nJonathan writes:\n>> It is a fix for a corner case, but it is renaming the switch, so I would\n>> expect that we'd wait for a new version at least.\n> The bug was that the documentation and error message gave advice that\n> didn't work.\n\nBy having the old way untested, we do not give guarantees any more about the '.'\ncase, which may be used in scripts and break, no?\n\nThanks,\nStefan\n\n Documentation/git-submodule.txt | 12 +++++++++++-\n git-submodule.sh                | 14 +++++++++++---\n t/t7400-submodule-basic.sh      | 14 +++++++++-----\n 3 files changed, 31 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 1572f05..24d7197 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]\n 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n 'git submodule' [--quiet] init [--] [<path>...]\n-'git submodule' [--quiet] deinit [-f|--force] [--] <path>...\n+'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...)\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n \t      [--depth <depth>] [--recursive] [--] [<path>...]\n@@ -144,6 +144,11 @@ deinit::\n \tyou really want to remove a submodule from the repository and commit\n \tthat use linkgit:git-rm[1] instead.\n +\n+To unregister all submodules use `--all` with no path spec. In\n+version 2.8 and prior, you were advised to give '.' to unregister\n+all submodules. This may continue to work in the future, but as the\n+path spec '.' may include regular files, this could stop working.\n++\n If `--force` is specified, the submodule's work tree will be removed even if\n it contains local modifications.\n \n@@ -247,6 +252,11 @@ OPTIONS\n --quiet::\n \tOnly print error messages.\n \n+-a::\n+--all::\n+\tThis option is only valid for the deinit command. Unregister all\n+\tsubmodules in the work tree.\n+\n -b::\n --branch::\n \tBranch of repository to add as submodule.\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..6dabb56 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -8,7 +8,7 @@ dashless=$(basename \"$0\" | sed -e 's/-/ /')\n USAGE=\"[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]\n    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]\n    or: $dashless [--quiet] init [--] [<path>...]\n-   or: $dashless [--quiet] deinit [-f|--force] [--] <path>...\n+   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...)\n    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]\n    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]\n    or: $dashless [--quiet] foreach [--recursive] <command>\n@@ -521,6 +521,7 @@ cmd_init()\n cmd_deinit()\n {\n \t# parse $args after \"submodule ... deinit\".\n+\tdeinit_all=\n \twhile test $# -ne 0\n \tdo\n \t\tcase \"$1\" in\n@@ -530,6 +531,9 @@ cmd_deinit()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n+\t\t-a|--all)\n+\t\t\tdeinit_all=t\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -544,9 +548,13 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tif test $# = 0\n+\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n+\tthen\n+\t\tdie \"$(eval_gettext \"usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\")\"\n+\tfi\n+\tif test $# = 0 && test -z \"$deinit_all\"\n \tthen\n-\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n+\t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n \tfi\n \n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex e1abd19..6e28ea5 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -11,6 +11,10 @@ subcommands of git submodule.\n \n . ./test-lib.sh\n \n+test_expect_success 'submodule deinit works on empty repository' '\n+\tgit submodule deinit --all\n+'\n+\n test_expect_success 'setup - initial commit' '\n \t>t &&\n \tgit add t &&\n@@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n \t\t>file &&\n \t\tgit add file &&\n \t\tgit commit -m \"repo should not be empty\"\n-\t\tgit submodule deinit .\n+\t\tgit submodule deinit --all\n \t)\n '\n \n@@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '\n \trmdir init\n '\n \n-test_expect_success 'submodule deinit . deinits all initialized submodules' '\n+test_expect_success 'submodule deinit --all deinits all initialized submodules' '\n \tgit submodule update --init &&\n \tgit config submodule.example.foo bar &&\n \tgit config submodule.example2.frotz nitfol &&\n \ttest_must_fail git submodule deinit &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\" &&\n \ttest -z \"$(git config --get-regexp \"submodule\\.example2\\.\")\" &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n@@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \tgit submodule deinit init >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-\tgit submodule deinit . >actual &&\n+\tgit submodule deinit --all >actual &&\n \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n \ttest_i18ngrep ! \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n \ttest_i18ngrep \"Cleared directory .init\" actual &&\n-- \n2.8.0.rc4.9.g9db9a47\n"},{"id":"285511","messageId":"xmqqlh3pft91.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"1462324785-26389-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T20:44:26Z","receivedAt":"2016-05-04T20:44:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The discussion in [1] realized that '.' is a faulty suggestion as\n> there is a corner case where it fails:\n\nA discussion does not \"realize\" (you may say \"the discussion made me\nrealize\" but that gets personal and subjective description that is\nirrelevant in the project history), and this phrase has been\nbothering me since the original round.\n\nPerhaps s/realized/pointed out/ or something?\n\n>\n>> \"submodule deinit .\" may have \"worked\" in the sense that you would\n>> have at least one path in your tree and avoided this \"nothing\n>> matches\" most of the time.  It would have still failed with the\n>> exactly same error if run in an empty repository, i.e.\n>>\n>>        $ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n>>        $ git init\n>>        $ rungit v2.6.6 submodule deinit .\n>>        error: pathspec '.' did not match any file(s) known to git.\n>>        Did you forget to 'git add'?\n>>        $ >file && git add file\n>>        $ rungit v2.6.6 submodule deinit .\n>>        $ echo $?\n>>        0\n>\n> So instead of a path spec add a parameter '--all' to deinit all submodules\n\ns/path spec/pathspec/;\ns/a parameter '--all'/the '--all' option/;\n\n> and add a test to check for the corner case of an empty repository.\n>\n> The code only needs to learn about the '--all' parameter and doesn't\n\nLikewise.\n\n> require further changes as `git submodule--helper list \"$@\"` will list\n> all submodules in case of \"$@\" being empty.\n\nI'd propose doing s/in case of.../when \"$@\" is empty./\n\n> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\n> index 1572f05..24d7197 100644\n> --- a/Documentation/git-submodule.txt\n> +++ b/Documentation/git-submodule.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n>  \t      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]\n>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n>  'git submodule' [--quiet] init [--] [<path>...]\n> -'git submodule' [--quiet] deinit [-f|--force] [--] <path>...\n> +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...)\n>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n>  \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n>  \t      [--depth <depth>] [--recursive] [--] [<path>...]\n> @@ -144,6 +144,11 @@ deinit::\n>  \tyou really want to remove a submodule from the repository and commit\n>  \tthat use linkgit:git-rm[1] instead.\n>  +\n> +To unregister all submodules use `--all` with no path spec. In\n\ns/path spec/pathspec/;  But I'd rather see something more like this\ninstead of the first sentence:\n\n\tWhen the command is run without pathspec, it errors out,\n\tinstead of deinit-ing everything, to prevent mistakes.\n\n\n> +version 2.8 and prior, you were advised to give '.' to unregister\n> +all submodules. This may continue to work in the future, but as the\n> +path spec '.' may include regular files, this could stop working.\n\n\t... the command gave a suggestion to use '.' to unregister\n\tall submodules when it was invoked without any argument, but\n\tthis suggestion did not work and gave a wrong message if you\n\tfollowed in pathological cases and is no longer recommended.\n\nDo not predict the future in the documentation when we ourselves\nhave not committed to any concrete plan.\n\n>  If `--force` is specified, the submodule's work tree will be removed even if\n>  it contains local modifications.\n\nI think this sentence talks about \"working tree\" (as opposed to\n\"worktree\"), so s/work tree/working tree/.\n\n> @@ -247,6 +252,11 @@ OPTIONS\n>  --quiet::\n>  \tOnly print error messages.\n>  \n> +-a::\n> +--all::\n> +\tThis option is only valid for the deinit command. Unregister all\n> +\tsubmodules in the work tree.\n\nLikewise.\n\n>  -b::\n>  --branch::\n>  \tBranch of repository to add as submodule.\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43c68de..6dabb56 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -8,7 +8,7 @@ dashless=$(basename \"$0\" | sed -e 's/-/ /')\n>  USAGE=\"[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]\n>     or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]\n>     or: $dashless [--quiet] init [--] [<path>...]\n> -   or: $dashless [--quiet] deinit [-f|--force] [--] <path>...\n> +   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...)\n>     or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]\n>     or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]\n>     or: $dashless [--quiet] foreach [--recursive] <command>\n> @@ -521,6 +521,7 @@ cmd_init()\n>  cmd_deinit()\n>  {\n>  \t# parse $args after \"submodule ... deinit\".\n> +\tdeinit_all=\n>  \twhile test $# -ne 0\n>  \tdo\n>  \t\tcase \"$1\" in\n> @@ -530,6 +531,9 @@ cmd_deinit()\n>  \t\t-q|--quiet)\n>  \t\t\tGIT_QUIET=1\n>  \t\t\t;;\n> +\t\t-a|--all)\n> +\t\t\tdeinit_all=t\n> +\t\t\t;;\n>  \t\t--)\n>  \t\t\tshift\n>  \t\t\tbreak\n> @@ -544,9 +548,13 @@ cmd_deinit()\n>  \t\tshift\n>  \tdone\n>  \n> -\tif test $# = 0\n> +\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n> +\tthen\n> +\t\tdie \"$(eval_gettext \"usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\")\"\n\nI doubt that \"usage:\" wants to go thru l10n.\n\nI suspect that it is more friendly to the user to say that in prose,\ni.e.e.g. \"--all and pathspec cannot be given at the same time\", than\nforcing them to grok the (alternative|possibilities).\n\n> +\tfi\n> +\tif test $# = 0 && test -z \"$deinit_all\"\n>  \tthen\n> -\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n> +\t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n>  \tfi\n\nThis is good.\n\n>  \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index e1abd19..6e28ea5 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -11,6 +11,10 @@ subcommands of git submodule.\n>  \n>  . ./test-lib.sh\n>  \n> +test_expect_success 'submodule deinit works on empty repository' '\n> +\tgit submodule deinit --all\n> +'\n> +\n>  test_expect_success 'setup - initial commit' '\n>  \t>t &&\n>  \tgit add t &&\n> @@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n>  \t\t>file &&\n>  \t\tgit add file &&\n>  \t\tgit commit -m \"repo should not be empty\"\n> -\t\tgit submodule deinit .\n> +\t\tgit submodule deinit --all\n>  \t)\n>  '\n>  \n> @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '\n>  \trmdir init\n>  '\n>  \n> -test_expect_success 'submodule deinit . deinits all initialized submodules' '\n> +test_expect_success 'submodule deinit --all deinits all initialized submodules' '\n>  \tgit submodule update --init &&\n>  \tgit config submodule.example.foo bar &&\n>  \tgit config submodule.example2.frotz nitfol &&\n>  \ttest_must_fail git submodule deinit &&\n> -\tgit submodule deinit . >actual &&\n> +\tgit submodule deinit --all >actual &&\n>  \ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\" &&\n>  \ttest -z \"$(git config --get-regexp \"submodule\\.example2\\.\")\" &&\n>  \ttest_i18ngrep \"Cleared directory .init\" actual &&\n> @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n>  \tgit submodule deinit init >actual &&\n>  \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n>  \ttest_i18ngrep \"Cleared directory .init\" actual &&\n> -\tgit submodule deinit . >actual &&\n> +\tgit submodule deinit --all >actual &&\n>  \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n>  \ttest_i18ngrep \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n>  \ttest_i18ngrep \"Cleared directory .init\" actual &&\n> -\tgit submodule deinit . >actual &&\n> +\tgit submodule deinit --all >actual &&\n>  \ttest_i18ngrep ! \"Submodule .example. (.*) unregistered for path .init\" actual &&\n>  \ttest_i18ngrep ! \"Submodule .example2. (.*) unregistered for path .example2\" actual &&\n>  \ttest_i18ngrep \"Cleared directory .init\" actual &&\n\nI would have expected that we'd be testing both '.' and '--all', by\nkeeping the '.' tests as they were and adding tests for '--all'.  It\nis not like we are discouraging use of '.' when the repository is\nknown to have submodule(s) and '.' is expected to match.\n\nOther than that, looks good to me.  Thanks.\n"},{"id":"285518","messageId":"CAGZ79kbdnKMxrp_rQp7k-G6mveNYxES4XB-+2jh2e4+_RwPeyQ@mail.gmail.com","threadId":"42212","inReplyTo":"xmqqlh3pft91.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T21:34:11Z","receivedAt":"2016-05-04T21:34:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 4, 2016 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> I think this sentence talks about \"working tree\" (as opposed to\n> \"worktree\"), so s/work tree/working tree/.\n\nI'll fix this up in a resend, though it may be a fix on its own.\n\nSo the two \"official\" terms are working tree (files on your disk)\nand worktree (the command) and we don't want to have anything in between?\n(e.g. work tree for working tree?)\n\nOr as `grep -r \"work tree\"` puts it, we may want to have an extra cleanup patch\nand not do it here for this single occurrence.\n"},{"id":"285520","messageId":"xmqqzis5ebxk.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"CAGZ79kbdnKMxrp_rQp7k-G6mveNYxES4XB-+2jh2e4+_RwPeyQ@mail.gmail.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T21:43:51Z","receivedAt":"2016-05-04T21:43:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> (e.g. work tree for working tree?)\n\nThis was probably primarily my fault, not just because I've written\nmore than my share of documentation (compared to the code that\ntouched), but I was deliberately writing \"work tree\" when both \"work\ntree\" and \"working tree\" terms meant the same thing.  Compared to\nthe length of the timeperiod, the newcomer who is now known as\n\"worktree\" has only lived a very short period of time, so it is not\nsurprising to see remaining \"work tree\" in our documentation set.\n\nI think some attempts like 06cdac5a (git-reset.txt: use \"working\ntree\" consistently, 2010-09-15) were made to clean things up even\nbefore \"worktree\" started to mean an entirely different thing, and\nwe shouldn't make things worse by adding mentions of \"work tree\"\nwhen we mean \"working tree\".\n"},{"id":"285522","messageId":"CAGZ79kbLRDoAm5BaQ-OQ_OA19HBmhK3r6TWLiK1WMGGcTBVgtQ@mail.gmail.com","threadId":"42212","inReplyTo":"xmqqzis5ebxk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T21:47:22Z","receivedAt":"2016-05-04T21:47:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 4, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> (e.g. work tree for working tree?)\n>\n> This was probably primarily my fault, not just because I've written\n> more than my share of documentation (compared to the code that\n> touched), but I was deliberately writing \"work tree\" when both \"work\n> tree\" and \"working tree\" terms meant the same thing.  Compared to\n> the length of the timeperiod, the newcomer who is now known as\n> \"worktree\" has only lived a very short period of time, so it is not\n> surprising to see remaining \"work tree\" in our documentation set.\n\nSure.\n\n>\n> I think some attempts like 06cdac5a (git-reset.txt: use \"working\n> tree\" consistently, 2010-09-15) were made to clean things up even\n> before \"worktree\" started to mean an entirely different thing, and\n> we shouldn't make things worse by adding mentions of \"work tree\"\n> when we mean \"working tree\".\n\nOk. I'll see if I can send a similar patch like that.\n"},{"id":"285523","messageId":"xmqqshxxeboj.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"xmqqlh3pft91.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T21:49:16Z","receivedAt":"2016-05-04T21:49:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +\tif test -n \"$deinit_all\" && test \"$#\" -ne 0\n>> +\tthen\n>> +\t\tdie \"$(eval_gettext \"usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\")\"\n>\n> I doubt that \"usage:\" wants to go thru l10n.\n>\n> I suspect that it is more friendly to the user to say that in prose,\n> i.e.e.g. \"--all and pathspec cannot be given at the same time\", than\n> forcing them to grok the (alternative|possibilities).\n>\n>> +\tfi\n>> +\tif test $# = 0 && test -z \"$deinit_all\"\n>>  \tthen\n>> -\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n>> +\t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n>>  \tfi\n>\n> This is good.\n\nBy the way, while it is a very good idea to die upon\n\n\t$ git submodule deinit --all no-only-this-one\n\nit may not be too bad if we demoted the output to \"info\" with clean\nno-op exit when the user said\n\n\t$ git submodule deinit\n\nIOW, the latter part _might_ be better if it were\n\n\tif test $# = 0 && test -z \"$deinit_all\"\n\tthen\n\t\techo >&2 \"info: not deinitializing anything.\"\n                echo >&2 \"info: Use --all to deinitialize all submodules.\"\n                exit 0\n\tfi\n\ngiven that this is really about preventing mistakes from doing mass\ndestruction.\n"},{"id":"285524","messageId":"xmqqoa8lebkr.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"CAGZ79kbLRDoAm5BaQ-OQ_OA19HBmhK3r6TWLiK1WMGGcTBVgtQ@mail.gmail.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T21:51:32Z","receivedAt":"2016-05-04T21:51:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I think some attempts like 06cdac5a (git-reset.txt: use \"working\n>> tree\" consistently, 2010-09-15) were made to clean things up even\n>> before \"worktree\" started to mean an entirely different thing, and\n>> we shouldn't make things worse by adding mentions of \"work tree\"\n>> when we mean \"working tree\".\n>\n> Ok. I'll see if I can send a similar patch like that.\n\nPlease don't churn the documentation especially when there are more\nimportant changes in flight that deserve larger share of reviewer\nbandwidth.\n\nMy review comment on the documentation part of the patch was\nprimarily not to make things worse when you do not have to.\n"},{"id":"285527","messageId":"CAGZ79kbUJjqYbY3hC1W0Aw=gR49i4Q0BjY+qM9aiWG5ME89Ruw@mail.gmail.com","threadId":"42212","inReplyTo":"xmqqshxxeboj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T21:57:59Z","receivedAt":"2016-05-04T21:57:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 4, 2016 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> +    if test -n \"$deinit_all\" && test \"$#\" -ne 0\n>>> +    then\n>>> +            die \"$(eval_gettext \"usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\")\"\n>>\n>> I doubt that \"usage:\" wants to go thru l10n.\n>>\n>> I suspect that it is more friendly to the user to say that in prose,\n>> i.e.e.g. \"--all and pathspec cannot be given at the same time\", than\n>> forcing them to grok the (alternative|possibilities).\n>>\n>>> +    fi\n>>> +    if test $# = 0 && test -z \"$deinit_all\"\n>>>      then\n>>> -            die \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n>>> +            die \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n>>>      fi\n>>\n>> This is good.\n>\n> By the way, while it is a very good idea to die upon\n>\n>         $ git submodule deinit --all no-only-this-one\n>\n> it may not be too bad if we demoted the output to \"info\" with clean\n> no-op exit when the user said\n>\n>         $ git submodule deinit\n>\n> IOW, the latter part _might_ be better if it were\n>\n>         if test $# = 0 && test -z \"$deinit_all\"\n>         then\n>                 echo >&2 \"info: not deinitializing anything.\"\n>                 echo >&2 \"info: Use --all to deinitialize all submodules.\"\n>                 exit 0\n>         fi\n>\n> given that this is really about preventing mistakes from doing mass\n> destruction.\n\nDemote to a new class in a class of its own?\n\n* grep -r \"info:\" gives no match for user facing code, so it's a\nprefix you made up now.\n* I assume we want to translate it?\n* This would not respect the --quiet option?\n* returning 0 as in \"everyting is fine\", while nothing is fine.\n   There are two cases:\n     - Either the user knows what they are doing (See Per complaining\nabout this)\n       A very deliberate \"I want it all gone or error out if you\ncannot remove it all\"\n     - The user has no idea what they are doing. Exiting non zero\ndoesn't do any harm.\n\nMaybe:\n\n> -             die \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n> +            usage()\n\ninstead?\n"},{"id":"285528","messageId":"xmqqfutxeawm.fsf@gitster.mtv.corp.google.com","threadId":"42212","inReplyTo":"CAGZ79kbUJjqYbY3hC1W0Aw=gR49i4Q0BjY+qM9aiWG5ME89Ruw@mail.gmail.com","subject":"Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T22:06:01Z","receivedAt":"2016-05-04T22:06:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> IOW, the latter part _might_ be better if it were\n>>\n>>         if test $# = 0 && test -z \"$deinit_all\"\n>>         then\n>>                 echo >&2 \"info: not deinitializing anything.\"\n>>                 echo >&2 \"info: Use --all to deinitialize all submodules.\"\n>>                 exit 0\n>>         fi\n>>\n>> given that this is really about preventing mistakes from doing mass\n>> destruction.\n>  ...\n> Maybe:\n>\n>> -             die \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n>> +            usage()\n>\n> instead?\n\nI was primarily concerned about die giving a non-zero exit status\nwhen \"git submodule deinit\" did not find anything worthwhile to do\n(because we specified nothing on the command line to deinit).  IOW,\nit was an attempt to do \"You asked me a no-op, so I am doing a\nno-op, but if I stayed silent, you wouldn't even notice it, and you\nmight have meant to deinit all, so I am telling you I didn't do\nanything, and advising how to say 'deinit all' to me.\"\n\nBut what we see in the patch under discussion is perfectly fine; it\nbehaves just like \"$ rm<RET>\" and \"$ git add<RET>\" that give an\nerror message and exit with a non-zero status.\n"}]}