{"thread":{"id":"42220","subject":"[PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","startedAt":"2016-05-04T22:40:03Z","lastAt":"2016-05-05T19:35:43Z","messageCount":11,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"285534","messageId":"1462401603-2067-1-git-send-email-sbeller@google.com","threadId":"42220","inReplyTo":null,"subject":"[PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T22:40:03Z","receivedAt":"2016-05-04T22:40:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The discussion in [1] pointed out 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 pathspec add the '--all' option 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' option and doesn't\nrequire further changes as `git submodule--helper list \"$@\"` will list\nall submodules when \"$@\" is 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* reworded commit message slightly (realize, pathspec)\n* reworded the documentation \n* no \"usage:\" to go thru l10n.\n\n Documentation/git-submodule.txt | 25 +++++++++++++++++++------\n git-submodule.sh                | 14 +++++++++++---\n t/t7400-submodule-basic.sh      | 28 +++++++++++++++++++++++++++-\n 3 files changed, 57 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 1572f05..aeeb2ed 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@@ -140,12 +140,20 @@ deinit::\n \ttree. Further calls to `git submodule update`, `git submodule foreach`\n \tand `git submodule sync` will skip any unregistered submodules until\n \tthey are initialized again, so use this command if you don't want to\n-\thave a local checkout of the submodule in your work tree anymore. If\n+\thave a local checkout of the submodule in your working tree anymore. If\n \tyou really want to remove a submodule from the repository and commit\n \tthat use linkgit:git-rm[1] instead.\n +\n-If `--force` is specified, the submodule's work tree will be removed even if\n-it contains local modifications.\n+When the command is run without pathspec, it errors out,\n+instead of deinit-ing everything, to prevent mistakes. In\n+version 2.8 and before the command gave a suggestion to use\n+'.' to unregister all submodules when it was invoked without\n+any argument, but this suggestion did not work and gave a\n+wrong message if you followed it in pathological cases and is\n+no longer recommended.\n++\n+If `--force` is specified, the submodule's working tree will\n+be removed even if it contains local modifications.\n \n update::\n +\n@@ -247,6 +255,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 working tree.\n+\n -b::\n --branch::\n \tBranch of repository to add as submodule.\n@@ -257,8 +270,8 @@ OPTIONS\n --force::\n \tThis option is only valid for add, deinit and update commands.\n \tWhen running add, allow adding an otherwise ignored submodule path.\n-\tWhen running deinit the submodule work trees will be removed even if\n-\tthey contain local changes.\n+\tWhen running deinit the submodule working trees will be removed even\n+\tif they contain local changes.\n \tWhen running update (only effective with the checkout procedure),\n \tthrow away local changes in submodules when switching to a\n \tdifferent commit; and always run a checkout operation in the\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..caa80fe 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 \"--all and pathspec are incompatible\")\"\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..bd0d229 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,8 @@ 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 . &&\n+\t\tgit submodule deinit --all\n \t)\n '\n \n@@ -900,6 +905,19 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '\n \trmdir init example2\n '\n \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 --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+\ttest_i18ngrep \"Cleared directory .example2\" actual &&\n+\trmdir init example2\n+'\n+\n test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '\n \tgit submodule update --init &&\n \trm -rf init example2/* example2/.git &&\n@@ -966,6 +984,14 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\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 --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 -a >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 \trmdir init example2\n '\n \n-- \n2.8.0.rc4.9.g082ee08.dirty\n"},{"id":"285543","messageId":"xmqqshxxctgf.fsf@gitster.mtv.corp.google.com","threadId":"42220","inReplyTo":"1462401603-2067-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-04T23:08:16Z","receivedAt":"2016-05-04T23:08:16Z","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> +When the command is run without pathspec, it errors out,\n> +instead of deinit-ing everything, to prevent mistakes. In\n> +version 2.8 and before the command gave a suggestion to use\n> +'.' to unregister all submodules when it was invoked without\n> +any argument, but this suggestion did not work and gave a\n> +wrong message if you followed it in pathological cases and is\n> +no longer recommended.\n\nThanks for noticing and fixing my typo ;-)\n\nI read everything again and it all looks good.\n\nThanks.\n"},{"id":"285545","messageId":"20160504232642.GC395@google.com","threadId":"42220","inReplyTo":"1462401603-2067-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-05-04T23:26:42Z","receivedAt":"2016-05-04T23:26:42Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> * reworded commit message slightly (realize, pathspec)\n> * reworded the documentation \n\nYay, thanks for your work on this.\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> @@ -140,12 +140,20 @@ deinit::\n>  \ttree. Further calls to `git submodule update`, `git submodule foreach`\n>  \tand `git submodule sync` will skip any unregistered submodules until\n>  \tthey are initialized again, so use this command if you don't want to\n> -\thave a local checkout of the submodule in your work tree anymore. If\n> +\thave a local checkout of the submodule in your working tree anymore. If\n>  \tyou really want to remove a submodule from the repository and commit\n>  \tthat use linkgit:git-rm[1] instead.\n> ++\n> +When the command is run without pathspec, it errors out,\n> +instead of deinit-ing everything, to prevent mistakes. In\n> +version 2.8 and before the command gave a suggestion to use\n> +'.' to unregister all submodules when it was invoked without\n> +any argument, but this suggestion did not work and gave a\n> +wrong message if you followed it in pathological cases and is\n> +no longer recommended.\n\nWhy tell the user what happened in 2.8 and earlier?  It's not clear what\nthe reader would do with that information.\n\nI think this paragraph could be removed.  --all is explained lower\ndown and the error message points it out to users who need it.\n\n>  +\n> -If `--force` is specified, the submodule's work tree will be removed even if\n> -it contains local modifications.\n> +If `--force` is specified, the submodule's working tree will\n> +be removed even if it contains local modifications.\n\n(unnecessary rewrapping)\n\n[...]\n>  update::\n>  +\n> @@ -247,6 +255,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 working tree.\n\nThis could use an explanation of why I'd want to use it.  E.g.\n\n\tThis option is only valid for the deinit command. Unregister all\n\tsubmodules. Scripts should use this option instead of passing '.'\n\tto deinit because it works even in an empty repository with no\n\tsubmodules present.\n\nNot about this patch: the organization of options is a little strange.\nA separate section with options for each subcommand would be easier to\nread.\n\nDo we want to claim the short-and-sweet option -a?  (I don't mind but it\ndoesn't seem necessary.)\n\n[...]\n> @@ -257,8 +270,8 @@ OPTIONS\n>  --force::\n>  \tThis option is only valid for add, deinit and update commands.\n>  \tWhen running add, allow adding an otherwise ignored submodule path.\n> -\tWhen running deinit the submodule work trees will be removed even if\n> -\tthey contain local changes.\n> +\tWhen running deinit the submodule working trees will be removed even\n> +\tif they contain local changes.\n\nUnrelated change?\n\n[...]\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 and pathspec are incompatible\")\"\n\nThis message still feels too low-level to me, but I might be swimming\nuphill here.\n\nAnother option would be to call 'usage' and be done.\n\n[...]\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n\nMakes sense.\n\nIn the context of the original motivation: this patch improves the\nadvice printed by plain \"git submodule deinit\" but doesn't help with\nexisting callers that might have run \"git submodule deinit .\".  It might\nmake sense to handle '.' as a historical special case in a separate\npatch.\n\nThanks and sorry for all the complication,\nJonathan\n"},{"id":"285548","messageId":"CAGZ79kbeCCcmGh57zUdQ=BzFOWUiwj8-3nM4dbK9yONbrmLaPw@mail.gmail.com","threadId":"42220","inReplyTo":"20160504232642.GC395@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-04T23:38:38Z","receivedAt":"2016-05-04T23:38:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>> * reworded commit message slightly (realize, pathspec)\n>> * reworded the documentation\n>\n> Yay, thanks for your work on this.\n>\n> [...]\n>> +++ b/Documentation/git-submodule.txt\n>> @@ -13,7 +13,7 @@ SYNOPSIS\n>>             [--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>>             [-f|--force] [--rebase|--merge] [--reference <repository>]\n>>             [--depth <depth>] [--recursive] [--] [<path>...]\n>> @@ -140,12 +140,20 @@ deinit::\n>>       tree. Further calls to `git submodule update`, `git submodule foreach`\n>>       and `git submodule sync` will skip any unregistered submodules until\n>>       they are initialized again, so use this command if you don't want to\n>> -     have a local checkout of the submodule in your work tree anymore. If\n>> +     have a local checkout of the submodule in your working tree anymore. If\n>>       you really want to remove a submodule from the repository and commit\n>>       that use linkgit:git-rm[1] instead.\n>> ++\n>> +When the command is run without pathspec, it errors out,\n>> +instead of deinit-ing everything, to prevent mistakes. In\n>> +version 2.8 and before the command gave a suggestion to use\n>> +'.' to unregister all submodules when it was invoked without\n>> +any argument, but this suggestion did not work and gave a\n>> +wrong message if you followed it in pathological cases and is\n>> +no longer recommended.\n>\n> Why tell the user what happened in 2.8 and earlier?  It's not clear what\n> the reader would do with that information.\n\nBecause people may wonder what happened to '.' ?\n\n>\n> I think this paragraph could be removed.  --all is explained lower\n> down and the error message points it out to users who need it.\n\nWhen we want to keep supporting '.' forever, I would remove this section.\n\n>\n>>  +\n>> -If `--force` is specified, the submodule's work tree will be removed even if\n>> -it contains local modifications.\n>> +If `--force` is specified, the submodule's working tree will\n>> +be removed even if it contains local modifications.\n>\n> (unnecessary rewrapping)\n>\n> [...]\n>>  update::\n>>  +\n>> @@ -247,6 +255,11 @@ OPTIONS\n>>  --quiet::\n>>       Only print error messages.\n>>\n>> +-a::\n>> +--all::\n>> +     This option is only valid for the deinit command. Unregister all\n>> +     submodules in the working tree.\n>\n> This could use an explanation of why I'd want to use it.  E.g.\n>\n>         This option is only valid for the deinit command. Unregister all\n>         submodules. Scripts should use this option instead of passing '.'\n>         to deinit because it works even in an empty repository with no\n>         submodules present.\n\nI would not want to mention '.' in the documentation. this can read:\n\n    As a user I am fine to use '.' and then I wonder when it breaks.\n\n\n>\n> Not about this patch: the organization of options is a little strange.\n> A separate section with options for each subcommand would be easier to\n> read.\n\nI agree.\n\n>\n> Do we want to claim the short-and-sweet option -a?  (I don't mind but it\n> doesn't seem necessary.)\n\nWe do.\n\n>\n> [...]\n>> @@ -257,8 +270,8 @@ OPTIONS\n>>  --force::\n>>       This option is only valid for add, deinit and update commands.\n>>       When running add, allow adding an otherwise ignored submodule path.\n>> -     When running deinit the submodule work trees will be removed even if\n>> -     they contain local changes.\n>> +     When running deinit the submodule working trees will be removed even\n>> +     if they contain local changes.\n>\n> Unrelated change?\n\nIt's close enough for deinit to squash it in here, no?\n\n\n>\n> [...]\n>> @@ -544,9 +548,13 @@ cmd_deinit()\n>>               shift\n>>       done\n>>\n>> -     if test $# = 0\n>> +     if test -n \"$deinit_all\" && test \"$#\" -ne 0\n>> +     then\n>> +             die \"$(eval_gettext \"--all and pathspec are incompatible\")\"\n>\n> This message still feels too low-level to me, but I might be swimming\n> uphill here.\n>\n> Another option would be to call 'usage' and be done.\n\nI had that idea as well, but I think pointing out the low level is better\nthan giving the high level again, so the user immediately sees what's wrong.\n\n>\n> [...]\n>> --- a/t/t7400-submodule-basic.sh\n>> +++ b/t/t7400-submodule-basic.sh\n>\n> Makes sense.\n>\n> In the context of the original motivation: this patch improves the\n> advice printed by plain \"git submodule deinit\" but doesn't help with\n> existing callers that might have run \"git submodule deinit .\".  It might\n> make sense to handle '.' as a historical special case in a separate\n> patch.\n\nOnce we change how '.' is handled we can do that?\n\n>\n> Thanks and sorry for all the complication,\n> Jonathan\n"},{"id":"285551","messageId":"20160504235914.GD395@google.com","threadId":"42220","inReplyTo":"CAGZ79kbeCCcmGh57zUdQ=BzFOWUiwj8-3nM4dbK9yONbrmLaPw@mail.gmail.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-05-04T23:59:14Z","receivedAt":"2016-05-04T23:59:14Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> I think this paragraph could be removed.  --all is explained lower\n>> down and the error message points it out to users who need it.\n>\n> When we want to keep supporting '.' forever, I would remove this section.\n\nYes, please.  We can't remove something like that without a deprecation\nprocess that I don't think is worth it here.\n\n[...]\n>>> +-a::\n>>> +--all::\n>>> +     This option is only valid for the deinit command. Unregister all\n>>> +     submodules in the working tree.\n>>\n>> This could use an explanation of why I'd want to use it.  E.g.\n>>\n>>         This option is only valid for the deinit command. Unregister all\n>>         submodules. Scripts should use this option instead of passing '.'\n>>         to deinit because it works even in an empty repository with no\n>>         submodules present.\n>\n> I would not want to mention '.' in the documentation. this can read:\n>\n>     As a user I am fine to use '.' and then I wonder when it breaks.\n\nSorry for the lack of clarity.  By referencing scripts I was referring\nto \"callers that want to be able to run the same command in all\nsituations, even the edge case of no files present\".  I agree with you\nthat humans should care just as much as scripts about things that will\nbreak and that we shouldn't break them. :)\n\n[...]\n>>> @@ -257,8 +270,8 @@ OPTIONS\n>>>  --force::\n>>>       This option is only valid for add, deinit and update commands.\n>>>       When running add, allow adding an otherwise ignored submodule path.\n>>> -     When running deinit the submodule work trees will be removed even if\n>>> -     they contain local changes.\n>>> +     When running deinit the submodule working trees will be removed even\n>>> +     if they contain local changes.\n>>\n>> Unrelated change?\n>\n> It's close enough for deinit to squash it in here, no?\n\nMy preference would be to have a separate patch since its commit message\ncan explain the purpose.  I don't care much --- it was just something I\nnoticed while reviewing the rest.\n\n[...]\n>>> +     if test -n \"$deinit_all\" && test \"$#\" -ne 0\n>>> +     then\n>>> +             die \"$(eval_gettext \"--all and pathspec are incompatible\")\"\n>>\n>> This message still feels too low-level to me, but I might be swimming\n>> uphill here.\n>>\n>> Another option would be to call 'usage' and be done.\n>\n> I had that idea as well, but I think pointing out the low level is better\n> than giving the high level again, so the user immediately sees what's wrong.\n\nI mean low level as in implementation detail.  The human user would\nwonder \"what is incompatible about them?  Why are you stopping me from\nwhat I am trying to do?\"  Most likely the user was trying to do\nsomething other than specify a path, since they also passed --all.  If\nI run something like\n\n\tgit submodule deinit force --all\n\nand the output tells me that --all and pathspec are incompatible then\nI just scratch my head more.\n\nWe can do\n\n\tUSAGE=\"$dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\"\n\tusage\n\nto print the subcommand's usage.  git commandline tools don't\ntranslate any usage strings today, so not getting translation here\nwouldn't feel out of place.\n\n[...]\n>> In the context of the original motivation: this patch improves the\n>> advice printed by plain \"git submodule deinit\" but doesn't help with\n>> existing callers that might have run \"git submodule deinit .\".  It might\n>> make sense to handle '.' as a historical special case in a separate\n>> patch.\n>\n> Once we change how '.' is handled we can do that?\n\nIt's harmless even before then.  Anyway, I meant \"as a preparatory step\nbefore such a change that would otherwise be backward incompatible.\"\n\nThanks,\nJonathan\n"},{"id":"285579","messageId":"xmqqr3dgbd3k.fsf@gitster.mtv.corp.google.com","threadId":"42220","inReplyTo":"CAGZ79kbeCCcmGh57zUdQ=BzFOWUiwj8-3nM4dbK9yONbrmLaPw@mail.gmail.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T17:59:11Z","receivedAt":"2016-05-05T17:59:11Z","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>>> +When the command is run without pathspec, it errors out,\n>>> +instead of deinit-ing everything, to prevent mistakes. In\n>>> +version 2.8 and before the command gave a suggestion to use\n>>> +'.' to unregister all submodules when it was invoked without\n>>> +any argument, but this suggestion did not work and gave a\n>>> +wrong message if you followed it in pathological cases and is\n>>> +no longer recommended.\n>>\n>> Why tell the user what happened in 2.8 and earlier?  It's not clear what\n>> the reader would do with that information.\n>\n> Because people may wonder what happened to '.' ?\n\nI am to blame on that final text, but I think Jonathan is right.\n\"In version 2.8 and earlier...\" can just go.  Users may need to \nunderstand why no-arg form is not a silent no-op but an error,\nand they need to know how to de-init everything with the version\nof Git they have (i.e. with \"--all\").  Compared to these two,\n\"Your fingers may have been trained to say '.', but it was found\nnot to work in pathological cases\" is of much lessor importance,\nespecially because with or without this patch, the definition of\n\"pathological\" cases does not change.\n\n>> I think this paragraph could be removed.  --all is explained lower\n>> down and the error message points it out to users who need it.\n>\n> When we want to keep supporting '.' forever, I would remove this section.\n\nI am not sure what you mean by \"keep supporting '.'\".  If your\nrepository has any tracked path, \"deinit .\" would deinit all\nsubmodules, with or without this change.\n\nAre you worried about the future change you are planning to that\ninvolves reverting 84ba959b (submodule: fix regression for deinit\nwithout submodules, 2016-03-22), after which a pathspec that does\nnot match any submodule would become a \"possible typo\" error?\n\nIt is true that '.' would error out if there is no submodule in the\nrepository, as opposed to erroring out only when there is no tracked\npath, which is what you get with today's version (and the version\nwith this fix in the patch under discussion).  But '.' is not\nspecial with respect to that change.  'README' would also error out\nif there is no submodule whose path matches that pathspec in that\nfuture version, as opposed to erroring out only if 'README' is not\ntracked at all in today's version.\n\nOr are you thinking that it may be better to give '.' a special\nmeaning, iow, not treating it as just a regular pathspec?  Perhaps\nmake '.' to mean \"everything but it is not an error if there is none\nto begin with\"?  I fear that going in that direction would deform\nthe mental model the users would form from seeing how commands\nbehave when given a pathspec.  The \".\" would still look like any\nother pathspec elements, and I am sure you will not special case \".\"\nin the usage string but will claim that it is covered by the mention\nof <pathspec> at the end of the command line in the usage string,\nso you are making them expect that \".\" used as a pathspec would\nbehave like that for all other places that we take pathspec, when\nin reality, only \"submodule deinit\" make it behave differently.\n\nWhich I do not think is particularly a good idea.\n\n>> Not about this patch: the organization of options is a little strange.\n>> A separate section with options for each subcommand would be easier to\n>> read.\n>\n> I agree.\n\nI agree.\n\n>> Do we want to claim the short-and-sweet option -a?  (I don't mind but it\n>> doesn't seem necessary.)\n>\n> We do.\n\nI don't, but I do not care too deeaply.\n\n\n>>> @@ -257,8 +270,8 @@ OPTIONS\n>>>  --force::\n>>>       This option is only valid for add, deinit and update commands.\n>>>       When running add, allow adding an otherwise ignored submodule path.\n>>> -     When running deinit the submodule work trees will be removed even if\n>>> -     they contain local changes.\n>>> +     When running deinit the submodule working trees will be removed even\n>>> +     if they contain local changes.\n>>\n>> Unrelated change?\n>\n> It's close enough for deinit to squash it in here, no?\n\nMore importantly, the patch adds a new instance of \"working tree\" to\nthe documentation elsewhere; fixing this existing instance of \"work\ntree\" is relevant from consistency's point of view.\n\n>>> @@ -544,9 +548,13 @@ cmd_deinit()\n>>>               shift\n>>>       done\n>>>\n>>> -     if test $# = 0\n>>> +     if test -n \"$deinit_all\" && test \"$#\" -ne 0\n>>> +     then\n>>> +             die \"$(eval_gettext \"--all and pathspec are incompatible\")\"\n>>\n>> This message still feels too low-level to me, but I might be swimming\n>> uphill here.\n>>\n>> Another option would be to call 'usage' and be done.\n>\n> I had that idea as well, but I think pointing out the low level is better\n> than giving the high level again, so the user immediately sees what's wrong.\n\nI do not particularly see the message low-level.  Jonathan, what do\nyou have against pointing out the exact problem?  After seeing the\nusage string that also talks about --quite, --force, etc., I have to\nsomehow realize that these are irrelevant noises that have nothing\nto do with the error, and puzzle out that the (choose|from|here) is\ntelling me that I cannot give pathspec when I am giving --all\nmyself.\n\n> Once we change how '.' is handled we can do that?\n\nAgain, I am worried about \"Once we change how ...\".\n"},{"id":"285581","messageId":"xmqqmvo4bcwf.fsf@gitster.mtv.corp.google.com","threadId":"42220","inReplyTo":"20160504235914.GD395@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T18:03:28Z","receivedAt":"2016-05-05T18:03:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I mean low level as in implementation detail.  The human user would\n> wonder \"what is incompatible about them?  Why are you stopping me from\n> what I am trying to do?\"\n\nMaybe s/incompatible/inconsistent/ is what you are after?  Why are\nyou stopping me from what I am trying to do?  Because you are asking\nto do two contradicting things.  Do you want to nuke everything, or\ndo you want to keep everything outside what you specified?\n\nAfter being saved countless times from a stupid mistake\n\n    $ git commit -a -s foo.c\n\nthat was caused by habitually typing \"-a\", when I do want to limit\nthe changes to record to a specific path (or two) with similar\nsafety, I do not think \"what is incompatible about them\" is a valid\nquestion.\n\n\n    \n"},{"id":"285582","messageId":"CAGZ79kah7Ry5j6+2n4ndcw5=RqT7wDOsRL7zPE7UphtSE5ib1Q@mail.gmail.com","threadId":"42220","inReplyTo":"xmqqr3dgbd3k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T18:11:17Z","receivedAt":"2016-05-05T18:11:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>>> +When the command is run without pathspec, it errors out,\n>>>> +instead of deinit-ing everything, to prevent mistakes. In\n>>>> +version 2.8 and before the command gave a suggestion to use\n>>>> +'.' to unregister all submodules when it was invoked without\n>>>> +any argument, but this suggestion did not work and gave a\n>>>> +wrong message if you followed it in pathological cases and is\n>>>> +no longer recommended.\n>>>\n>>> Why tell the user what happened in 2.8 and earlier?  It's not clear what\n>>> the reader would do with that information.\n>>\n>> Because people may wonder what happened to '.' ?\n>\n> I am to blame on that final text, but I think Jonathan is right.\n> \"In version 2.8 and earlier...\" can just go.  Users may need to\n> understand why no-arg form is not a silent no-op but an error,\n> and they need to know how to de-init everything with the version\n> of Git they have (i.e. with \"--all\").  Compared to these two,\n> \"Your fingers may have been trained to say '.', but it was found\n> not to work in pathological cases\" is of much lessor importance,\n> especially because with or without this patch, the definition of\n> \"pathological\" cases does not change.\n\nSo we'll only give\n\n    When the command is run without pathspec, it errors out,\n    instead of deinit-ing everything, to prevent mistakes.\n\n>\n>>> I think this paragraph could be removed.  --all is explained lower\n>>> down and the error message points it out to users who need it.\n>>\n>> When we want to keep supporting '.' forever, I would remove this section.\n>\n> I am not sure what you mean by \"keep supporting '.'\".  If your\n> repository has any tracked path, \"deinit .\" would deinit all\n> submodules, with or without this change.\n>\n> Are you worried about the future change you are planning to that\n> involves reverting 84ba959b (submodule: fix regression for deinit\n> without submodules, 2016-03-22), after which a pathspec that does\n> not match any submodule would become a \"possible typo\" error?\n\nWhen redoing the groups series, I may or may not go that route.\nIt sounds compelling to me.\n\n>\n> It is true that '.' would error out if there is no submodule in the\n> repository, as opposed to erroring out only when there is no tracked\n> path, which is what you get with today's version (and the version\n> with this fix in the patch under discussion).  But '.' is not\n> special with respect to that change.  'README' would also error out\n> if there is no submodule whose path matches that pathspec in that\n> future version, as opposed to erroring out only if 'README' is not\n> tracked at all in today's version.\n>\n> Or are you thinking that it may be better to give '.' a special\n> meaning, iow, not treating it as just a regular pathspec?  Perhaps\n> make '.' to mean \"everything but it is not an error if there is none\n> to begin with\"?  I fear that going in that direction would deform\n> the mental model the users would form from seeing how commands\n> behave when given a pathspec.  The \".\" would still look like any\n> other pathspec elements, and I am sure you will not special case \".\"\n> in the usage string but will claim that it is covered by the mention\n> of <pathspec> at the end of the command line in the usage string,\n> so you are making them expect that \".\" used as a pathspec would\n> behave like that for all other places that we take pathspec, when\n> in reality, only \"submodule deinit\" make it behave differently.\n>\n> Which I do not think is particularly a good idea.\n\nI did not think special casing '.'. (I did in the very first patch, but I\nunderstand that it's a bad idea now, so I do not think of it again)\n\n>\n>>> Not about this patch: the organization of options is a little strange.\n>>> A separate section with options for each subcommand would be easier to\n>>> read.\n>>\n>> I agree.\n>\n> I agree.\n>\n>>> Do we want to claim the short-and-sweet option -a?  (I don't mind but it\n>>> doesn't seem necessary.)\n>>\n>> We do.\n>\n> I don't, but I do not care too deeaply.\n\nMe neither, so I'll remove the short option.\n\n>\n>\n>>>> @@ -257,8 +270,8 @@ OPTIONS\n>>>>  --force::\n>>>>       This option is only valid for add, deinit and update commands.\n>>>>       When running add, allow adding an otherwise ignored submodule path.\n>>>> -     When running deinit the submodule work trees will be removed even if\n>>>> -     they contain local changes.\n>>>> +     When running deinit the submodule working trees will be removed even\n>>>> +     if they contain local changes.\n>>>\n>>> Unrelated change?\n>>\n>> It's close enough for deinit to squash it in here, no?\n>\n> More importantly, the patch adds a new instance of \"working tree\" to\n> the documentation elsewhere; fixing this existing instance of \"work\n> tree\" is relevant from consistency's point of view.\n>\n>>>> @@ -544,9 +548,13 @@ cmd_deinit()\n>>>>               shift\n>>>>       done\n>>>>\n>>>> -     if test $# = 0\n>>>> +     if test -n \"$deinit_all\" && test \"$#\" -ne 0\n>>>> +     then\n>>>> +             die \"$(eval_gettext \"--all and pathspec are incompatible\")\"\n>>>\n>>> This message still feels too low-level to me, but I might be swimming\n>>> uphill here.\n>>>\n>>> Another option would be to call 'usage' and be done.\n>>\n>> I had that idea as well, but I think pointing out the low level is better\n>> than giving the high level again, so the user immediately sees what's wrong.\n>\n> I do not particularly see the message low-level.  Jonathan, what do\n> you have against pointing out the exact problem?  After seeing the\n> usage string that also talks about --quite, --force, etc., I have to\n> somehow realize that these are irrelevant noises that have nothing\n> to do with the error, and puzzle out that the (choose|from|here) is\n> telling me that I cannot give pathspec when I am giving --all\n> myself.\n>\n>> Once we change how '.' is handled we can do that?\n>\n> Again, I am worried about \"Once we change how ...\".\n\nBy that I mainly mean reverting 84ba959b (submodule: fix\nregression for deinit without submodules, 2016-03-22),\nbut I am aware that this is a major change as it breaks\nexisting users.\n\nThanks,\nStefan\n"},{"id":"285583","messageId":"CAGZ79kbxDbmeh-rTHAqykjLBZ47HAviA11Q9LLYpcwT-Wr9X0A@mail.gmail.com","threadId":"42220","inReplyTo":"20160504235914.GD395@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T19:02:31Z","receivedAt":"2016-05-05T19:02:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 4, 2016 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>>>\n>>> Another option would be to call 'usage' and be done.\n>>\n>> I had that idea as well, but I think pointing out the low level is better\n>> than giving the high level again, so the user immediately sees what's wrong.\n>\n> I mean low level as in implementation detail.  The human user would\n> wonder \"what is incompatible about them?  Why are you stopping me from\n> what I am trying to do?\"  Most likely the user was trying to do\n> something other than specify a path, since they also passed --all.  If\n> I run something like\n>\n>\n\n\n    $ git submodule deinit force --all\n    error: unknown option `all'\n    usage: git submodule--helper list [--prefix=<path>] [<path>...]\n\n        --prefix <path>       alternative anchor for relative paths\n\n`force` is seen as the first pathspec, so \"force --all\" is given to the\n`submodule--helper list`, which gives a less than optimal error message\n\n    $ git submodule deinit --all force\ngettext: unrecognized option '--all and pathspec are incompatible'\nTry 'gettext --help' for more information.\nenvsubst: unrecognized option '--all and pathspec are incompatible'\nTry 'envsubst --help' for more information.\nenvsubst: unrecognized option '--all and pathspec are incompatible'\nTry 'envsubst --help' for more information.\n\nWe should not put --all as the first thing in the error message.\n\n    $ git submodule deinit --all force\n    pathspec and --all are incompatible\n\nWith the next patch this is the error message. I think the missing\ndashes for force are quite visible as force is after --all.\n\n>\n> and the output tells me that --all and pathspec are incompatible then\n> I just scratch my head more.\n>\n> We can do\n>\n>         USAGE=\"$dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)\"\n>         usage\n>\n> to print the subcommand's usage.  git commandline tools don't\n> translate any usage strings today, so not getting translation here\n> wouldn't feel out of place.\n\nWe can have both? I'd prefer not rewriting the USAGE string here\nas it would easily be out of sync in the future?\nI tried to just grep the deinit line from the USAGe though, but that doesn't\nlook right as it would need some post processing. (remove the \"or:\")\nand processing the USAGE string also doesn't sound future proof.\n"},{"id":"285587","messageId":"20160505192036.GA22726@google.com","threadId":"42220","inReplyTo":"xmqqmvo4bcwf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-05-05T19:20:37Z","receivedAt":"2016-05-05T19:20:37Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> I mean low level as in implementation detail.  The human user would\n>> wonder \"what is incompatible about them?  Why are you stopping me from\n>> what I am trying to do?\"\n>\n> Maybe s/incompatible/inconsistent/ is what you are after?  Why are\n> you stopping me from what I am trying to do?  Because you are asking\n> to do two contradicting things.  Do you want to nuke everything, or\n> do you want to keep everything outside what you specified?\n>\n> After being saved countless times from a stupid mistake\n>\n>     $ git commit -a -s foo.c\n>\n> that was caused by habitually typing \"-a\", when I do want to limit\n> the changes to record to a specific path (or two) with similar\n> safety, I do not think \"what is incompatible about them\" is a valid\n> question.\n\nYes, 'inconsistent' works better than 'incompatible'.  Stepping back,\nwhat I meant is that when I pass an invalid set of command line\narguments, it's difficult to give advice back because it's not obvious\nwhat I intended to do.\n\nWhen I say \"git submodule deinit --all -- foo/\", I'm most likely trying\nto deinit all submodules within the foo subdirectory.  That's a\nperfectly consistent thing to want --- it just doesn't match what the\ncommand is expecting.  It is more helpful for the command to tell me\nwhat it is expecting me to do instead instead of just telling me that what\nI gave it is wrong.\n\nThe ideal would be something like \"git check-attr\"'s error_with_usage.\nIt tells in a targetted way where my error was and also gives a guide\nof what to do instead.\n\n\t$ git submodule deinit --all lib/\n\terror: paths with --all do not make sense\n\tusage: git submodule deinit [-f | --force] (--all | [--] <path>...)\n\nThanks,\nJonathan\n\n-- >8 --\nSubject: git-sh-setup: add error_with_usage helper\n\nWhen given an impossible set of options, this allows a command to\nprint a targetted message followed by a short usage string that tells\nthe user both (1) what part of their command wasn't supported (what\nwent wrong) and (2) what correct usage looks like (what to do\ninstead).\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n git-sh-setup.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex c48139a..2b56636 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -63,6 +63,11 @@ say () {\n \tfi\n }\n \n+error_with_usage () {\n+\tprintf >&2 'error: %s\\n' \"$*\"\n+\tusage\n+}\n+\n if test -n \"$OPTIONS_SPEC\"; then\n \tusage() {\n \t\t\"$0\" -h\n-- \n"},{"id":"285590","messageId":"CAGZ79kbsgddnrF+oibSrpRu9e3PP2DTWMesqsdHCDHdySnhTyQ@mail.gmail.com","threadId":"42220","inReplyTo":"20160505192036.GA22726@google.com","subject":"Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T19:35:43Z","receivedAt":"2016-05-05T19:35:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 12:20 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Junio C Hamano wrote:\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>> I mean low level as in implementation detail.  The human user would\n>>> wonder \"what is incompatible about them?  Why are you stopping me from\n>>> what I am trying to do?\"\n>>\n>> Maybe s/incompatible/inconsistent/ is what you are after?  Why are\n>> you stopping me from what I am trying to do?  Because you are asking\n>> to do two contradicting things.  Do you want to nuke everything, or\n>> do you want to keep everything outside what you specified?\n>>\n>> After being saved countless times from a stupid mistake\n>>\n>>     $ git commit -a -s foo.c\n>>\n>> that was caused by habitually typing \"-a\", when I do want to limit\n>> the changes to record to a specific path (or two) with similar\n>> safety, I do not think \"what is incompatible about them\" is a valid\n>> question.\n>\n> Yes, 'inconsistent' works better than 'incompatible'.  Stepping back,\n> what I meant is that when I pass an invalid set of command line\n> arguments, it's difficult to give advice back because it's not obvious\n> what I intended to do.\n>\n> When I say \"git submodule deinit --all -- foo/\", I'm most likely trying\n> to deinit all submodules within the foo subdirectory.  That's a\n> perfectly consistent thing to want --- it just doesn't match what the\n> command is expecting.  It is more helpful for the command to tell me\n> what it is expecting me to do instead instead of just telling me that what\n> I gave it is wrong.\n>\n> The ideal would be something like \"git check-attr\"'s error_with_usage.\n> It tells in a targetted way where my error was and also gives a guide\n> of what to do instead.\n>\n>         $ git submodule deinit --all lib/\n>         error: paths with --all do not make sense\n>         usage: git submodule deinit [-f | --force] (--all | [--] <path>...)\n>\n> Thanks,\n> Jonathan\n>\n> -- >8 --\n> Subject: git-sh-setup: add error_with_usage helper\n>\n> When given an impossible set of options, this allows a command to\n> print a targetted message followed by a short usage string that tells\n> the user both (1) what part of their command wasn't supported (what\n> went wrong) and (2) what correct usage looks like (what to do\n> instead).\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for this patch, I can take that as a preparation for the patch\nunder discussion.\n\n> ---\n>  git-sh-setup.sh | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index c48139a..2b56636 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -63,6 +63,11 @@ say () {\n>         fi\n>  }\n>\n> +error_with_usage () {\n\nAs usage is the last thing (it's die(...) essentially),\nI'd rename it to die_with_usage ?\n\n> +       printf >&2 'error: %s\\n' \"$*\"\n> +       usage\n> +}\n> +\n>  if test -n \"$OPTIONS_SPEC\"; then\n>         usage() {\n>                 \"$0\" -h\n> --\n"}]}