{"thread":{"id":"41868","subject":"[PATCHv4 1/6] submodule foreach: correct path display in recursive submodules","startedAt":"2016-03-30T01:27:40Z","lastAt":"2016-03-30T20:05:25Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282175","messageId":"1459301266-12403-1-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":null,"subject":"[PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:40Z","receivedAt":"2016-03-30T01:27:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n* addressed Junios comments in patch 1&2, which is:\n1)\n * reworded commit message\n * when writing the commit message I discovered a new way to fix the bug\n (fix the computation of the displaypath instead of its parameters\n wt_prefix and prefix.)\n \n The result is the same, but I am not yet sure if I like it more.\n\n2)\n * Use `git -C ` instead of `cd && git`.\n * Reword commit message\n\nv3:\nResending without `test_pause` in the last test.\n\nv2:\n\nThe first 3 commits are\n* Test and bugfix in one commit each\n* better explained than before\n\nThe expansion of an expected test result moved to the back of the series.\n\nThere are two new commits\n* one being another bugfix of the display path for `submodule update`\n* another test for `submodule update` as I suspect it may break further on\n refactoring and we currently have no tests for it.\n\nThanks,\nStefan\n\nStefan Beller (6):\n submodule foreach: correct path display in recursive submodules\n submodule update --init: correct path handling in recursive submodules\n submodule status: correct path handling in recursive submodules\n submodule update: align reporting path for custom command execution\n submodule update: test recursive path reporting from subdirectory\n t7407: make expectation as clear as possible\n\n git-submodule.sh | 13 +++----\n t/t7406-submodule-update.sh | 82 ++++++++++++++++++++++++++++++++++++++++++--\n t/t7407-submodule-foreach.sh | 49 ++++++++++++++++++++++++--\n 3 files changed, 133 insertions(+), 11 deletions(-)\n\n-- \n2.8.0.2.gb331331\n"},{"id":"282174","messageId":"1459301266-12403-2-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":"1459301266-12403-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 1/6] submodule foreach: correct path display in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:41Z","receivedAt":"2016-03-30T01:27:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `prefix` was put in front of the display path unconditionally.\nThis is wrong as any relative path computation would need to be at\nthe front, so include the prefix into the display path.\n\nThe new test replicates the previous test with the difference of executing\nfrom a sub directory. By executing from a sub directory all we would\nexpect all displayed paths to be prefixed by '../'.\n\nPrior to this patch the test would report\n Entering 'nested1/nested2/../nested3'\ninstead of the expected\n Entering '../nested1/nested2/nested3'\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n * reworded commit message\n * when writing the commit message I discovered a new way to fix the bug\n (fix the computation of the displaypath instead of its parameters\n wt_prefix and prefix.)\n \n The result is the same, but I am not yet sure if I like it more.\n\n git-submodule.sh | 6 +++---\n t/t7407-submodule-foreach.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..b3f248c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -413,8 +413,8 @@ cmd_foreach()\n \t\tdie_if_unmatched \"$mode\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n-\t\t\tdisplaypath=$(relative_path \"$sm_path\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n+\t\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n+\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n \t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n@@ -434,7 +434,7 @@ cmd_foreach()\n \t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n \t\t\t\tfi\n \t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$prefix\\$displaypath'; script returned non-zero status.\")\"\n+\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n \t\tfi\n \tdone\n }\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 7ca10b8..776b349 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 <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+cat > expect <\n---\n\nNotes:\n * Use `git -C ` instead of `cd && git`.\n * Reword commit message\n\n git-submodule.sh | 2 +-\n t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex b3f248c..9fffa5c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -473,7 +473,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\")\"\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 68ea31d..d4745f4 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=$(git -C super rev-parse HEAD)\n+mergingsha1=$(git -C super/merging rev-parse HEAD)\n+nonesha1=$(git -C super/none rev-parse HEAD)\n+rebasingsha1=$(git -C super/rebasing rev-parse HEAD)\n+submodulesha1=$(git -C super/submodule rev-parse HEAD)\n+pwd=$(pwd)\n+\n+cat <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_success 'submodule update --init --recursive from subdirectory' '\n+\tgit -C recursivesuper/super reset --hard HEAD^ &&\n+\t(cd recursivesuper &&\n+\t mkdir tmp &&\n+\t cd tmp &&\n+\t git submodule update --init --recursive ../super >../../actual\n+\t) &&\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.2.gb331331\n"},{"id":"282180","messageId":"1459301266-12403-4-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":"1459301266-12403-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 3/6] submodule status: correct path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:43Z","receivedAt":"2016-03-30T01:27:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new test which is a replica of the previous test except\nthat it executes from a sub directory. Prior to this patch\nthe test failed by having too many '../' prefixed:\n\n --- expect\t2016-03-29 19:02:33.087336115 +0000\n +++ actual\t2016-03-29 19:02:33.359343311 +0000\n @@ -1,7 +1,7 @@\n b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)\n -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)\n - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 (heads/master)\n - 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../nested1/nested2/nested3/submodule (heads/master)\n ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)\n + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 (heads/master)\n + 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../../../../nested1/nested2/nested3/submodule (heads/master)\n 0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)\n 0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)\n 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)\n\nThe path code in question:\n displaypath=$(relative_path \"$prefix$sm_path\")\n prefix=$displaypath\n if recursive:\n eval cmd_status\n\nThat way we change `prefix` each iteration to contain another\n'../', because of the the relative_path computation is done\non an already computed relative path.\n\nWe must call relative_path exactly once with `wt_prefix` non empty.\nFurther calls in recursive instances to to calculate the displaypath\nalready incorporate the correct prefix from before. Fix the issue by\nclearing `wt_prefix` in recursive calls.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 1 +\n t/t7407-submodule-foreach.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9fffa5c..1024774 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1159,6 +1159,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 776b349..4b35e12 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -277,6 +277,27 @@ test_expect_success 'ensure \"status --cached --recursive\" preserves the --cached\n \ttest_cmp expect actual\n '\n \n+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)\n+\n+cat > expect < ../../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'use \"git clone --recursive\" to checkout all submodules' '\n \tgit clone --recursive super clone4 &&\n \t(\n-- \n2.8.0.2.gb331331\n"},{"id":"282177","messageId":"1459301266-12403-5-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":"1459301266-12403-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 4/6] submodule update: align reporting path for custom command execution","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:44Z","receivedAt":"2016-03-30T01:27:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the predefined actions (merge, rebase, none, checkout), we use\nthe display path, which is relative to the current working directory.\nAlso use the display path when running a custom command.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 4 ++--\n t/t7406-submodule-update.sh | 29 ++++++++++++++++++++++++++---\n 2 files changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1024774..753a90d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -802,8 +802,8 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\t;;\n \t\t\t!*)\n \t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$prefix\\$sm_path'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$prefix\\$sm_path': '\\$command \\$sha1'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n+\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t*)\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex d4745f4..01dd324 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in .git/config' '\n \t)\n '\n \n+cat << EOF >expect\n+Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n+EOF\n+\n test_expect_success 'submodule update - command in .git/config catches failure' '\n \t(cd super &&\n \t git config submodule.submodule.update \"!false\"\n \t) &&\n \t(cd super/submodule &&\n-\t git reset --hard HEAD^\n+\t git reset --hard $submodulesha1^\n \t) &&\n \t(cd super &&\n-\t test_must_fail git submodule update submodule\n-\t)\n+\t test_must_fail git submodule update submodule 2>../actual\n+\t) &&\n+\ttest_cmp actual expect\n+'\n+\n+cat << EOF >expect\n+Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n+EOF\n+\n+test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '\n+\t(cd super &&\n+\t git config submodule.submodule.update \"!false\"\n+\t) &&\n+\t(cd super/submodule &&\n+\t git reset --hard $submodulesha1^\n+\t) &&\n+\t(cd super &&\n+\t mkdir tmp && cd tmp &&\n+\t test_must_fail git submodule update ../submodule 2>../../actual\n+\t) &&\n+\ttest_cmp actual expect\n '\n \n test_expect_success 'submodule init does not copy command into .git/config' '\n-- \n2.8.0.2.gb331331\n"},{"id":"282178","messageId":"1459301266-12403-6-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":"1459301266-12403-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 5/6] submodule update: test recursive path reporting from subdirectory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:45Z","receivedAt":"2016-03-30T01:27:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch is just a test and fixes no bug as there is currently no bug\nin the path handling of `submodule update`.\n\nIn `submodule update` we make a call to `submodule--helper list --prefix\n\"$wt_prefix\"` which looks a bit brittle and likely to introduce a bug\nfor the path handling. It is not a bug as the prefix is ignored inside\nthe submodule helper for now. If this test breaks eventually, we want\nto make sure the `wt_prefix` is passed correctly into recursive submodules.\nHint: In recursive submodules we expect `wt_prefix` to be empty.\n\nSigned-off-by: Stefan Beller \n---\n t/t7406-submodule-update.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 01dd324..e5af4b4 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -379,6 +379,26 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n \ttest_cmp actual expect\n '\n \n+cat << EOF >expect\n+Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n+Failed to recurse into submodule path '../super'\n+EOF\n+\n+test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '\n+\t(cd recursivesuper &&\n+\t git submodule update --remote super &&\n+\t git add super &&\n+\t git commit -m \"update to latest to have more than one commit in submodules\"\n+\t) &&\n+\tgit -C recursivesuper/super config submodule.submodule.update \"!false\" &&\n+\tgit -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&\n+\t(cd recursivesuper &&\n+\t mkdir -p tmp && cd tmp &&\n+\t test_must_fail git submodule update --recursive ../super 2>../../actual\n+\t) &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'submodule init does not copy command into .git/config' '\n \t(cd super &&\n \t H=$(git ls-files -s submodule | cut -d\" \" -f2) &&\n-- \n2.8.0.2.gb331331\n"},{"id":"282179","messageId":"1459301266-12403-7-git-send-email-sbeller@google.com","threadId":"41868","inReplyTo":"1459301266-12403-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 6/6] t7407: make expectation as clear as possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-30T01:27:46Z","receivedAt":"2016-03-30T01:27:46Z","isPatch":false,"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 \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 4b35e12..6ba5daf 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 < writes:\n\n> The `prefix` was put in front of the display path unconditionally.\n> This is wrong as any relative path computation would need to be at\n> the front, so include the prefix into the display path.\n>\n> The new test replicates the previous test with the difference of executing\n> from a sub directory. By executing from a sub directory all we would\n> expect all displayed paths to be prefixed by '../'.\n>\n> Prior to this patch the test would report\n> Entering 'nested1/nested2/../nested3'\n> instead of the expected\n> Entering '../nested1/nested2/nested3'\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nDefinitely easier to read and helps understand what is going on in\nthe changed codepath much better. Very well done.\n\nThanks.\n\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 7ca10b8..776b349 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 < +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_success '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 < nested1-nested1\n> nested2-nested2\n> nested3-nested3\n"}]}