{"thread":{"id":"34607","subject":"[PATCH 1/2] submodule: fix confusing variable name","startedAt":"2013-08-03T17:14:18Z","lastAt":"2013-08-17T16:27:25Z","messageCount":17,"participants":["brian m. carlson","Jonathan Nieder","Jens Lehmann","Fredrik Gustafsson","Ramsay Jones","Junio C Hamano","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"224536","messageId":"1375550060-5406-1-git-send-email-sandals@crustytoothpaste.net","threadId":"34607","inReplyTo":null,"subject":"Don't print status output with submodule.<name>.ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-03T17:14:18Z","receivedAt":"2013-08-03T17:14:18Z","isPatch":false,"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 what\ncircumstances git status should display output for that submodule.\nUnfortunately, these settings were not being respected, and as such the tests\nwere marked TODO.\n\nThis patch series consists of two patches: the first is a fix for a confusing\nvariable name, and the latter actually makes git status not output the submodule\ninformation.  The tests have been updated accordingly.\n\n git-submodule.sh  | 10 ++++++----\n t/t7508-status.sh |  4 ++--\n 2 files changed, 8 insertions(+), 6 deletions(-)\n"},{"id":"224535","messageId":"1375550060-5406-2-git-send-email-sandals@crustytoothpaste.net","threadId":"34607","inReplyTo":"1375550060-5406-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH 1/2] submodule: fix confusing variable name","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-03T17:14:19Z","receivedAt":"2013-08-03T17:14:19Z","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..30b7fc1 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 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 \"$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=\"$path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n+\t\t\techo \"$path\"\n \t\tdone\n \t)\n \n-- \n1.8.4.rc1\n"},{"id":"224537","messageId":"1375550060-5406-3-git-send-email-sandals@crustytoothpaste.net","threadId":"34607","inReplyTo":"1375550060-5406-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-03T17:14:20Z","receivedAt":"2013-08-03T17:14:20Z","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\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n git-submodule.sh  | 2 ++\n t/t7508-status.sh | 4 ++--\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 30b7fc1..5694ae6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1034,6 +1034,8 @@ cmd_summary() {\n \t\tsane_egrep '^:([0-7]* )?160000' |\n \t\twhile read mod_src mod_dst sha1_src sha1_dst status path\n \t\tdo\n+\t\t\tname=$(module_name \"$path\")\n+\t\t\ttest $(get_submodule_config \"$name\" ignore none) = all && continue\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n \t\t\ttest $status = D -o $status = T && echo \"$path\" && continue\n \t\t\t# Also show added or modified modules which are checked out\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.rc1\n"},{"id":"224539","messageId":"20130803181415.GF2893@elie.Belkin","threadId":"34607","inReplyTo":"1375550060-5406-2-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-03T18:14:15Z","receivedAt":"2013-08-03T18:14:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> cmd_summary reads the output of git diff, but reads in the submodule\n> path into a variable called name.  Since this variable does not\n> contain the name of the submodule, but the path, rename it to be\n> clearer what data it actually holds.\n\nNice.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"224540","messageId":"20130803182420.GG2893@elie.Belkin","threadId":"34607","inReplyTo":"1375550060-5406-3-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-03T18:24:20Z","receivedAt":"2013-08-03T18:24:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\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> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n>  git-submodule.sh  | 2 ++\n>  t/t7508-status.sh | 4 ++--\n>  2 files changed, 4 insertions(+), 2 deletions(-)\n\nThanks.  Cc-ing Jens, who wrote that test and knows this code much\nbetter than I do. :)\n\n[...]\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -1034,6 +1034,8 @@ cmd_summary() {\n>  \t\tsane_egrep '^:([0-7]* )?160000' |\n>  \t\twhile read mod_src mod_dst sha1_src sha1_dst status path\n>  \t\tdo\n> +\t\t\tname=$(module_name \"$path\")\n> +\t\t\ttest $(get_submodule_config \"$name\" ignore none) = all && continue\n>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n>  \t\t\ttest $status = D -o $status = T && echo \"$path\" && continue\n\nI'm not sure what the exact semantics should be here, though that's\nmostly because of my unfamiliarity with submodules in general.\n\nIf I have '[submodule \"favorite\"] ignore = all' and I then replace\nthat submodule with a blob, should \"git submodule status\" not mention\nthat path?\n\nIf I just renamed a submodule, will 'module_name \"$path\"' do the right\nthing with the old path?\n\n(rest of the patch kept unsnipped for reference)\n>  \t\t\t# Also show added or modified modules which are checked out\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> 1.8.4.rc1\n"},{"id":"224560","messageId":"51FE90B8.8030203@web.de","threadId":"34607","inReplyTo":"20130803181415.GF2893@elie.Belkin","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-04T17:34:48Z","receivedAt":"2013-08-04T17:34:48Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 03.08.2013 20:14, schrieb Jonathan Nieder:\n> brian m. carlson wrote:\n> \n>> cmd_summary reads the output of git diff, but reads in the submodule\n>> path into a variable called name.  Since this variable does not\n>> contain the name of the submodule, but the path, rename it to be\n>> clearer what data it actually holds.\n> \n> Nice.\n\nI totally agree. Paths and names of submodules are often confused\n(especially as they are identical for a submodule that was never\nmoved), so cleaning up our sources so they use the correct term is\nThe Right Thing.\n\nBut we'll have to use sm_path here (like everywhere else in the\nsubmodule script), because we'll run into problems under Windows\notherwise (see 64394e3ae9 for details). Apart from that the patch\nis fine.\n"},{"id":"224563","messageId":"51FE9C49.4020902@web.de","threadId":"34607","inReplyTo":"20130803182420.GG2893@elie.Belkin","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-04T18:24:09Z","receivedAt":"2013-08-04T18:24:09Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 03.08.2013 20:24, schrieb Jonathan Nieder:\n> brian m. carlson wrote:\n> \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\nI'm a bit confused. The commit message talks about \"git status\", but the code\nyou changed handles \"git submodule summary\". Looks like you are trying to fix\nthe output of status when the status.submodulesummary option is set, right?\nThat's a good thing to do.\n\nBut your patch also changes the default behavior of \"git submodule summary\",\nwhich is a change in behavior as that is currently not configurable via the\nignore option (and I believe it should stay that way for backward compatibility\nreasons unless actual users provide sound reasons to change that). So a NACK\non this patch from me (and a note to self that tests are missing that should\nhave failed due to this change).\n\nAs a short term solution you could honor the submodule.<name>.ignore setting\nonly if --for-status is used, as that is explicitly given by \"git status\" when\nit forks the \"git submodule summary\" script (to make it prepend \"# \" to each\nline, which it could do easily itself nowadays using recently added code ;-).\n\nIf you want to fix that issue and make git status perform a lot better, you\nshould make the status.submodulesummary use what we already have in the diff\nmachinery instead of forking the submodule script (which it does for hysterical\nraisins). Basically you'd need to run just what \"git diff-files\" and \"git\ndiff-index HEAD\" run when they are given the --submodule option, prepend \"# \"\nto the output and limit it to the amount of lines configured via the\nstatus.submodulesummary option. Then we could get rid of the --for-status\noption of submodule summary and move some more functionality from that script\ninto core git.\n\nI'll be glad to help you fixing this problem either way.\n\n>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n>> ---\n>>  git-submodule.sh  | 2 ++\n>>  t/t7508-status.sh | 4 ++--\n>>  2 files changed, 4 insertions(+), 2 deletions(-)\n> \n> Thanks.  Cc-ing Jens, who wrote that test and knows this code much\n> better than I do. :)\n>\n> [...]\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -1034,6 +1034,8 @@ cmd_summary() {\n>>  \t\tsane_egrep '^:([0-7]* )?160000' |\n>>  \t\twhile read mod_src mod_dst sha1_src sha1_dst status path\n>>  \t\tdo\n>> +\t\t\tname=$(module_name \"$path\")\n>> +\t\t\ttest $(get_submodule_config \"$name\" ignore none) = all && continue\n>>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n>>  \t\t\ttest $status = D -o $status = T && echo \"$path\" && continue\n> \n> I'm not sure what the exact semantics should be here, though that's\n> mostly because of my unfamiliarity with submodules in general.\n> \n> If I have '[submodule \"favorite\"] ignore = all' and I then replace\n> that submodule with a blob, should \"git submodule status\" not mention\n> that path?\n> \n> If I just renamed a submodule, will 'module_name \"$path\"' do the right\n> thing with the old path?\n> \n> (rest of the patch kept unsnipped for reference)\n>>  \t\t\t# Also show added or modified modules which are checked out\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>> 1.8.4.rc1\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"224564","messageId":"20130804212938.GL19369@paksenarrion.iveqy.com","threadId":"34607","inReplyTo":"51FE90B8.8030203@web.de","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-08-04T21:29:38Z","receivedAt":"2013-08-04T21:29:38Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:\n> But we'll have to use sm_path here (like everywhere else in the\n> submodule script), because we'll run into problems under Windows\n> otherwise (see 64394e3ae9 for details). Apart from that the patch\n> is fine.\n\nWe're still using path= in the foreach-script. Or rather, we're setting\nit. From what I can see and from the commit message 64394e3ae9 it could\npossible be a problem.\n\nNot sure how to solve it though... Just a simple correction would break\nall script depending on that.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"224691","messageId":"5201335C.5040001@web.de","threadId":"34607","inReplyTo":"20130804212938.GL19369@paksenarrion.iveqy.com","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-08-06T17:33:16Z","receivedAt":"2013-08-06T17:33:16Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 04.08.2013 23:29, schrieb Fredrik Gustafsson:\n> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:\n>> But we'll have to use sm_path here (like everywhere else in the\n>> submodule script), because we'll run into problems under Windows\n>> otherwise (see 64394e3ae9 for details). Apart from that the patch\n>> is fine.\n> \n> We're still using path= in the foreach-script. Or rather, we're setting\n> it. From what I can see and from the commit message 64394e3ae9 it could\n> possible be a problem.\n\nThe commit message of 64394e3ae9 explicitly talks about that one:\n\n  We note that the foreach subcommand provides $path to user scripts\n  (ie it is part of the API), so we can't simply rename it to $sm_path.\n\n> Not sure how to solve it though... Just a simple correction would break\n> all script depending on that.\n\nThat's exactly why that one is still there. We could deprecate it in\nfavor of $sm_path and remove it some time later, but I'm not convinced\nit is an urgent issue. But me thinks the documentation of foreach should\nbe updated, as it still talks about $path.\n"},{"id":"224919","messageId":"5203D8F6.2020102@ramsay1.demon.co.uk","threadId":"34607","inReplyTo":"20130804212938.GL19369@paksenarrion.iveqy.com","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-08-08T17:44:22Z","receivedAt":"2013-08-08T17:44:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Fredrik Gustafsson wrote:\n> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:\n>> But we'll have to use sm_path here (like everywhere else in the\n>> submodule script), because we'll run into problems under Windows\n>> otherwise (see 64394e3ae9 for details). Apart from that the patch\n>> is fine.\n> \n> We're still using path= in the foreach-script. Or rather, we're setting\n> it. From what I can see and from the commit message 64394e3ae9 it could\n> possible be a problem.\n\nPlease do not use a $path variable in any script intended to be run on\nwindows; those poor souls who would otherwise have to fix the bugs will\nthank you! :-D\n\nActually, it's not so much the use of a $path variable, rather the act\nof _exporting_ such a variable that causes the problem. (Which is why\nusing $path with eval_gettext[ln] is such a problem, of course.)\n\nAs noted in the above commit, $path is unfortunately a documented part\nof the public API for the foreach subcommand. However, the foreach\nsubcommand is (mostly) fine; given the fact that the user script is\neval-ed in a context in which $path is not exported. The reason for\nthe 'mostly' is simply that the user could shoot himself in the foot\nby export-ing $path in their script, so that something like:\n\n    $ git submodule foreach 'export path; echo $path `git rev-parse HEAD`'\n\nwill indeed fail (ie git rev-parse will not execute).\n\n> Not sure how to solve it though... Just a simple correction would break\n> all script depending on that.\n\n$path is part of the public API, so we can't just remove it. It would\nrequire a deprecation period, etc,.  (Adding/documenting $sm_path as an\nalternative *may* be worth doing. dunno.)\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"224921","messageId":"20130809172632.GB25779@paksenarrion.iveqy.com","threadId":"34607","inReplyTo":"5203D8F6.2020102@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-08-09T17:26:32Z","receivedAt":"2013-08-09T17:26:32Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Thu, Aug 08, 2013 at 06:44:22PM +0100, Ramsay Jones wrote:\n> $path is part of the public API, so we can't just remove it. It would\n> require a deprecation period, etc,.  (Adding/documenting $sm_path as an\n> alternative *may* be worth doing. dunno.)\n\nMaybe something for git 2.0? Well, Jens and Junio is the ones who can\nmake sane decissions about this. I trust they make a good decision. The\nstate as now is that this is a bug for case insesitive systems.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"224933","messageId":"7va9kq66h6.fsf@alter.siamese.dyndns.org","threadId":"34607","inReplyTo":"5203D8F6.2020102@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-09T18:53:41Z","receivedAt":"2013-08-09T18:53:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> $path is part of the public API, so we can't just remove it. It would\n> require a deprecation period, etc,.  (Adding/documenting $sm_path as an\n> alternative *may* be worth doing. dunno.)\n\nI think exporting sm_path (if not done already) and documenting the\ntransition may be a good starting point, but deprecation of $path\nmay be tricky.  There is no good way for us to warn people who\ncontinue using $path and ask them to fix their fingers and scripts\nto use $sm_path instead.\n"},{"id":"225028","messageId":"20130810163709.GD164436@vauxhall.crustytoothpaste.net","threadId":"34607","inReplyTo":"51FE9C49.4020902@web.de","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-10T16:37:09Z","receivedAt":"2013-08-10T16:37:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Aug 04, 2013 at 08:24:09PM +0200, Jens Lehmann wrote:\n> I'm a bit confused. The commit message talks about \"git status\", but the code\n> you changed handles \"git submodule summary\". Looks like you are trying to fix\n> the output of status when the status.submodulesummary option is set, right?\n> That's a good thing to do.\n> \n> But your patch also changes the default behavior of \"git submodule summary\",\n> which is a change in behavior as that is currently not configurable via the\n> ignore option (and I believe it should stay that way for backward compatibility\n> reasons unless actual users provide sound reasons to change that). So a NACK\n> on this patch from me (and a note to self that tests are missing that should\n> have failed due to this change).\n\nRight, that wasn't the intent.\n\n> As a short term solution you could honor the submodule.<name>.ignore setting\n> only if --for-status is used, as that is explicitly given by \"git status\" when\n> it forks the \"git submodule summary\" script (to make it prepend \"# \" to each\n> line, which it could do easily itself nowadays using recently added code ;-).\n\nI think I'm going to go this route.  My goal is to fix up the TODO tests\nand make them work so I can get more familiar with the code.\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"},{"id":"225058","messageId":"20130811160316.GE164436@vauxhall.crustytoothpaste.net","threadId":"34607","inReplyTo":"20130803182420.GG2893@elie.Belkin","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-11T16:03:17Z","receivedAt":"2013-08-11T16:03:17Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Aug 03, 2013 at 11:24:20AM -0700, Jonathan Nieder wrote:\n> If I have '[submodule \"favorite\"] ignore = all' and I then replace\n> that submodule with a blob, should \"git submodule status\" not mention\n> that path?\n\nYes, I think it should.  I'll fix this in the reroll.\n\n> If I just renamed a submodule, will 'module_name \"$path\"' do the right\n> thing with the old path?\n\nmodule_name uses whatever's in .gitmodules.  I'm not sure what you mean\nby \"renamed a submodule\", since \"git mv foo bar\" fails with:\n\n  vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq\n  fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq\n\nCan you provide me a set of steps to reproduce that operation so I can\ntest it effectively?\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"},{"id":"225059","messageId":"20130811183350.GA2942@elie.Belkin","threadId":"34607","inReplyTo":"20130811160316.GE164436@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-11T18:33:50Z","receivedAt":"2013-08-11T18:33:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> module_name uses whatever's in .gitmodules.  I'm not sure what you mean\n> by \"renamed a submodule\", since \"git mv foo bar\" fails with:\n>\n>   vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq\n>   fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq\n>\n> Can you provide me a set of steps to reproduce that operation so I can\n> test it effectively?\n\nAh, I forgot Jens's \"submodule-mv\" patch series has not hit master yet.\nYou can get it by running\n\n\tgit merge origin/pu^{/jl/submodule-mv}^2\n\nbefore building git.\n\nThanks,\nJonathan\n"},{"id":"225060","messageId":"5207EBD2.6050802@gmail.com","threadId":"34607","inReplyTo":"5203D8F6.2020102@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/2] submodule: fix confusing variable name","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-08-11T19:53:54Z","receivedAt":"2013-08-11T19:53:54Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 08/08/2013 01:44 PM, Ramsay Jones wrote:\n> Fredrik Gustafsson wrote:\n>> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:\n>>> But we'll have to use sm_path here (like everywhere else in the\n>>> submodule script), because we'll run into problems under Windows\n>>> otherwise (see 64394e3ae9 for details). Apart from that the patch\n>>> is fine.\n>>\n>> We're still using path= in the foreach-script. Or rather, we're setting\n>> it. From what I can see and from the commit message 64394e3ae9 it could\n>> possible be a problem.\n>\n> Please do not use a $path variable in any script intended to be run on\n> windows; those poor souls who would otherwise have to fix the bugs will\n> thank you! :-D\n>\n> Actually, it's not so much the use of a $path variable, rather the act\n> of _exporting_ such a variable that causes the problem. (Which is why\n> using $path with eval_gettext[ln] is such a problem, of course.)\n>\n\nPlease note that especially in this case, Cygwin != Windows. Cygwin \nallows $path, $Path, $PATH, etc., to all coexist and be accessed case \nsensitively. Exporting $path causes no problem, either. Should the eval \ninvoke a Windows program, $PATH is converted to Windows format and \nexported, the other case-sensitive variants of path remain in the \nenvironment and can be accessed by any program implementing \ncase-sensitive lookup as well. Not sure what will happen with \ncase-insensitive lookups, but a quick test showed that cmd.exe is not \nbothered by $path given $PATH exists.\n\nMark\n"},{"id":"225376","messageId":"20130817162725.GE64402@vauxhall.crustytoothpaste.net","threadId":"34607","inReplyTo":"20130811160316.GE164436@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 2/2] submodule: don't print status output with ignore=all","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-17T16:27:25Z","receivedAt":"2013-08-17T16:27:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Aug 11, 2013 at 04:03:17PM +0000, brian m. carlson wrote:\n> On Sat, Aug 03, 2013 at 11:24:20AM -0700, Jonathan Nieder wrote:\n> > If I just renamed a submodule, will 'module_name \"$path\"' do the right\n> > thing with the old path?\n> \n> module_name uses whatever's in .gitmodules.  I'm not sure what you mean\n> by \"renamed a submodule\", since \"git mv foo bar\" fails with:\n> \n>   vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq\n>   fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq\n\nOkay, I've tested this against next and it seems that the code handles\nit properly (at least I think it does).  I left the following code\n\n  # Always show modules deleted or type-changed (blob<->module)\n  test $status = D -o $status = T && echo \"$sm_path\" && continue\n\nbefore the ignore code, so I adjusted the new ignore code so that it\nprints both the new and the old path.  I don't know that it's possible\nto print neither in that case, since we no longer can look up the old\npath in .gitmodules.  I'll be sending my reroll shortly.\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"}]}