{"thread":{"id":"41852","subject":"[PATCH 1/7] submodule foreach: test path handling in recursive submodules","startedAt":"2016-03-28T23:28:16Z","lastAt":"2016-03-29T19:49:50Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"282010","messageId":"1459207703-1635-1-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":null,"subject":"[PATCH 0/7] Fix path bugs in submodule commands executed from sub dir [WAS: submodule--helper clone: lose the extra prefix option]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:16Z","receivedAt":"2016-03-28T23:28:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Junio wrote:\n> I suspect that the fix in your 1&2 may be demonstratable without\n> forcing an early failure by switching to \"git -C\". \n\nSo for now I present test coverage and their minimal fixes.\nThis series follows a \"tick-tock\" pattern except for patch5,\nwhich I wrote quickly as I was annoying by the bells and whistles.\nI expect test code to be dumb, not tricking ourselves by \"smart\" code there.\n\nThe \"tick\" patches introduce failing tests. They need to fail to demonstrate\nthe bugs exist, which are fixed in the \"tock\" patches, which are doing\nnothing fancy but just a one or two line correction of the path handling\ncode.\n\nThis applies to 2.8.\n\nAs this is taking a completely different turn than I expected in \n\"[PATCHv3 0/5] submodule helper: cleanup prefix passing\", I made this\na new series. (It also doesn't do cleanup any more, but just fixes bugs.)\n\n\nThanks,\nStefan\n\nStefan Beller (7):\n  submodule foreach: test path handling in recursive submodules\n  submodule foreach: correct path computation in recursive submodules\n  submodule update --init: test path handling in recursive submodules\n  submodule update --init: correct path handling in recursive submodules\n  t7407: make expectation as clear as possible\n  submodule status: test path handling in recursive submodules\n  submodule status: fix path handling in recursive submodules\n\n git-submodule.sh             |  9 ++++++---\n t/t7406-submodule-update.sh  | 33 +++++++++++++++++++++++++++++++\n t/t7407-submodule-foreach.sh | 47 ++++++++++++++++++++++++++++++++++++++++++--\n 3 files changed, 84 insertions(+), 5 deletions(-)\n\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282007","messageId":"1459207703-1635-2-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/7] submodule foreach: test path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:17Z","receivedAt":"2016-03-28T23:28:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replicates the previous test with the difference of executing\nfrom a sub directory. By executing from a sub directory all we would\nexpect all paths to be prefixed by '../'.\n\nDocument this as failure in this patch, to be fixed in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7407-submodule-foreach.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 7ca10b8..f868636 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -178,6 +178,26 @@ test_expect_success 'test messages from \"foreach --recursive\"' '\n '\n \n cat > expect <<EOF\n+Entering '../nested1'\n+Entering '../nested1/nested2'\n+Entering '../nested1/nested2/nested3'\n+Entering '../nested1/nested2/nested3/submodule'\n+Entering '../sub1'\n+Entering '../sub2'\n+Entering '../sub3'\n+EOF\n+\n+test_expect_failure 'test messages from \"foreach --recursive\" from subdirectory' '\n+\t(\n+\t\tcd clone2 &&\n+\t\tmkdir untracked &&\n+\t\tcd untracked &&\n+\t\tgit submodule foreach --recursive >../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+cat > expect <<EOF\n nested1-nested1\n nested2-nested2\n nested3-nested3\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282009","messageId":"1459207703-1635-3-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/7] submodule foreach: correct path computation in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:18Z","receivedAt":"2016-03-28T23:28:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The test which is fixed by this patch would report\n    Entering 'nested1/nested2/../nested3'\ninstead of the expected\n    Entering '../nested1/nested2/nested3'\n\nbecause the prefix is put unconditionally in front and after that a\ncomputed display path with is affected by `wt_prefix`. This is wrong as\nany relative path computation would need to be at the front. By emptying\nthe `wt_prefix` in recursive submodules and adding the information of any\nrelative path into the `prefix` this is fixed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh             | 3 ++-\n t/t7407-submodule-foreach.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..2838069 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -417,10 +417,11 @@ cmd_foreach()\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n \t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n \t\t\t\tclear_local_git_env\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\tsm_path=$(relative_path \"$sm_path\") &&\n+\t\t\t\twt_prefix=\n \t\t\t\t# we make $path available to scripts ...\n \t\t\t\tpath=$sm_path &&\n \t\t\t\tif test $# -eq 1\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex f868636..776b349 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -187,7 +187,7 @@ Entering '../sub2'\n Entering '../sub3'\n EOF\n \n-test_expect_failure 'test messages from \"foreach --recursive\" from subdirectory' '\n+test_expect_success 'test messages from \"foreach --recursive\" from subdirectory' '\n \t(\n \t\tcd clone2 &&\n \t\tmkdir untracked &&\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282008","messageId":"1459207703-1635-4-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/7] submodule update --init: test path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:19Z","receivedAt":"2016-03-28T23:28:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This demonstrates a failure to handle paths correctly.\nInstead of getting the expected\n    Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'\nthe `super` directory is omitted and you get\n    Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'\ninstead.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++\n 1 file changed, 33 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 68ea31d..c1b9ffa 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '\n \t git submodule add ../none none &&\n \t test_tick &&\n \t git commit -m \"none\"\n+\t) &&\n+\tgit clone . recursivesuper &&\n+\t( cd recursivesuper\n+\t git submodule add ../super super\n \t)\n '\n \n@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '\n \t)\n '\n \n+supersha1=$(cd super && git rev-parse HEAD)\n+mergingsha1=$(cd super/merging && git rev-parse HEAD)\n+nonesha1=$(cd super/none && git rev-parse HEAD)\n+rebasingsha1=$(cd super/rebasing && git rev-parse HEAD)\n+submodulesha1=$(cd super/submodule && git rev-parse HEAD)\n+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+test_expect_failure '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) &&\n+\ttest_cmp expect actual\n+'\n+\n apos=\"'\";\n test_expect_success 'submodule update does not fetch already present commits' '\n \t(cd submodule &&\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282014","messageId":"1459207703-1635-5-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/7] submodule update --init: correct path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:20Z","receivedAt":"2016-03-28T23:28:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This fixes the test introduced by the parent commit.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh            | 5 +++--\n t/t7406-submodule-update.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2838069..a7c8599 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -474,7 +474,7 @@ cmd_init()\n \t\tdie_if_unmatched \"$mode\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \n-\t\tdisplaypath=$(relative_path \"$sm_path\")\n+\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \n \t\t# Copy url setting when it is not set yet\n \t\tif test -z \"$(git config \"submodule.$name.url\")\"\n@@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n \t\t\t\tclear_local_git_env\n+\t\t\t\twt_prefix=\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_update\n \t\t\t)\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c1b9ffa..3bd1552 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'\n Submodule path '../super/submodule': checked out '${submodulesha1}'\n EOF\n \n-test_expect_failure 'submodule update --init --recursive from subdirectory' '\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-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282013","messageId":"1459207703-1635-6-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/7] t7407: make expectation as clear as possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:21Z","receivedAt":"2016-03-28T23:28:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Not everyone (including me) grasps the sed expression in a split second as\nthey would grasp the 4 lines printed as is.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7407-submodule-foreach.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 776b349..808c6c6 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -262,8 +262,12 @@ test_expect_success 'test \"status --recursive\"' '\n \ttest_cmp expect actual\n '\n \n-sed -e \"/nested2 /s/.*/+$nested2sha1 nested1\\/nested2 (file2~1)/;/sub[1-3]/d\" < expect > expect2\n-mv -f expect2 expect\n+cat > expect <<EOF\n+ $nested1sha1 nested1 (heads/master)\n++$nested2sha1 nested1/nested2 (file2~1)\n+ $nested3sha1 nested1/nested2/nested3 (heads/master)\n+ $submodulesha1 nested1/nested2/nested3/submodule (heads/master)\n+EOF\n \n test_expect_success 'ensure \"status --cached --recursive\" preserves the --cached flag' '\n \t(\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282012","messageId":"1459207703-1635-7-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/7] submodule status: test path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:22Z","receivedAt":"2016-03-28T23:28:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replicates the previous test except that it executes from a sub\ndirectory. Document the expected outcome, which currently fails\nby having too many '../' prefixed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 808c6c6..5c57151 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -263,6 +263,25 @@ test_expect_success 'test \"status --recursive\"' '\n '\n \n cat > expect <<EOF\n+ $nested1sha1 ../nested1 (heads/master)\n+ $nested2sha1 ../nested1/nested2 (heads/master)\n+ $nested3sha1 ../nested1/nested2/nested3 (heads/master)\n+ $submodulesha1 ../nested1/nested2/nested3/submodule (heads/master)\n+ $sub1sha1 ../sub1 ($sub1sha1_short)\n+ $sub2sha1 ../sub2 ($sub2sha1_short)\n+ $sub3sha1 ../sub3 (heads/master)\n+EOF\n+\n+test_expect_failure 'test \"status --recursive\" from sub directory' '\n+\t(\n+\t\tcd clone3 &&\n+\t\tmkdir tmp && cd tmp &&\n+\t\tgit submodule status --recursive > ../../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+cat > expect <<EOF\n  $nested1sha1 nested1 (heads/master)\n +$nested2sha1 nested1/nested2 (file2~1)\n  $nested3sha1 nested1/nested2/nested3 (heads/master)\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282011","messageId":"1459207703-1635-8-git-send-email-sbeller@google.com","threadId":"41852","inReplyTo":"1459207703-1635-1-git-send-email-sbeller@google.com","subject":"[PATCH 7/7] submodule status: fix path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-28T23:28:23Z","receivedAt":"2016-03-28T23:28:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This fixes the test which was introduced in the parent commit.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh             | 1 +\n t/t7407-submodule-foreach.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a7c8599..7503b27 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1161,6 +1161,7 @@ cmd_status()\n \t\t\t(\n \t\t\t\tprefix=\"$displaypath/\"\n \t\t\t\tclear_local_git_env\n+\t\t\t\twt_prefix=\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_status\n \t\t\t) ||\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 5c57151..91f9ca9 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -272,7 +272,7 @@ cat > expect <<EOF\n  $sub3sha1 ../sub3 (heads/master)\n EOF\n \n-test_expect_failure 'test \"status --recursive\" from sub directory' '\n+test_expect_success 'test \"status --recursive\" from sub directory' '\n \t(\n \t\tcd clone3 &&\n \t\tmkdir tmp && cd tmp &&\n-- \n2.8.0.rc4.23.gd22361a.dirty\n"},{"id":"282024","messageId":"xmqq60w525yd.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"1459207703-1635-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T05:44:58Z","receivedAt":"2016-03-29T05:44:58Z","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 test which is fixed by this patch would report\n>     Entering 'nested1/nested2/../nested3'\n> instead of the expected\n>     Entering '../nested1/nested2/nested3'\n>\n> because the prefix is put unconditionally in front and after that a\n> computed display path with is affected by `wt_prefix`. This is wrong as\n> any relative path computation would need to be at the front. By emptying\n> the `wt_prefix` in recursive submodules and adding the information of any\n> relative path into the `prefix` this is fixed.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nNicely explained and executed.\n\nFWIW, it is fine to have a fix and new tests to demonstrate the fix\nto pre-existing breakages in a single step.  It is easier to review\nwhen we can see the body of the test (as opposed to just the change\ns/expect_failure/expect_success/) in the same patch as the change to\nthe code for a focused and small fix like these patches 1 & 2; it is\neasy to partially revert the patch for such a focused and small fix\nwhen a reviewer wants to verify that the new tests fail without the\ncode change.\n\n>  git-submodule.sh             | 3 ++-\n>  t/t7407-submodule-foreach.sh | 2 +-\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43c68de..2838069 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -417,10 +417,11 @@ cmd_foreach()\n>  \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n>  \t\t\tname=$(git submodule--helper name \"$sm_path\")\n>  \t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> +\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n\nMake sure that \"$prefix$sm_path\" is given as a single string to\nrelative_path.  I'd probably write this like so:\n\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\t\tprefix=$(relative_path \"$prefix$sm_path\")/\n\nThanks.\n"},{"id":"282025","messageId":"xmqq1t6t25so.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"1459207703-1635-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/7] submodule update --init: test path handling in recursive submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T05:48:23Z","receivedAt":"2016-03-29T05:48:23Z","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> @@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '\n>  \t)\n>  '\n>  \n> +supersha1=$(cd super && git rev-parse HEAD)\n\n\"supersha1=$(git -C super rev-parse HEAD)\" perhaps?\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\nWhy all these {curly braces}?\n"},{"id":"282026","messageId":"xmqqwpolzvb6.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"1459207703-1635-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/7] submodule update --init: correct path handling in recursive submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T05:50:53Z","receivedAt":"2016-03-29T05:50:53Z","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> This fixes the test introduced by the parent commit.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n\nUnlike 2/7, this is kinda on the thin side in the explanation\ndepartment, it looks.\n\n> ---\n>  git-submodule.sh            | 5 +++--\n>  t/t7406-submodule-update.sh | 2 +-\n>  2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2838069..a7c8599 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -474,7 +474,7 @@ cmd_init()\n>  \t\tdie_if_unmatched \"$mode\"\n>  \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n>  \n> -\t\tdisplaypath=$(relative_path \"$sm_path\")\n> +\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n>  \n>  \t\t# Copy url setting when it is not set yet\n>  \t\tif test -z \"$(git config \"submodule.$name.url\")\"\n> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\tif test -n \"$recursive\"\n>  \t\tthen\n>  \t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> +\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n\nSame here as 2/7 (see the above hunk which does this correctly).\n\n>  \t\t\t\tclear_local_git_env\n> +\t\t\t\twt_prefix=\n>  \t\t\t\tcd \"$sm_path\" &&\n>  \t\t\t\teval cmd_update\n>  \t\t\t)\n"},{"id":"282084","messageId":"xmqqk2klxg6b.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"xmqq60w525yd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T19:00:44Z","receivedAt":"2016-03-29T19:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The test which is fixed by this patch would report\n>>     Entering 'nested1/nested2/../nested3'\n>> instead of the expected\n>>     Entering '../nested1/nested2/nested3'\n>>\n>> because the prefix is put unconditionally in front and after that a\n>> computed display path with is affected by `wt_prefix`. This is wrong as\n>> any relative path computation would need to be at the front. By emptying\n>> the `wt_prefix` in recursive submodules and adding the information of any\n>> relative path into the `prefix` this is fixed.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>\n> Nicely explained and executed.\n\nInterestingly, this breakage, as 1/7 shows, only affects the\n\"Entering $there\" message--I somehow expected from reading the\ndescription above that the command given to \"foreach\" is run\nin a wrong submodule directory, but there is no such bug that\nis fixed by this change, as far as \"foreach\" is concerned.\n\nWhich might be an indication that it wasn't so \"Nicely explained\"\nafter all X-<.  Are there codepaths that use the same incorrect\ninformation (which was fixed by this patch) for things other than\nthe message and chdir to an incorrect place?  Then this change is\nfixing more than \"just a bug in foreach message\".\n\nThe explanation does not make it clear what the extent of the fix\nis, in other words.\n\nNevertheless, it is a good fix for \"foreach message\".  Thanks.  It\njust needs to clarify if that is the only change, or if it fixes\nother things.\n"},{"id":"282091","messageId":"CAGZ79ka5_C0HcWGyYThT7O2ts-B1dTkBWEt1MUsbEN-AciE15g@mail.gmail.com","threadId":"41852","inReplyTo":"xmqqk2klxg6b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T19:21:21Z","receivedAt":"2016-03-29T19:21:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 29, 2016 at 12:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> The test which is fixed by this patch would report\n>>>     Entering 'nested1/nested2/../nested3'\n>>> instead of the expected\n>>>     Entering '../nested1/nested2/nested3'\n>>>\n>>> because the prefix is put unconditionally in front and after that a\n>>> computed display path with is affected by `wt_prefix`. This is wrong as\n>>> any relative path computation would need to be at the front. By emptying\n>>> the `wt_prefix` in recursive submodules and adding the information of any\n>>> relative path into the `prefix` this is fixed.\n>>>\n>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>> ---\n>>\n>> Nicely explained and executed.\n>\n> Interestingly, this breakage, as 1/7 shows, only affects the\n> \"Entering $there\" message--I somehow expected from reading the\n> description above that the command given to \"foreach\" is run\n> in a wrong submodule directory, but there is no such bug that\n> is fixed by this change, as far as \"foreach\" is concerned.\n\nforeach is a special beast as it is the only submodule command that\nignores the current directory, i.e.\n    cd repo/plugins && git submodule foreach ...\nalso affects submodules in repo/other-submodules.\n\nSo yeah this is only about the displaypath being relative to\nthe users cwd, internally we operate from the toplevel\ndirectory. :/\n\n>\n> Which might be an indication that it wasn't so \"Nicely explained\"\n> after all X-<.  Are there codepaths that use the same incorrect\n> information (which was fixed by this patch) for things other than\n> the message and chdir to an incorrect place?  Then this change is\n> fixing more than \"just a bug in foreach message\".\n\nEach submodule command does its own chdir and computation\nof the prefix and displaypath. The common thing is the name and\nthe meaning of the variables. Currently I am redoing the other patches\nto follow this patch. Apparently I still need to hone the commit message\nto actually transport the message that this only affects the display path.\n\n>\n> The explanation does not make it clear what the extent of the fix\n> is, in other words.\n>\n> Nevertheless, it is a good fix for \"foreach message\".  Thanks.  It\n> just needs to clarify if that is the only change, or if it fixes\n> other things.\n>\n\nThanks,\nStefan\n"},{"id":"282093","messageId":"CAGZ79kbaSNt9UViQQ2ro48Dqhe3c-k+CDvfppjb3Y=nOmnJs4A@mail.gmail.com","threadId":"41852","inReplyTo":"CAGZ79ka5_C0HcWGyYThT7O2ts-B1dTkBWEt1MUsbEN-AciE15g@mail.gmail.com","subject":"Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T19:26:30Z","receivedAt":"2016-03-29T19:26:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 29, 2016 at 12:21 PM, Stefan Beller <sbeller@google.com> wrote:\n> On Tue, Mar 29, 2016 at 12:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> The test which is fixed by this patch would report\n>>>>     Entering 'nested1/nested2/../nested3'\n>>>> instead of the expected\n>>>>     Entering '../nested1/nested2/nested3'\n>>>>\n>>>> because the prefix is put unconditionally in front and after that a\n>>>> computed display path with is affected by `wt_prefix`. This is wrong as\n>>>> any relative path computation would need to be at the front. By emptying\n>>>> the `wt_prefix` in recursive submodules and adding the information of any\n>>>> relative path into the `prefix` this is fixed.\n>>>>\n>>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>>> ---\n>>>\n>>> Nicely explained and executed.\n>>\n>> Interestingly, this breakage, as 1/7 shows, only affects the\n>> \"Entering $there\" message--I somehow expected from reading the\n>> description above that the command given to \"foreach\" is run\n>> in a wrong submodule directory, but there is no such bug that\n>> is fixed by this change, as far as \"foreach\" is concerned.\n>\n> foreach is a special beast as it is the only submodule command that\n> ignores the current directory, i.e.\n>     cd repo/plugins && git submodule foreach ...\n> also affects submodules in repo/other-submodules.\n\nI missspoke.\n\nIt actually respects the sub directory, but no further path spec. :(\n"},{"id":"282094","messageId":"xmqqfuv9xerx.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"1459207703-1635-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/7] t7407: make expectation as clear as possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T19:30:58Z","receivedAt":"2016-03-29T19:30:58Z","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> Not everyone (including me) grasps the sed expression in a split second as\n> they would grasp the 4 lines printed as is.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nI agree that the overlong sed expression is not very readable.\nSpelling the expectation out like this patch does would make sense.\nA slight possible downside is that future changes in the test setup\nmay require updating two places now instead of one, but I would say\nthat would not make a very strong objection--after all, such future\nchanges may affect the line that is munged by the sed script, in\nwhich case you'd need to change two places anyway (i.e. the previous\n\"expect\" and also the script), and updating two explicit expectation\nwould be far easier than updating one explicit expectation and the\noverlong sed expression.\n\nPerhaps this wants to come much earlier in the series?  It felt a\nbit distracting while reading the series in sequence, derailing my\ntrain of thought.\n\nThanks.\n\n>  t/t7407-submodule-foreach.sh | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 776b349..808c6c6 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -262,8 +262,12 @@ test_expect_success 'test \"status --recursive\"' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -sed -e \"/nested2 /s/.*/+$nested2sha1 nested1\\/nested2 (file2~1)/;/sub[1-3]/d\" < expect > expect2\n> -mv -f expect2 expect\n> +cat > expect <<EOF\n> + $nested1sha1 nested1 (heads/master)\n> ++$nested2sha1 nested1/nested2 (file2~1)\n> + $nested3sha1 nested1/nested2/nested3 (heads/master)\n> + $submodulesha1 nested1/nested2/nested3/submodule (heads/master)\n> +EOF\n>  \n>  test_expect_success 'ensure \"status --cached --recursive\" preserves the --cached flag' '\n>  \t(\n"},{"id":"282098","messageId":"xmqqbn5xxe25.fsf@gitster.mtv.corp.google.com","threadId":"41852","inReplyTo":"1459207703-1635-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/7] submodule update --init: correct path handling in recursive submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-29T19:46:26Z","receivedAt":"2016-03-29T19:46:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This fixes the test introduced by the parent commit.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nThe first hunk in this patch touches lines that goes away with\nd5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on\nyour sb/submodule-init topic and the whole block is replaced\nby a call to \"submodule--helper init\".\n\nI'll drop the first hunk when merging this series to 'pu' for now;\nhopefully you did not inherit the bug when rewriting the part into\n\"submodule--helper init\".\n\nThanks.\n\n>  git-submodule.sh            | 5 +++--\n>  t/t7406-submodule-update.sh | 2 +-\n>  2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2838069..a7c8599 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -474,7 +474,7 @@ cmd_init()\n>  \t\tdie_if_unmatched \"$mode\"\n>  \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n>  \n> -\t\tdisplaypath=$(relative_path \"$sm_path\")\n> +\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n>  \n>  \t\t# Copy url setting when it is not set yet\n>  \t\tif test -z \"$(git config \"submodule.$name.url\")\"\n> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\tif test -n \"$recursive\"\n>  \t\tthen\n>  \t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> +\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n>  \t\t\t\tclear_local_git_env\n> +\t\t\t\twt_prefix=\n>  \t\t\t\tcd \"$sm_path\" &&\n>  \t\t\t\teval cmd_update\n>  \t\t\t)\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index c1b9ffa..3bd1552 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'\n>  Submodule path '../super/submodule': checked out '${submodulesha1}'\n>  EOF\n>  \n> -test_expect_failure 'submodule update --init --recursive from subdirectory' '\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"},{"id":"282099","messageId":"CAGZ79kZZoxBsAfsTohrLqf2f01fmbjK2rrOG5vQ6ShT-zOjeJw@mail.gmail.com","threadId":"41852","inReplyTo":"xmqqbn5xxe25.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/7] submodule update --init: correct path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T19:49:50Z","receivedAt":"2016-03-29T19:49:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 29, 2016 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This fixes the test introduced by the parent commit.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>\n> The first hunk in this patch touches lines that goes away with\n> d5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on\n> your sb/submodule-init topic and the whole block is replaced\n> by a call to \"submodule--helper init\".\n>\n> I'll drop the first hunk when merging this series to 'pu' for now;\n> hopefully you did not inherit the bug when rewriting the part into\n> \"submodule--helper init\".\n\nAfter examining this patch more closely for a better commit message, the second\nhunk also goes away, i.e. only the test will remain.\n\nMaybe I can roll this series on top of origin/sb/submodule-init, to\navoid confusion.\n\n>\n> Thanks.\n>\n>>  git-submodule.sh            | 5 +++--\n>>  t/t7406-submodule-update.sh | 2 +-\n>>  2 files changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 2838069..a7c8599 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -474,7 +474,7 @@ cmd_init()\n>>               die_if_unmatched \"$mode\"\n>>               name=$(git submodule--helper name \"$sm_path\") || exit\n>>\n>> -             displaypath=$(relative_path \"$sm_path\")\n>> +             displaypath=$(relative_path \"$prefix$sm_path\")\n>>\n>>               # Copy url setting when it is not set yet\n>>               if test -z \"$(git config \"submodule.$name.url\")\"\n>> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n>>               if test -n \"$recursive\"\n>>               then\n>>                       (\n>> -                             prefix=\"$prefix$sm_path/\"\n>> +                             prefix=\"$(relative_path $prefix$sm_path)/\"\n>>                               clear_local_git_env\n>> +                             wt_prefix=\n>>                               cd \"$sm_path\" &&\n>>                               eval cmd_update\n>>                       )\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index c1b9ffa..3bd1552 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out '${rebasingsha1}'\n>>  Submodule path '../super/submodule': checked out '${submodulesha1}'\n>>  EOF\n>>\n>> -test_expect_failure 'submodule update --init --recursive from subdirectory' '\n>> +test_expect_success 'submodule update --init --recursive from subdirectory' '\n>>       git -C recursivesuper/super reset --hard HEAD^ &&\n>>       (cd recursivesuper &&\n>>        mkdir tmp &&\n"}]}