{"thread":{"id":"34825","subject":"[PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all","startedAt":"2013-09-01T20:06:47Z","lastAt":"2013-09-06T00:19:46Z","messageCount":13,"participants":["brian m. carlson","Jens Lehmann","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"226488","messageId":"1378066009-1017855-1-git-send-email-sandals@crustytoothpaste.net","threadId":"34825","inReplyTo":null,"subject":"[PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-09-01T20:06:47Z","receivedAt":"2013-09-01T20:06:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"There are configuration options for each submodule that specify under\nwhat circumstances git status should display output for that submodule.\nUnfortunately, these settings were not being respected, and as such the\ntests were marked TODO.\n\nThis patch series consists of two patches: the first is a fix for a\nconfusing variable name, and the latter actually makes git status not\noutput the submodule information.  The tests have been updated\naccordingly, and additional tests have been included to ensure that git\nsubmodule summary is not affected by these changes unless the\n--for-status option is given.\n\nChanges from v2:\n\n* Add tests to ensure that git submodule summary is not affected.\n* Fix bug that caused git submodule summary to be affected.\n\nChanges from v1:\n\n* Handle moved submodules by not ignoring them.\n* Use sm_path instead of path.\n* Only ignore when --for-status is given.\n\nbrian m. carlson (2):\n  submodule: fix confusing variable name\n  submodule: don't print status output with ignore=all\n\n git-submodule.sh             | 15 +++++++++++----\n t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++\n t/t7508-status.sh            |  4 ++--\n 3 files changed, 43 insertions(+), 6 deletions(-)\n\n-- \n1.8.4.rc3\n"},{"id":"226489","messageId":"1378066009-1017855-2-git-send-email-sandals@crustytoothpaste.net","threadId":"34825","inReplyTo":"1378066009-1017855-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 1/2] submodule: fix confusing variable name","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-09-01T20:06:48Z","receivedAt":"2013-09-01T20:06:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"cmd_summary reads the output of git diff, but reads in the submodule path into a\nvariable called name.  Since this variable does not contain the name of the\nsubmodule, but the path, rename it to be clearer what data it actually holds.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n git-submodule.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2979197..38520db 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1032,13 +1032,13 @@ cmd_summary() {\n \t# Get modified modules cared by user\n \tmodules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- \"$@\" |\n \t\tsane_egrep '^:([0-7]* )?160000' |\n-\t\twhile read mod_src mod_dst sha1_src sha1_dst status name\n+\t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n \t\tdo\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n-\t\t\ttest $status = D -o $status = T && echo \"$name\" && continue\n+\t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n \t\t\t# Also show added or modified modules which are checked out\n-\t\t\tGIT_DIR=\"$name/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n-\t\t\techo \"$name\"\n+\t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n+\t\t\techo \"$sm_path\"\n \t\tdone\n \t)\n \n-- \n1.8.4.rc3\n"},{"id":"226490","messageId":"1378066009-1017855-3-git-send-email-sandals@crustytoothpaste.net","threadId":"34825","inReplyTo":"1378066009-1017855-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-09-01T20:06:49Z","receivedAt":"2013-09-01T20:06:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"git status prints information for submodules, but it should ignore the status of\nthose which have submodule.<name>.ignore set to all.  Fix it so that it does\nproperly ignore those which have that setting either in .git/config or in\n.gitmodules.\n\nNot ignored are submodules that are added, deleted, or moved (which is\nessentially a combination of the first two) because it is not easily possible to\ndetermine the old path once a move has occurred, nor is it easily possible to\ndetect which adds and deletions are moves and which are not.  This also\npreserves the previous behavior of always listing modules which are to be\ndeleted.\n\nTests are included which verify that this change has no effect on git submodule\nsummary without the --for-status option.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n git-submodule.sh             |  7 +++++++\n t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++\n t/t7508-status.sh            |  4 ++--\n 3 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 38520db..004b21c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1036,6 +1036,13 @@ cmd_summary() {\n \t\tdo\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n \t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n+\t\t\t# Respect the ignore setting for --for-status.\n+\t\t\tif test -n \"$for_status\"\n+\t\t\tthen\n+\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n+\t\t\t\ttest $status != A -a $ignore_config = all && continue\n+\t\t\tfi\n \t\t\t# Also show added or modified modules which are checked out\n \t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n \t\t\techo \"$sm_path\"\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex ac2434c..ca9441e 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -104,6 +104,36 @@ EOF\n \ttest_cmp expected actual\n \"\n \n+test_expect_success '.gitmodules ignore=all has no effect' \"\n+\tgit config --add -f .gitmodules submodule.sm1.ignore all &&\n+\tgit config --add -f .gitmodules submodule.sm1.path sm1 &&\n+\tgit submodule summary >actual &&\n+\tcat >expected <<-EOF &&\n+* sm1 $head1...$head2 (1):\n+  > Add foo3\n+\n+EOF\n+\ttest_cmp expected actual &&\n+\tgit config -f .gitmodules --remove-section submodule.sm1\n+\"\n+\n+test_expect_success '.git/config ignore=all has no effect' \"\n+\tgit config --add -f .gitmodules submodule.sm1.ignore none &&\n+\tgit config --add -f .gitmodules submodule.sm1.path sm1 &&\n+\tgit config --add submodule.sm1.ignore all &&\n+\tgit config --add submodule.sm1.path sm1 &&\n+\tgit submodule summary >actual &&\n+\tcat >expected <<-EOF &&\n+* sm1 $head1...$head2 (1):\n+  > Add foo3\n+\n+EOF\n+\ttest_cmp expected actual &&\n+\tgit config --remove-section submodule.sm1 &&\n+\tgit config -f .gitmodules --remove-section submodule.sm1\n+\"\n+\n+\n commit_file sm1 &&\n head3=$(\n \tcd sm1 &&\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex ac3d0fe..fb89fb9 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1316,7 +1316,7 @@ test_expect_success \"--ignore-submodules=all suppresses submodule summary\" '\n \ttest_i18ncmp expect output\n '\n \n-test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '\n+test_expect_success '.gitmodules ignore=all suppresses submodule summary' '\n \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n \tgit config --add -f .gitmodules submodule.subname.path sm &&\n \tgit status > output &&\n@@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '\n \tgit config -f .gitmodules  --remove-section submodule.subname\n '\n \n-test_expect_failure '.git/config ignore=all suppresses submodule summary' '\n+test_expect_success '.git/config ignore=all suppresses submodule summary' '\n \tgit config --add -f .gitmodules submodule.subname.ignore none &&\n \tgit config --add -f .gitmodules submodule.subname.path sm &&\n \tgit config --add submodule.subname.ignore all &&\n-- \n1.8.4.rc3\n"},{"id":"226656","messageId":"522635B3.5050306@web.de","threadId":"34825","inReplyTo":"1378066009-1017855-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-03T19:17:07Z","receivedAt":"2013-09-03T19:17:07Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 01.09.2013 22:06, schrieb brian m. carlson:\n> git status prints information for submodules, but it should ignore the status of\n> those which have submodule.<name>.ignore set to all.  Fix it so that it does\n> properly ignore those which have that setting either in .git/config or in\n> .gitmodules.\n> \n> Not ignored are submodules that are added, deleted, or moved (which is\n> essentially a combination of the first two) because it is not easily possible to\n> determine the old path once a move has occurred, nor is it easily possible to\n> detect which adds and deletions are moves and which are not.  This also\n> preserves the previous behavior of always listing modules which are to be\n> deleted.\n> \n> Tests are included which verify that this change has no effect on git submodule\n> summary without the --for-status option.\n\nThanks, that fixes the bug Matthieu noticed which is now tested for.\n\nI only have some nits concerning the new tests, please see below.\n\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n>  git-submodule.sh             |  7 +++++++\n>  t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++\n>  t/t7508-status.sh            |  4 ++--\n>  3 files changed, 39 insertions(+), 2 deletions(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 38520db..004b21c 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -1036,6 +1036,13 @@ cmd_summary() {\n>  \t\tdo\n>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n>  \t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n> +\t\t\t# Respect the ignore setting for --for-status.\n> +\t\t\tif test -n \"$for_status\"\n> +\t\t\tthen\n> +\t\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n> +\t\t\t\ttest $status != A -a $ignore_config = all && continue\n> +\t\t\tfi\n>  \t\t\t# Also show added or modified modules which are checked out\n>  \t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n>  \t\t\techo \"$sm_path\"\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index ac2434c..ca9441e 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -104,6 +104,36 @@ EOF\n>  \ttest_cmp expected actual\n>  \"\n>  \n> +test_expect_success '.gitmodules ignore=all has no effect' \"\n> +\tgit config --add -f .gitmodules submodule.sm1.ignore all &&\n> +\tgit config --add -f .gitmodules submodule.sm1.path sm1 &&\n> +\tgit submodule summary >actual &&\n> +\tcat >expected <<-EOF &&\n> +* sm1 $head1...$head2 (1):\n> +  > Add foo3\n> +\n> +EOF\n> +\ttest_cmp expected actual &&\n> +\tgit config -f .gitmodules --remove-section submodule.sm1\n> +\"\n> +\n> +test_expect_success '.git/config ignore=all has no effect' \"\n> +\tgit config --add -f .gitmodules submodule.sm1.ignore none &&\n> +\tgit config --add -f .gitmodules submodule.sm1.path sm1 &&\n> +\tgit config --add submodule.sm1.ignore all &&\n> +\tgit config --add submodule.sm1.path sm1 &&\n> +\tgit submodule summary >actual &&\n> +\tcat >expected <<-EOF &&\n> +* sm1 $head1...$head2 (1):\n> +  > Add foo3\n> +\n> +EOF\n> +\ttest_cmp expected actual &&\n> +\tgit config --remove-section submodule.sm1 &&\n> +\tgit config -f .gitmodules --remove-section submodule.sm1\n> +\"\n> +\n> +\n>  commit_file sm1 &&\n>  head3=$(\n>  \tcd sm1 &&\n\nI think we should do both tests in one go: just turn on all ignore\noptions and test against that. Additionally we should also set\ndiff.ignoreSubmodules too, while there is no need to set the path\nin .git/config. And I believe we can drop the --add option from\nthe config command, so I'd propose something like this:\n\n-------------------------8<---------------------------\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex ac2434c..81ae7c9 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -104,6 +104,24 @@ EOF\n \ttest_cmp expected actual\n \"\n\n+test_expect_success 'no ignore=all setting has any effect' \"\n+\tgit config -f .gitmodules submodule.sm1.path sm1 &&\n+\tgit config -f .gitmodules submodule.sm1.ignore all &&\n+\tgit config submodule.sm1.ignore all &&\n+\tgit config diff.ignoreSubmodules all &&\n+\tgit submodule summary >actual &&\n+\tcat >expected <<-EOF &&\n+* sm1 $head1...$head2 (1):\n+  > Add foo3\n+\n+EOF\n+\ttest_cmp expected actual &&\n+\tgit config --unset diff.ignoreSubmodules &&\n+\tgit config --remove-section submodule.sm1 &&\n+\tgit config -f .gitmodules --remove-section submodule.sm1\n+\"\n+\n+\n commit_file sm1 &&\n head3=$(\n \tcd sm1 &&\n-------------------------8<---------------------------\n\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index ac3d0fe..fb89fb9 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -1316,7 +1316,7 @@ test_expect_success \"--ignore-submodules=all suppresses submodule summary\" '\n>  \ttest_i18ncmp expect output\n>  '\n>  \n> -test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '\n> +test_expect_success '.gitmodules ignore=all suppresses submodule summary' '\n>  \tgit config --add -f .gitmodules submodule.subname.ignore all &&\n>  \tgit config --add -f .gitmodules submodule.subname.path sm &&\n>  \tgit status > output &&\n> @@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '\n>  \tgit config -f .gitmodules  --remove-section submodule.subname\n>  '\n>  \n> -test_expect_failure '.git/config ignore=all suppresses submodule summary' '\n> +test_expect_success '.git/config ignore=all suppresses submodule summary' '\n>  \tgit config --add -f .gitmodules submodule.subname.ignore none &&\n>  \tgit config --add -f .gitmodules submodule.subname.path sm &&\n>  \tgit config --add submodule.subname.ignore all &&\n> \n"},{"id":"226663","messageId":"xmqqob89r8bc.fsf@gitster.dls.corp.google.com","threadId":"34825","inReplyTo":"1378066009-1017855-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T19:53:11Z","receivedAt":"2013-09-03T19:53:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens, I see 1/2 is the same as the previous one you already acked.\nDoes this update to 2/2 look good to you?  Sorry, but I lost track\nof the discussion that led to this reroll, hence a ping.\n\nThanks.\n"},{"id":"226701","messageId":"vpqa9jtayiq.fsf@anie.imag.fr","threadId":"34825","inReplyTo":"1378066009-1017855-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-04T06:31:25Z","receivedAt":"2013-09-04T06:31:25Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> Tests are included which verify that this change has no effect on git submodule\n> summary without the --for-status option.\n\nI still don't understand why this is needed. Why do we want \"git status\"\nand \"git submodule summary\" to display different information? Wasn't it\na nice property that the part of \"git status\" about submodule is the\nsame as \"git submodule summary\"?\n\nThis should at least be explained in the commit message IMHO.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226761","messageId":"522791AE.8070103@web.de","threadId":"34825","inReplyTo":"xmqqob89r8bc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-04T20:01:50Z","receivedAt":"2013-09-04T20:01:50Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 03.09.2013 21:53, schrieb Junio C Hamano:\n> Jens, I see 1/2 is the same as the previous one you already acked.\n\nYep.\n\n> Does this update to 2/2 look good to you?  Sorry, but I lost track\n> of the discussion that led to this reroll, hence a ping.\n\nv3 fixes the bug Matthieu noticed, I only had some remarks to the\nnew test Brian added. If you could replace his patch to t7401 with\nthe following diff it's an ack from me on this one too.\n\n-------------------------8<---------------------------\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex ac2434c..81ae7c9 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -104,6 +104,24 @@ EOF\n \ttest_cmp expected actual\n \"\n\n+test_expect_success 'no ignore=all setting has any effect' \"\n+\tgit config -f .gitmodules submodule.sm1.path sm1 &&\n+\tgit config -f .gitmodules submodule.sm1.ignore all &&\n+\tgit config submodule.sm1.ignore all &&\n+\tgit config diff.ignoreSubmodules all &&\n+\tgit submodule summary >actual &&\n+\tcat >expected <<-EOF &&\n+* sm1 $head1...$head2 (1):\n+  > Add foo3\n+\n+EOF\n+\ttest_cmp expected actual &&\n+\tgit config --unset diff.ignoreSubmodules &&\n+\tgit config --remove-section submodule.sm1 &&\n+\tgit config -f .gitmodules --remove-section submodule.sm1\n+\"\n+\n+\n commit_file sm1 &&\n head3=$(\n \tcd sm1 &&\n"},{"id":"226763","messageId":"52279ACC.2070308@web.de","threadId":"34825","inReplyTo":"vpqa9jtayiq.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-04T20:40:44Z","receivedAt":"2013-09-04T20:40:44Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 04.09.2013 08:31, schrieb Matthieu Moy:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n>> Tests are included which verify that this change has no effect on git submodule\n>> summary without the --for-status option.\n> \n> I still don't understand why this is needed.\n\nTo avoid a change in behavior for \"git submodule summary\", as that\nnever honored the submodule.*.ignore nor the diff.ignoreSubmodules\nsetting (and I don't think it ever should).\n\n> Why do we want \"git status\"\n> and \"git submodule summary\" to display different information? Wasn't it\n> a nice property that the part of \"git status\" about submodule is the\n> same as \"git submodule summary\"?\n\nI changed that in 2010 (1.7.3) to make the output of status consistent,\nmeaning that submodules that didn't show up in the regular status output\ndon't appear in the summary part either. And I still believe it is the\nright thing to do for the target audience of the ignore settings, as\nthey want to hide any changes in submodules that are either expensive to\ntraverse or uninteresting for the developer until they explicitly use\nthe submodule commands, which still behave as they always did (I might\nbe wrong here as I'm not in that group myself, but so far no one spoke\nup).\n\n> This should at least be explained in the commit message IMHO.\n\nFine by me, what would you propose to clarify that? (Though I have the\nsuspicion that the explanation will be three years late ;-)\n"},{"id":"226769","messageId":"xmqqli3cl2yk.fsf@gitster.dls.corp.google.com","threadId":"34825","inReplyTo":"522791AE.8070103@web.de","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-04T20:57:39Z","receivedAt":"2013-09-04T20:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Am 03.09.2013 21:53, schrieb Junio C Hamano:\n>> Jens, I see 1/2 is the same as the previous one you already acked.\n>\n> Yep.\n>\n>> Does this update to 2/2 look good to you?  Sorry, but I lost track\n>> of the discussion that led to this reroll, hence a ping.\n>\n> v3 fixes the bug Matthieu noticed, I only had some remarks to the\n> new test Brian added. If you could replace his patch to t7401 with\n> the following diff it's an ack from me on this one too.\n>\n> -------------------------8<---------------------------\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index ac2434c..81ae7c9 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -104,6 +104,24 @@ EOF\n>  \ttest_cmp expected actual\n>  \"\n>\n> +test_expect_success 'no ignore=all setting has any effect' \"\n> +\tgit config -f .gitmodules submodule.sm1.path sm1 &&\n> +\tgit config -f .gitmodules submodule.sm1.ignore all &&\n> +\tgit config submodule.sm1.ignore all &&\n> +\tgit config diff.ignoreSubmodules all &&\n> +\tgit submodule summary >actual &&\n> +\tcat >expected <<-EOF &&\n> +* sm1 $head1...$head2 (1):\n> +  > Add foo3\n> +\n> +EOF\n> +\ttest_cmp expected actual &&\n> +\tgit config --unset diff.ignoreSubmodules &&\n> +\tgit config --remove-section submodule.sm1 &&\n> +\tgit config -f .gitmodules --remove-section submodule.sm1\n> +\"\n> +\n> +\n>  commit_file sm1 &&\n>  head3=$(\n>  \tcd sm1 &&\n\nThanks.\n\nThe above patch makes the <<-EOF situation that already exists in\nthis script worse. The only reason we would say -EOF not EOF is\nbecause we would want to indent the here-document to align with the\nrest of the command sequence, so we should either indent with HT, or\ndrop the dash.  I suspect the original did it that way fearing that\nsomeday the indentation of the submodule difference list might start\nusing HT, but I do not think that is likely to happen, so my vote\ngoes to keeping '-' and indenting.\n\nWe need a clean-up patch after this series settles.\n"},{"id":"226770","messageId":"5227A570.7040500@web.de","threadId":"34825","inReplyTo":"xmqqli3cl2yk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-04T21:26:08Z","receivedAt":"2013-09-04T21:26:08Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 04.09.2013 22:57, schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> Am 03.09.2013 21:53, schrieb Junio C Hamano:\n>>> Does this update to 2/2 look good to you?  Sorry, but I lost track\n>>> of the discussion that led to this reroll, hence a ping.\n>>\n>> v3 fixes the bug Matthieu noticed, I only had some remarks to the\n>> new test Brian added. If you could replace his patch to t7401 with\n>> the following diff it's an ack from me on this one too.\n>>\n>> -------------------------8<---------------------------\n>> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n>> index ac2434c..81ae7c9 100755\n>> --- a/t/t7401-submodule-summary.sh\n>> +++ b/t/t7401-submodule-summary.sh\n>> @@ -104,6 +104,24 @@ EOF\n>>  \ttest_cmp expected actual\n>>  \"\n>>\n>> +test_expect_success 'no ignore=all setting has any effect' \"\n>> +\tgit config -f .gitmodules submodule.sm1.path sm1 &&\n>> +\tgit config -f .gitmodules submodule.sm1.ignore all &&\n>> +\tgit config submodule.sm1.ignore all &&\n>> +\tgit config diff.ignoreSubmodules all &&\n>> +\tgit submodule summary >actual &&\n>> +\tcat >expected <<-EOF &&\n>> +* sm1 $head1...$head2 (1):\n>> +  > Add foo3\n>> +\n>> +EOF\n>> +\ttest_cmp expected actual &&\n>> +\tgit config --unset diff.ignoreSubmodules &&\n>> +\tgit config --remove-section submodule.sm1 &&\n>> +\tgit config -f .gitmodules --remove-section submodule.sm1\n>> +\"\n>> +\n>> +\n>>  commit_file sm1 &&\n>>  head3=$(\n>>  \tcd sm1 &&\n> \n> Thanks.\n> \n> The above patch makes the <<-EOF situation that already exists in\n> this script worse. The only reason we would say -EOF not EOF is\n> because we would want to indent the here-document to align with the\n> rest of the command sequence, so we should either indent with HT, or\n> drop the dash.  I suspect the original did it that way fearing that\n> someday the indentation of the submodule difference list might start\n> using HT, but I do not think that is likely to happen, so my vote\n> goes to keeping '-' and indenting.\n> \n> We need a clean-up patch after this series settles.\n\nOk, will do (unless someone else volunteers ;-).\n"},{"id":"226826","messageId":"vpqioyf231e.fsf@anie.imag.fr","threadId":"34825","inReplyTo":"52279ACC.2070308@web.de","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-05T06:30:53Z","receivedAt":"2013-09-05T06:30:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Am 04.09.2013 08:31, schrieb Matthieu Moy:\n>> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>> \n>>> Tests are included which verify that this change has no effect on git submodule\n>>> summary without the --for-status option.\n>> \n>> I still don't understand why this is needed.\n>\n> To avoid a change in behavior for \"git submodule summary\", as that\n> never honored the submodule.*.ignore nor the diff.ignoreSubmodules\n> setting (and I don't think it ever should).\n\nI don't get it. If the goal is to keep the old behavior, then \"git\nstatus\" shouldn't be changed either. Fixing bugs needs to change the\nbehavior.\n\nIOW, why was it a bug that \"git status\" showed ignored submodules and\nnot a bug that \"git submodule summary\" did the same?\n\n> Fine by me, what would you propose to clarify that? (Though I have the\n> suspicion that the explanation will be three years late ;-)\n\nI have no idea, as I do not understand the reason myself yet. I'm not a\nfrequent user of submodules and not a user of the ignore option at all,\nso I can't tell what's best. I'd just like the new behavior to be\njustified somewhere.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226831","messageId":"vpqob87zoa2.fsf@anie.imag.fr","threadId":"34825","inReplyTo":"vpqioyf231e.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-05T08:05:41Z","receivedAt":"2013-09-05T08:05:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>\n>> Fine by me, what would you propose to clarify that? (Though I have the\n>> suspicion that the explanation will be three years late ;-)\n>\n> I have no idea, as I do not understand the reason myself yet. I'm not a\n> frequent user of submodules and not a user of the ignore option at all,\n> so I can't tell what's best. I'd just like the new behavior to be\n> justified somewhere.\n\nI also think that the documentation in Documentation/config.txt could be\nupdated. Perhaps adding something like\n\n\tTo view the summary for submodules ignored by 'git status', one\n\tcan use 'git submodules summary' which shows a similar output\n\tbut does not honor this option.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226898","messageId":"20130906001946.GC5359@vauxhall.crustytoothpaste.net","threadId":"34825","inReplyTo":"vpqioyf231e.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-09-06T00:19:46Z","receivedAt":"2013-09-06T00:19:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Sep 05, 2013 at 08:30:53AM +0200, Matthieu Moy wrote:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n> > Am 04.09.2013 08:31, schrieb Matthieu Moy:\n> >> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> >> \n> >>> Tests are included which verify that this change has no effect on git submodule\n> >>> summary without the --for-status option.\n> >> \n> >> I still don't understand why this is needed.\n> >\n> > To avoid a change in behavior for \"git submodule summary\", as that\n> > never honored the submodule.*.ignore nor the diff.ignoreSubmodules\n> > setting (and I don't think it ever should).\n> \n> I don't get it. If the goal is to keep the old behavior, then \"git\n> status\" shouldn't be changed either. Fixing bugs needs to change the\n> behavior.\n> \n> IOW, why was it a bug that \"git status\" showed ignored submodules and\n> not a bug that \"git submodule summary\" did the same?\n\nI looked at the tests, and the tests expected git status to respect the\nignore settings, but there were no tests that expected git submodule\nsummary to work this way.  Perhaps this is a failure in the tests.  I\ngot feedback that git submodule summary should *not* change, and nothing\nuntil now to contradict that.\n\nIf the decision is that this should change both, then fine, I'll reroll.\nBut it's not reasonable to keep going back and forth: we should make a\ndecision and stick to it.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"}]}