{"thread":{"id":"41865","subject":"[PATCH 3/6] submodule status: correct path handling in recursive submodules","startedAt":"2016-03-29T22:23:42Z","lastAt":"2016-03-29T22:29:11Z","messageCount":8,"participants":["Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"282128","messageId":"1459290228-9069-1-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":null,"subject":"[PATCHv2 0/6] Fix path bugs in submodule commands executed from sub dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:42Z","receivedAt":"2016-03-29T22:23:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 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  t7407: make expectation as clear as possible\n  submodule update: align reporting path for custom command execution\n  submodule update: test recursive path reporting from subdirectory\n\n git-submodule.sh             | 10 +++---\n t/t7406-submodule-update.sh  | 83 ++++++++++++++++++++++++++++++++++++++++++--\n t/t7407-submodule-foreach.sh | 49 ++++++++++++++++++++++++--\n 3 files changed, 133 insertions(+), 9 deletions(-)\n\n-- \n2.8.0.4.g5639dee.dirty\n"},{"id":"282129","messageId":"1459290228-9069-2-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/6] submodule foreach: correct path display in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:43Z","receivedAt":"2016-03-29T22:23:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 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\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 | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\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 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 <<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_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 <<EOF\n nested1-nested1\n nested2-nested2\n nested3-nested3\n-- \n2.8.0.4.g5639dee.dirty\n"},{"id":"282130","messageId":"1459290228-9069-3-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/6] submodule update --init: correct path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:44Z","receivedAt":"2016-03-29T22:23:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new test demonstrates a failure in the code prior to this patch.\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\nThat happens because the prefix is ignored in `git submodule add`, probably\nbecause that function itself cannot recurse; it may however called by\nrecursive instances of `git submodule update`, so we need to respect the\n`prefix`.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\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 2838069..fdb5fbd 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\")\"\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 68ea31d..9a4ba41 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_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.4.g5639dee.dirty\n"},{"id":"282127","messageId":"1459290228-9069-4-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/6] submodule status: correct path handling in recursive submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:45Z","receivedAt":"2016-03-29T22:23:45Z","isPatch":true,"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 <sbeller@google.com>\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 fdb5fbd..11ed32a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1160,6 +1160,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 <<EOF\n+ $nested1sha1 ../nested1 (heads/master)\n++$nested2sha1 ../nested1/nested2 (file2)\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_success '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 test_expect_success 'use \"git clone --recursive\" to checkout all submodules' '\n \tgit clone --recursive super clone4 &&\n \t(\n-- \n2.8.0.4.g5639dee.dirty\n"},{"id":"282131","messageId":"1459290228-9069-5-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/6] t7407: make expectation as clear as possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:46Z","receivedAt":"2016-03-29T22:23:46Z","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 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 <<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.4.g5639dee.dirty\n"},{"id":"282133","messageId":"1459290228-9069-6-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/6] submodule update: align reporting path for custom command execution","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:47Z","receivedAt":"2016-03-29T22:23:47Z","isPatch":true,"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 <sbeller@google.com>\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 11ed32a..be2a2b5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -803,8 +803,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 9a4ba41..f062065 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.4.g5639dee.dirty\n"},{"id":"282132","messageId":"1459290228-9069-7-git-send-email-sbeller@google.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/6] submodule update: test recursive path reporting from subdirectory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:23:48Z","receivedAt":"2016-03-29T22:23:48Z","isPatch":true,"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 <sbeller@google.com>\n---\n t/t7406-submodule-update.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f062065..239d31f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -379,6 +379,27 @@ 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_pause &&\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.4.g5639dee.dirty\n"},{"id":"282134","messageId":"CAGZ79kYivg3VvS6zavzjhP3wu3oaTYdEGWPYKvHrrs77yF8big@mail.gmail.com","threadId":"41865","inReplyTo":"1459290228-9069-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/6] Fix path bugs in submodule commands executed from sub dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-29T22:29:11Z","receivedAt":"2016-03-29T22:29:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Please ignore this series; it was sent out prematurely. :(\n"}]}