{"thread":{"id":"42204","subject":"[PATCH 1/3] submodule deinit test: fix broken && chain in subshell","startedAt":"2016-05-02T22:24:01Z","lastAt":"2016-05-03T22:24:41Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"285218","messageId":"1462227844-10624-1-git-send-email-sbeller@google.com","threadId":"42204","inReplyTo":null,"subject":"[PATCH 0/3] preparatory patches for the submodule groups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T22:24:01Z","receivedAt":"2016-05-02T22:24:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"These patches build on top of origin/sb/submodule-init and are preparatoy for\nthe submodule groups series. Junio asked to send these preparatory patches\nas its own series to have a better mental focus when reviewing the groups\nseries later.\n\nWhen sending out a similar series end of last week[1], I went to far and included\nsome questionable patches in the series, this series is about the minimum needed.\n(patch 1 is a test suite correctness patch, which would not be strictly needed)\n\nThanks,\nStefan\n\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/293087\n\nStefan Beller (3):\n  submodule deinit test: fix broken && chain in subshell\n  submodule deinit: lose requirement for giving '.'\n  submodule init: redirect stdout to stderr\n\n builtin/submodule--helper.c |  3 ++-\n git-submodule.sh            |  5 -----\n t/t7400-submodule-basic.sh  |  7 +++++--\n t/t7406-submodule-update.sh | 24 ++++++++++++++++++------\n 4 files changed, 25 insertions(+), 14 deletions(-)\n\n-- \n2.8.0.37.gb114fff.dirty\n"},{"id":"285217","messageId":"1462227844-10624-2-git-send-email-sbeller@google.com","threadId":"42204","inReplyTo":"1462227844-10624-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/3] submodule deinit test: fix broken && chain in subshell","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T22:24:02Z","receivedAt":"2016-05-02T22:24:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7400-submodule-basic.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 814ee63..90d80d3 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n \t\tgit init &&\n \t\t>file &&\n \t\tgit add file &&\n-\t\tgit commit -m \"repo should not be empty\"\n+\t\tgit commit -m \"repo should not be empty\" &&\n \t\tgit submodule deinit .\n \t)\n '\n-- \n2.8.0.37.gb114fff.dirty\n"},{"id":"285220","messageId":"1462227844-10624-3-git-send-email-sbeller@google.com","threadId":"42204","inReplyTo":"1462227844-10624-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/3] submodule deinit: lose requirement for giving '.'","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T22:24:03Z","receivedAt":"2016-05-02T22:24:03Z","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\nAllow no argument for `submodule deinit` to mean 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\n[1] http://news.gmane.org/gmane.comp.version-control.git/289535\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh           | 5 -----\n t/t7400-submodule-basic.sh | 5 ++++-\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 82e95a9..d689265 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -428,11 +428,6 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tif test $# = 0\n-\tthen\n-\t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n-\tfi\n-\n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 90d80d3..9af47b5 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\n+'\n+\n test_expect_success 'setup - initial commit' '\n \t>t &&\n \tgit add t &&\n@@ -948,7 +952,6 @@ test_expect_success 'submodule deinit . 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 \ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\" &&\n \ttest -z \"$(git config --get-regexp \"submodule\\.example2\\.\")\" &&\n-- \n2.8.0.37.gb114fff.dirty\n"},{"id":"285219","messageId":"1462227844-10624-4-git-send-email-sbeller@google.com","threadId":"42204","inReplyTo":"1462227844-10624-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/3] submodule init: redirect stdout to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-02T22:24:04Z","receivedAt":"2016-05-02T22:24:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Reroute the output of stdout to stderr as it is just informative\nmessages, not to be consumed by machines.\n\nThis should not regress any scripts that try to parse the\ncurrent output, as the output is already internationalized\nand therefore unstable.\n\nWe want to init submodules from the helper for `submodule update`\nin a later patch and the stdout output of said helper is consumed\nby the parts of `submodule update` which are still written in shell.\nSo we have to be careful which messages are on stdout.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c |  3 ++-\n t/t7406-submodule-update.sh | 24 ++++++++++++++++++------\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5d05393..7f0941d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -366,7 +366,8 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t    displaypath);\n \t\tif (!quiet)\n-\t\t\tprintf(_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n \ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex fd741f5..5f27879 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -108,24 +108,36 @@ pwd=$(pwd)\n \n cat <<EOF >expect\n Submodule path '../super': checked out '$supersha1'\n-Submodule 'merging' ($pwd/merging) registered for path '../super/merging'\n-Submodule 'none' ($pwd/none) registered for path '../super/none'\n-Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'\n-Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'\n Submodule path '../super/merging': checked out '$mergingsha1'\n Submodule path '../super/none': checked out '$nonesha1'\n Submodule path '../super/rebasing': checked out '$rebasingsha1'\n Submodule path '../super/submodule': checked out '$submodulesha1'\n EOF\n \n+cat <<EOF >expect2\n+Submodule 'merging' ($pwd/merging) registered for path '../super/merging'\n+Submodule 'none' ($pwd/none) registered for path '../super/none'\n+Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'\n+Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'\n+Cloning into '$pwd/recursivesuper/super/merging'...\n+done.\n+Cloning into '$pwd/recursivesuper/super/none'...\n+done.\n+Cloning into '$pwd/recursivesuper/super/rebasing'...\n+done.\n+Cloning into '$pwd/recursivesuper/super/submodule'...\n+done.\n+EOF\n+\n test_expect_success 'submodule update --init --recursive from subdirectory' '\n \tgit -C recursivesuper/super reset --hard HEAD^ &&\n \t(cd recursivesuper &&\n \t mkdir tmp &&\n \t cd tmp &&\n-\t git submodule update --init --recursive ../super >../../actual\n+\t git submodule update --init --recursive ../super >../../actual 2>../../actual2\n \t) &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect2 actual2\n '\n \n apos=\"'\";\n-- \n2.8.0.37.gb114fff.dirty\n"},{"id":"285333","messageId":"xmqqvb2vnmh0.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"1462227844-10624-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/3] submodule deinit test: fix broken && chain in subshell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T16:19:07Z","receivedAt":"2016-05-03T16:19:07Z","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> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t7400-submodule-basic.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 814ee63..90d80d3 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n>  \t\tgit init &&\n>  \t\t>file &&\n>  \t\tgit add file &&\n> -\t\tgit commit -m \"repo should not be empty\"\n> +\t\tgit commit -m \"repo should not be empty\" &&\n>  \t\tgit submodule deinit .\n>  \t)\n>  '\n\nThanks.\n\nAs this was introduced by 84ba959b (submodule: fix regression for\ndeinit without submodules, 2016-03-22), which was merged to the\nmainline at v2.8.0-rc4-8-g2a4c8c3 and applies there, let's queue it\nthere.\n"},{"id":"285341","messageId":"xmqqoa8nnjld.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"1462227844-10624-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T17:21:18Z","receivedAt":"2016-05-03T17:21:18Z","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> The discussion in [1] realized that '.' is a faulty suggestion as\n> there 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>\n> Allow no argument for `submodule deinit` to mean 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\nOK, and the reason why there is no need to update the actual code,\nother than the \"do not allow\" gate, is because \"submodule--helper\nlist\" aka module_list already knows to list everything if no\npathspec is given.  Am I reading the code (not in the patch)\ncorrectly?\n\n> [1] http://news.gmane.org/gmane.comp.version-control.git/289535\n\nThe old discussion thread raises a good point.  The refusal to\naccept no-pathspec form for a potentially destructive \"deinit\" may\nhave been a safety measure, even though the suggested way to tell\nthe command \"Yes, I know I want to deinit everything\" was not a\ngood one (i.e. it resulted in an error if your project did not have\nany files tracked yet).\n\nSo possible ways forward may be\n\n - to remove the safety altogether; or\n - keep the safety, but give a better suggestion to say \"Yes, deinit\n   everything\".\n\nAnd this patch decides to take the former approach?\n\nI am wondering if this can be solved in a cleaner way to teach\n\"deinit\" take a new \"--all\" option instead, e.g. something like...\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 82e95a9..4b84116 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -405,6 +405,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@@ -414,6 +415,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@@ -428,9 +432,9 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tif test $# = 0\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\" \"$@\" |\n\n\nThat would work even in the pathological \"empty directory that has\nnothing to match even '.'\" case without losing the safety, no?\n"},{"id":"285351","messageId":"CAGZ79kaHmtRooJ7J6BknqWDkVFN0Bx3xngkEH0CnAp7Aaehnig@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqoa8nnjld.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T18:07:27Z","receivedAt":"2016-05-03T18:07:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> 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>>\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>> Allow no argument for `submodule deinit` to mean 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>\n> OK, and the reason why there is no need to update the actual code,\n> other than the \"do not allow\" gate, is because \"submodule--helper\n> list\" aka module_list already knows to list everything if no\n> pathspec is given.  Am I reading the code (not in the patch)\n> correctly?\n\nYou are correct.\n\n>\n>> [1] http://news.gmane.org/gmane.comp.version-control.git/289535\n>\n> The old discussion thread raises a good point.  The refusal to\n> accept no-pathspec form for a potentially destructive \"deinit\" may\n> have been a safety measure, even though the suggested way to tell\n> the command \"Yes, I know I want to deinit everything\" was not a\n> good one (i.e. it resulted in an error if your project did not have\n> any files tracked yet).\n>\n> So possible ways forward may be\n>\n>  - to remove the safety altogether; or\n>  - keep the safety, but give a better suggestion to say \"Yes, deinit\n>    everything\".\n>\n> And this patch decides to take the former approach?\n\nYes.\n\n>\n> I am wondering if this can be solved in a cleaner way to teach\n> \"deinit\" take a new \"--all\" option instead, e.g. something like...\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 82e95a9..4b84116 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -405,6 +405,7 @@ cmd_init()\n>  cmd_deinit()\n>  {\n>         # parse $args after \"submodule ... deinit\".\n> +       deinit_all=\n>         while test $# -ne 0\n>         do\n>                 case \"$1\" in\n> @@ -414,6 +415,9 @@ cmd_deinit()\n>                 -q|--quiet)\n>                         GIT_QUIET=1\n>                         ;;\n> +               -a|--all)\n> +                       deinit_all=t\n> +                       ;;\n>                 --)\n>                         shift\n>                         break\n> @@ -428,9 +432,9 @@ cmd_deinit()\n>                 shift\n>         done\n>\n> -       if test $# = 0\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>         git submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n>\n>\n> That would work even in the pathological \"empty directory that has\n> nothing to match even '.'\" case without losing the safety, no?\n\nIt would work for the case\n\n    git submodule deinit --all # as you would expect.\n\n    git submodule deinit --all COPYIN* # would break\n    git submodule deinit --all . # may break\n    git submodule deinit --all path/to/some/submodules/ # would be unclear to me\n\nSo maybe we want to add a check that no pathspec arguments are given when\n--all is given?\n"},{"id":"285361","messageId":"xmqqzis7m023.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"CAGZ79kaHmtRooJ7J6BknqWDkVFN0Bx3xngkEH0CnAp7Aaehnig@mail.gmail.com","subject":"Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T19:08:36Z","receivedAt":"2016-05-03T19:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, May 3, 2016 at 11:07 AM, Stefan Beller <sbeller@google.com> wrote:\n>\n> So maybe we want to add a check that no pathspec arguments are\n> given when\n> --all is given?\n\nYeah, I overlooked that case.  Just like \"commit -a path\" errors\nout, we should.\n"},{"id":"285363","messageId":"CAGZ79kb=jNKFPrnfYD5u64o6HBv5deh2QBpt-wSL4YxVSDNiuQ@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqzis7m023.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T19:22:12Z","receivedAt":"2016-05-03T19:22:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 12:08 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Tue, May 3, 2016 at 11:07 AM, Stefan Beller <sbeller@google.com> wrote:\n>>\n>> So maybe we want to add a check that no pathspec arguments are\n>> given when\n>> --all is given?\n>\n> Yeah, I overlooked that case.  Just like \"commit -a path\" errors\n> out, we should.\n\nThanks for the patch, though. I'll add the error-out in a resend of the series?\n"},{"id":"285366","messageId":"CAGZ79kZX-UkmTXG=s8qXe-RKynW3Oitk0URZk-K81UOSGfSeEQ@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqvb2vnmh0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] submodule deinit test: fix broken && chain in subshell","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T19:30:33Z","receivedAt":"2016-05-03T19:30:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 9:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  t/t7400-submodule-basic.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n>> index 814ee63..90d80d3 100755\n>> --- a/t/t7400-submodule-basic.sh\n>> +++ b/t/t7400-submodule-basic.sh\n>> @@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '\n>>               git init &&\n>>               >file &&\n>>               git add file &&\n>> -             git commit -m \"repo should not be empty\"\n>> +             git commit -m \"repo should not be empty\" &&\n>>               git submodule deinit .\n>>       )\n>>  '\n>\n> Thanks.\n>\n> As this was introduced by 84ba959b (submodule: fix regression for\n> deinit without submodules, 2016-03-22), which was merged to the\n> mainline at v2.8.0-rc4-8-g2a4c8c3 and applies there, let's queue it\n> there.\n\nOk, I'll reroll there with an improved 2/3 on top of v2.8.0-rc4-8-g2a4c8c3.\n"},{"id":"285373","messageId":"xmqqwpnalwf8.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"1462227844-10624-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T20:27:07Z","receivedAt":"2016-05-03T20:27:07Z","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> Stefan Beller (3):\n>   submodule deinit test: fix broken && chain in subshell\n>   submodule deinit: lose requirement for giving '.'\n>   submodule init: redirect stdout to stderr\n\nSo...\n\n * I'll take \"&&-chain\" patch on a separate topic on 84ba959, to be\n   later merged to 'master' and probably to 'maint'.\n\n * I'll queue the \"send diag message to stderr\" patch on top of\n   sb/submodule-init.\n\n * As to the second one, I prefer to hear opinions from others\n   before choosing the possible two approaches.  Perhaps losing the\n   \"safety\" is acceptable.  Otherwise, we could use the one I sent\n   but with a \"-a and pathspec are incompatible\" fix.  That can be\n   on its own separate topic.\n\nThanks.\n"},{"id":"285375","messageId":"CAGZ79kakayOhPkCK4hbRkj-h2Bt+PqD69EgHk-chbu4xCA8_pA@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqwpnalwf8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T20:53:30Z","receivedAt":"2016-05-03T20:53:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Stefan Beller (3):\n>>   submodule deinit test: fix broken && chain in subshell\n>>   submodule deinit: lose requirement for giving '.'\n>>   submodule init: redirect stdout to stderr\n>\n> So...\n>\n>  * I'll take \"&&-chain\" patch on a separate topic on 84ba959, to be\n>    later merged to 'master' and probably to 'maint'.\n>\n>  * I'll queue the \"send diag message to stderr\" patch on top of\n>    sb/submodule-init.\n>\n>  * As to the second one, I prefer to hear opinions from others\n>    before choosing the possible two approaches.  Perhaps losing the\n>    \"safety\" is acceptable.  Otherwise, we could use the one I sent\n>    but with a \"-a and pathspec are incompatible\" fix.  That can be\n>    on its own separate topic.\n\nI have your patch here and have a \"-a and pathspec are incompatible\" fix\nbuild on top.\n* I do wonder if we want to have the shortform '-a' though.\n* I think we want to head for consistency, eventually.\n   e.g. commands with no arguments such as tag, branch\n   give a list of their respective domain.\n\n   Subcommands do not give lists by default, e.g.\n   `git stash clear`, `git remote prune`\n   which are the moral equivalent to\n   `git submodule deinit` just work as they were told, no --switch needed.\n   However Jonathan suggests we may want to reserve the no arguments space\n   for future use and use the '--all' instead.\n"},{"id":"285376","messageId":"xmqqoa8mlutg.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"CAGZ79kakayOhPkCK4hbRkj-h2Bt+PqD69EgHk-chbu4xCA8_pA@mail.gmail.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T21:01:47Z","receivedAt":"2016-05-03T21:01:47Z","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 have your patch here and have a \"-a and pathspec are incompatible\" fix\n> build on top.\n> * I do wonder if we want to have the shortform '-a' though.\n\nI do not particularly care.  I was merely matching the other two\noptions there.\n\n> * I think we want to head for consistency, eventually.\n>    e.g. commands with no arguments such as tag, branch\n>    give a list of their respective domain.\n\nIsn't that a historical mistake we are regretting, though?  Only\nafter many other operation modes were invented and \"create X\" proves\nnot to be the only primary modes we had to invent \"tag -l\" and\n\"branch -l\".  Aren't we better off not having \"no option means list\"\nkind of default?\n\n>    Subcommands do not give lists by default, e.g.\n>    `git stash clear`, `git remote prune`\n>    which are the moral equivalent to\n>    `git submodule deinit` just work as they were told, no --switch needed.\n\nI wouldn't say \"git rm\" should remove everything by extending that\nlogic, but I can certainly buy if somebody argues \"git submodule\ndeinit\" is not destructive enough to warrant extra safety.\n"},{"id":"285377","messageId":"CAGZ79kbm5y+LeyA_dwQWKFNaa42280cOvwQYZTP=-aRnySsB1A@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqoa8mlutg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T21:12:47Z","receivedAt":"2016-05-03T21:12:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 2:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I have your patch here and have a \"-a and pathspec are incompatible\" fix\n>> build on top.\n>> * I do wonder if we want to have the shortform '-a' though.\n>\n> I do not particularly care.  I was merely matching the other two\n> options there.\n>\n>> * I think we want to head for consistency, eventually.\n>>    e.g. commands with no arguments such as tag, branch\n>>    give a list of their respective domain.\n>\n> Isn't that a historical mistake we are regretting, though?  Only\n> after many other operation modes were invented and \"create X\" proves\n> not to be the only primary modes we had to invent \"tag -l\" and\n> \"branch -l\".  Aren't we better off not having \"no option means list\"\n> kind of default?\n\nlisting is not destructive, and I really like to not type a single dash\nfor some commands.\n\n>\n>>    Subcommands do not give lists by default, e.g.\n>>    `git stash clear`, `git remote prune`\n>>    which are the moral equivalent to\n>>    `git submodule deinit` just work as they were told, no --switch needed.\n>\n> I wouldn't say \"git rm\" should remove everything by extending that\n> logic, but I can certainly buy if somebody argues \"git submodule\n> deinit\" is not destructive enough to warrant extra safety.\n\n`git rm` is a command, not a subcommand though.\n"},{"id":"285382","messageId":"xmqqfutylte1.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"CAGZ79kbm5y+LeyA_dwQWKFNaa42280cOvwQYZTP=-aRnySsB1A@mail.gmail.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T21:32:38Z","receivedAt":"2016-05-03T21:32:38Z","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 think we want to head for consistency, eventually.\n>>>    e.g. commands with no arguments such as tag, branch\n>>>    give a list of their respective domain.\n>>\n>> Isn't that a historical mistake we are regretting, though?  Only\n>> after many other operation modes were invented and \"create X\" proves\n>> not to be the only primary modes we had to invent \"tag -l\" and\n>> \"branch -l\".  Aren't we better off not having \"no option means list\"\n>> kind of default?\n>\n> listing is not destructive, and I really like to not type a single dash\n> for some commands.\n\nActually, listing is destructive to the user's cognitive state.\n\nI wouldn't be surprised if many people wished that \"git branch\" did\nnot list (and required \"git branch -l\" to list) to scroll everything\nthey are looking away but instead showed what the current branch is.\n\n>>>    Subcommands do not give lists by default, e.g.\n>>>    `git stash clear`, `git remote prune`\n>>>    which are the moral equivalent to\n>>>    `git submodule deinit` just work as they were told, no --switch needed.\n>>\n>> I wouldn't say \"git rm\" should remove everything by extending that\n>> logic, but I can certainly buy if somebody argues \"git submodule\n>> deinit\" is not destructive enough to warrant extra safety.\n>\n> `git rm` is a command, not a subcommand though.\n\nYes, it is a command, just like branch and tag you brought up.\n\n\"git branch -d\" is not a command, but a mode of operation.  If we\ndid the \"mode word\" UI [*1*], it would have been a subcommand.  And\nI certainly would not say it should remove everything if there is no\nargument on the command line.\n\nI am not sure where you are going with that though anyway.\n\nI think the \"safety\" is about forcing user to be more explicit when\ntriggering mass destruction, and I do not think it matters if that\ndestruction is done by a first-class subcommand of \"git\", or\nsubsubcommand.\n\n\n[References]\n\n*1* http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478\n"},{"id":"285385","messageId":"CAGZ79kZK_EtwBps3hcYVkRat2XbpXXXdXkQDrB281rxkedjaGA@mail.gmail.com","threadId":"42204","inReplyTo":"xmqqfutylte1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-03T21:57:00Z","receivedAt":"2016-05-03T21:57:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 3, 2016 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>>> * I think we want to head for consistency, eventually.\n>>>>    e.g. commands with no arguments such as tag, branch\n>>>>    give a list of their respective domain.\n>>>\n>>> Isn't that a historical mistake we are regretting, though?  Only\n>>> after many other operation modes were invented and \"create X\" proves\n>>> not to be the only primary modes we had to invent \"tag -l\" and\n>>> \"branch -l\".  Aren't we better off not having \"no option means list\"\n>>> kind of default?\n>>\n>> listing is not destructive, and I really like to not type a single dash\n>> for some commands.\n>\n> Actually, listing is destructive to the user's cognitive state.\n\nOh! I see.\n\n>\n> I wouldn't be surprised if many people wished that \"git branch\" did\n> not list (and required \"git branch -l\" to list) to scroll everything\n> they are looking away but instead showed what the current branch is.\n\nIsn't that yet another more specialized mode of operation?\n\nThe difference being that showing the current branch is less lines of output\nthan showing all branches. (listing the branches could also be done similar\nas `ls` lists files instead of `ls -1`)\n\n>\n>>>>    Subcommands do not give lists by default, e.g.\n>>>>    `git stash clear`, `git remote prune`\n>>>>    which are the moral equivalent to\n>>>>    `git submodule deinit` just work as they were told, no --switch needed.\n>>>\n>>> I wouldn't say \"git rm\" should remove everything by extending that\n>>> logic, but I can certainly buy if somebody argues \"git submodule\n>>> deinit\" is not destructive enough to warrant extra safety.\n>>\n>> `git rm` is a command, not a subcommand though.\n>\n> Yes, it is a command, just like branch and tag you brought up.\n>\n> \"git branch -d\" is not a command, but a mode of operation.  If we\n> did the \"mode word\" UI [*1*], it would have been a subcommand.  And\n> I certainly would not say it should remove everything if there is no\n> argument on the command line.\n\nRight.\n\nAnother point of confusion is that the `submodule deinit` case\nexpects a path spec unlike all the other examples and path specs\nhave the notion for specifying \"all of them\" in different contexts, i.e.\n'*' or '.' are valid \"all path specs\" in other programs.\n\nWe do not have a strong history for such \"all of them\" specifiers for\nbranches. (Well we could do a git branch -d refs/heads/* but these\nare files actually, so we'd think of * as a natural character to do so)\n\nThere are no notions for \"all of the stashes\" (i.e.  `git stash clear`\nwould be weird if it expected a '*'.\n\nI think '--all' is the right thing to do here then.\n\n>\n> I am not sure where you are going with that though anyway.\n\nI am trying to asses the users expectations on when you expect\na \"safety\" feature and when to expect the operation to perform.\n\n>\n> I think the \"safety\" is about forcing user to be more explicit when\n> triggering mass destruction, and I do not think it matters if that\n> destruction is done by a first-class subcommand of \"git\", or\n> subsubcommand.\n\nOk. I thought it mattered a bit as a subsubcommand is already more\nexplicit than a command. Compare `git stash clear` to the\nfictional `git clear` command. I would expect a `git clear`\nto not delete critical things without arguments. Maybe some minor things\nor internal things like garbage collection.\n\n`clear` sounding similar to `clean`, we could have a \"submodule.requireForce\"\nsimilar to clean.requireForce which would change the behavior of\nsubmodule deinit and defaults to the safe version. (I do not know the\nhistory of clean.requireForce but that sounds like it is a safety measure\nadded once people complained about having no safety default.)\n\nBut this discussion strengthens my opinion that we should just have a switch\nfor deinit.\n\nThanks,\nStefan\n\n>\n>\n> [References]\n>\n> *1* http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478\n"},{"id":"285389","messageId":"xmqq7ffalqza.fsf@gitster.mtv.corp.google.com","threadId":"42204","inReplyTo":"CAGZ79kZK_EtwBps3hcYVkRat2XbpXXXdXkQDrB281rxkedjaGA@mail.gmail.com","subject":"Re: [PATCH 0/3] preparatory patches for the submodule groups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T22:24:41Z","receivedAt":"2016-05-03T22:24:41Z","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 wouldn't be surprised if many people wished that \"git branch\" did\n>> not list (and required \"git branch -l\" to list) to scroll everything\n>> they are looking away but instead showed what the current branch is.\n>\n> Isn't that yet another more specialized mode of operation?\n\nYes.  It all boils down to \"what is the most common thing people\nwould want\" and \"which one of these operation modes deserve the\nshort-hand\".\n\nMost often, the first one that gets invented ends up squatting on\nthe short-and-sweet \"by default without parameter, we do this\" slot,\nand we have to let it squat forever due to backward compatibility.\n\nI think \"git branch\" that shows all 300 branches is a good example\nof that--if we didn't have command line prompt support, I am\nreasonably sure that \"git branch --current-branch\" would have been\nadded by popular request long time ago, and that would be a more\ncommon thing people would want than \"git branch -l\".  I would not be\nsurprised if in an alternative universe in which both \"list\" and\n\"tell me current\" were available back when the \"no option default\"\nhad not been defined yet, we gave the shortest-and-sweetest to \"git\nbranch ---current-branch\".\n"}]}