{"thread":{"id":"41820","subject":"[PATCH 1/4] submodule: fix recursive path printing from non root directory","startedAt":"2016-03-24T23:34:24Z","lastAt":"2016-03-25T18:32:46Z","messageCount":23,"participants":["Stefan Beller","Jacob Keller","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"281749","messageId":"1458862468-12460-1-git-send-email-sbeller@google.com","threadId":"41820","inReplyTo":null,"subject":"[PATCHv2 0/4] submodule helper: cleanup prefix passing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:34:24Z","receivedAt":"2016-03-24T23:34:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The first two patches fix two subtle bugs that would show up if we'd\napply the third patch without them. The third patch replaces\n \n git submodule--helper --prefix $wt_prefix list\n\nby the more Git idiomatic \n\n git -C $wt_prefix submodule--helper list\n\nThe series is finished by adding more tests which would have helped\nto have when developing this series.\n\nThis applies to origin/master.\n\nA prior version can be found at \n$gmane/287620\n\nThanks,\nStefan\n\nStefan Beller (4):\n submodule: fix recursive path printing from non root directory\n submodule: fix recursive execution from non root directory\n submodule--helper list: lose the extra prefix option\n submodule: add more tests for recursive submodule behavior\n\n builtin/submodule--helper.c | 5 +----\n git-submodule.sh | 19 +++++++++++--------\n t/t7403-submodule-sync.sh | 13 +++++++++----\n t/t7406-submodule-update.sh | 12 ++++++++++++\n 4 files changed, 33 insertions(+), 16 deletions(-)\n\n-- \n2.8.0.rc4.10.g52f3f33\n"},{"id":"281747","messageId":"1458862468-12460-2-git-send-email-sbeller@google.com","threadId":"41820","inReplyTo":"1458862468-12460-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/4] submodule: fix recursive path printing from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:34:25Z","receivedAt":"2016-03-24T23:34:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Recursing into submodules currently works by just calling\n(cd $submodule && eval ) for update, sync and status\ncommand.\n\nSay you have the following setup\n\nrepo/ # a superproject repository\nrepo/untracked/ # an untracked dir in repo/\nrepo/sub/ # a submodule\nrepo/sub/subsub # a submodule of a submodule\n\nWhen being in repo/untracked/ and invoking \"git submodule status\"\nyou would expect output like:\n\n repo/untracked/$ git submodule status --recursive\n ../sub (version)\n ../sub/subsub ()\n\nWe need to take into account that we are in the untracked/ dir,\nso we need to prepend ../ to the paths. By using relative_path\nto compute the prefix, we'll have that output.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..536ba68 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -825,7 +825,7 @@ 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\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_update\n@@ -1233,13 +1233,13 @@ cmd_sync()\n \t\t\tthen\n \t\t\t(\n \t\t\t\tclear_local_git_env\n+\t\t\t\tprefix=$(relative_path \"$prefix$sm_path/\")\n \t\t\t\tcd \"$sm_path\"\n \t\t\t\tremote=$(get_default_remote)\n \t\t\t\tgit config remote.\"$remote\".url \"$sub_origin_url\"\n \n \t\t\t\tif test -n \"$recursive\"\n \t\t\t\tthen\n-\t\t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\t\teval cmd_sync\n \t\t\t\tfi\n \t\t\t)\n-- \n2.8.0.rc4.10.g52f3f33\n"},{"id":"281748","messageId":"1458862468-12460-3-git-send-email-sbeller@google.com","threadId":"41820","inReplyTo":"1458862468-12460-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/4] submodule: fix recursive execution from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:34:26Z","receivedAt":"2016-03-24T23:34:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"One of the first things that happens in most submodule sub commands is\n\n git submodule--helper list --prefix \"$wt_prefix\"\n\nCurrently the passed --prefix is used for doing path calculation\nas if we were in that path relative to the repository root, which is\nwhy we need to pass \"$wt_prefix\". The more common way in Git however\nwould be to use\n\n git -C \"$wt_prefix\" submodule--helper list\n\nwhich I want to change later. That way however does not just\npass the prefix into the submodule command, but also changes\ninto that directory.\n\nSay you have the following setup\n\nrepo/ # a superproject repository\nrepo/untracked/ # an untracked dir in repo/\nrepo/sub/ # a submodule\nrepo/sub/subsub # a submodule of a submodule\n\nWhen in repo/untracked/ and invoking \"git submodule status --recursive\",\nthe recursed instance of the latter version for listing submodules would\ntry to change into the directory repo/sub/untracked, which is a bug.\nThis happens as we cd into the submodule in git-submodule.sh without\nclearing wt_prefix, which is the assumed relative path inside the working\ndirectory.\n\nMost times that directory doesn't exist and we error out. Fix this bug\nby clearing wt_prefix, such that any recursive instances of will assume\nto operate from the respective root of the respective submodule.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 536ba68..6b18a03 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -827,6 +827,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t(\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@@ -1159,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) ||\n@@ -1240,6 +1242,7 @@ cmd_sync()\n \n \t\t\t\tif test -n \"$recursive\"\n \t\t\t\tthen\n+\t\t\t\t\twt_prefix=\n \t\t\t\t\teval cmd_sync\n \t\t\t\tfi\n \t\t\t)\n-- \n2.8.0.rc4.10.g52f3f33\n"},{"id":"281750","messageId":"1458862468-12460-4-git-send-email-sbeller@google.com","threadId":"41820","inReplyTo":"1458862468-12460-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:34:27Z","receivedAt":"2016-03-24T23:34:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The usual early machinery of Git is to change the directory to\nthe top level of the working tree and pass the actual path inside\nthe working tree as `prefix` to the command being run.\nThis is the case both for commands written in C (where the\nprefix is passed into the command in a function parameter) as\nwell as in git-submodule.sh where the setup code runs\n\n wt_prefix=$(git rev-parse show-prefix)\n cd_to_top_level\n\nSo the prefix passed into the `submodule--helper list` is actually\nthe relative path inside the working tree, but we were not using\nthe standard way of passing it through.\n\nAdhere to Gits standard of passing the relative path inside the\nworking tree by passing it via -C.\n\nWe do not need to pass it for `submodule foreach` as that command\ndoesn't take further arguments ('$@') to operate on a subset of\nsubmodules, such that it is irrelevant for listing the submodules.\nThe computation of the displaypath ('Entering ') is done\nseparately there.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 5 +----\n git-submodule.sh | 12 ++++++------\n 2 files changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed764c9..2983783 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -68,14 +68,11 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \tstruct module_list list = MODULE_LIST_INIT;\n \n \tstruct option module_list_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"alternative anchor for relative paths\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper list [--prefix=] [...]\"),\n+\t\tN_(\"git submodule--helper list [...]\"),\n \t\tNULL\n \t};\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 6b18a03..1f7ad6e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -407,7 +407,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\"|\n+\tgit submodule--helper list |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -467,7 +467,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit -C \"$wt_prefix\" submodule--helper list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -549,7 +549,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit -C \"$wt_prefix\" submodule--helper list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -683,7 +683,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit -C \"$wt_prefix\" submodule--helper list \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1121,7 +1121,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit -C \"$wt_prefix\" submodule--helper list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1199,7 +1199,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit -C \"$wt_prefix\" submodule--helper list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-- \n2.8.0.rc4.10.g52f3f33\n"},{"id":"281751","messageId":"1458862468-12460-5-git-send-email-sbeller@google.com","threadId":"41820","inReplyTo":"1458862468-12460-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/4] submodule: add more tests for recursive submodule behavior","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:34:28Z","receivedAt":"2016-03-24T23:34:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This adds a test for \"submodule update\", wich calls \"submodule update\"\nfrom an untracked repository in the superproject. When doing creating\nthe parent patch a similar test failed for \"submodule sync\", but\nall tests passed for \"submodule update\". It took me a long time\nto figure out this was a difference in test coverage instead of\ncommands behaving differently. Let's improve the test coverage such\nto make it a better place.\n\nWhen trying to fix the issue in the parent patch I could get\nthe test suite passing when removing the $@ argument from module_list\nin the sync command. This also indicates a low test coverage, so\nfix that.\n\nSigned-off-by: Stefan Beller \n---\n t/t7403-submodule-sync.sh | 13 +++++++++----\n t/t7406-submodule-update.sh | 12 ++++++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh\nindex 79bc135..5dde123 100755\n--- a/t/t7403-submodule-sync.sh\n+++ b/t/t7403-submodule-sync.sh\n@@ -28,6 +28,9 @@ test_expect_success setup '\n \t\tgit submodule add ../submodule submodule &&\n \t\ttest_tick &&\n \t\tgit commit -m \"submodule\"\n+\t\tgit submodule add ../submodule submodule2 &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"second submodule\"\n \t) &&\n \tgit clone super super-clone &&\n \t(\n@@ -149,15 +152,16 @@ test_expect_success 'reset submodule URLs' '\n \treset_submodule_urls super-clone\n '\n \n-test_expect_success '\"git submodule sync\" should update submodule URLs - subdirectory' '\n+test_expect_success '\"git submodule sync\" should update specified submodule URLs - subdirectory' '\n \t(\n \t\tcd super-clone &&\n \t\tgit pull --no-recurse-submodules &&\n \t\tmkdir -p sub &&\n \t\tcd sub &&\n-\t\tgit submodule sync >../../output\n+\t\tgit submodule sync ../submodule >../../output\n \t) &&\n \tgrep \"\\\\.\\\\./submodule\" output &&\n+\t! grep submodule2 output &&\n \ttest -d \"$(\n \t\tcd super-clone/submodule &&\n \t\tgit config remote.origin.url\n@@ -177,7 +181,7 @@ test_expect_success '\"git submodule sync\" should update submodule URLs - subdire\n \t)\n '\n \n-test_expect_success '\"git submodule sync --recursive\" should update all submodule URLs - subdirectory' '\n+test_expect_success '\"git submodule sync --recursive\" should update all specified submodule URLs - subdirectory' '\n \t(\n \t\tcd super-clone &&\n \t\t(\n@@ -186,9 +190,10 @@ test_expect_success '\"git submodule sync --recursive\" should update all submodul\n \t\t) &&\n \t\tmkdir -p sub &&\n \t\tcd sub &&\n-\t\tgit submodule sync --recursive >../../output\n+\t\tgit submodule sync --recursive ../submodule >../../output\n \t) &&\n \tgrep \"\\\\.\\\\./submodule/sub-submodule\" output &&\n+\t! grep submodule2 output &&\n \ttest -d \"$(\n \t\tcd super-clone/submodule &&\n \t\tgit config remote.origin.url\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 68ea31d..628da7f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n \t test_i18ngrep \"Submodule path .deeper/submodule/subsubmodule.: checked out\" actual\n \t)\n '\n+\n+test_expect_success 'submodule update --recursive works from subdirectory' '\n+\t(cd super2 &&\n+\t (cd deeper/submodule/subsubmodule &&\n+\t git checkout HEAD^\n+\t ) &&\n+\t mkdir untracked &&\n+\t cd untracked &&\n+\t git submodule update --recursive >actual &&\n+\t test_i18ngrep \"Submodule path .../deeper/submodule/subsubmodule.: checked out\" actual\n+\t)\n+'\n test_done\n-- \n2.8.0.rc4.10.g52f3f33\n"},{"id":"281752","messageId":"CA+P7+xqbkK7GKf5=eFVYkBzmrjy9hOsWGTn0W31qWiT-0ZhuPQ@mail.gmail.com","threadId":"41820","inReplyTo":"1458862468-12460-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-24T23:38:31Z","receivedAt":"2016-03-24T23:38:31Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller wrote:\n> Recursing into submodules currently works by just calling\n> (cd $submodule && eval ) for update, sync and status\n> command.\n>\n> Say you have the following setup\n>\n> repo/ # a superproject repository\n> repo/untracked/ # an untracked dir in repo/\n> repo/sub/ # a submodule\n> repo/sub/subsub # a submodule of a submodule\n>\n> When being in repo/untracked/ and invoking \"git submodule status\"\n> you would expect output like:\n>\n> repo/untracked/$ git submodule status --recursive\n> ../sub (version)\n> ../sub/subsub ()\n>\n> We need to take into account that we are in the untracked/ dir,\n> so we need to prepend ../ to the paths. By using relative_path\n> to compute the prefix, we'll have that output.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> git-submodule.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43c68de..536ba68 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -825,7 +825,7 @@ 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> cd \"$sm_path\" &&\n> eval cmd_update\n> @@ -1233,13 +1233,13 @@ cmd_sync()\n> then\n> (\n> clear_local_git_env\n> + prefix=$(relative_path \"$prefix$sm_path/\")\n\nNot really sure why this got moved, but I don't think it hurts\nanything, though we will have prefix defined now regardless of if\nwe're recursive or not. But I think that's correct.\n\n> cd \"$sm_path\"\n> remote=$(get_default_remote)\n> git config remote.\"$remote\".url \"$sub_origin_url\"\n>\n> if test -n \"$recursive\"\n> then\n> - prefix=\"$prefix$sm_path/\"\n> eval cmd_sync\n> fi\n> )\n> --\n> 2.8.0.rc4.10.g52f3f33\n>\n\nReviewed-by: Jacob Keller \n\nRegards,\nJake\n"},{"id":"281753","messageId":"CA+P7+xqravWtkXQFC2aRg7=-mOdmQgu6Ci9QeFBMYUoy9YejWA@mail.gmail.com","threadId":"41820","inReplyTo":"1458862468-12460-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: fix recursive execution from non root directory","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-24T23:41:42Z","receivedAt":"2016-03-24T23:41:42Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller wrote:\n> One of the first things that happens in most submodule sub commands is\n>\n> git submodule--helper list --prefix \"$wt_prefix\"\n>\n> Currently the passed --prefix is used for doing path calculation\n> as if we were in that path relative to the repository root, which is\n> why we need to pass \"$wt_prefix\". The more common way in Git however\n> would be to use\n>\n> git -C \"$wt_prefix\" submodule--helper list\n>\n> which I want to change later. That way however does not just\n> pass the prefix into the submodule command, but also changes\n> into that directory.\n>\n> Say you have the following setup\n>\n> repo/ # a superproject repository\n> repo/untracked/ # an untracked dir in repo/\n> repo/sub/ # a submodule\n> repo/sub/subsub # a submodule of a submodule\n>\n> When in repo/untracked/ and invoking \"git submodule status --recursive\",\n> the recursed instance of the latter version for listing submodules would\n> try to change into the directory repo/sub/untracked, which is a bug.\n> This happens as we cd into the submodule in git-submodule.sh without\n> clearing wt_prefix, which is the assumed relative path inside the working\n> directory.\n>\n> Most times that directory doesn't exist and we error out. Fix this bug\n> by clearing wt_prefix, such that any recursive instances of will assume\n> to operate from the respective root of the respective submodule.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> git-submodule.sh | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 536ba68..6b18a03 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -827,6 +827,7 @@ Maybe you want to use 'update --init'?\")\"\n> (\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> @@ -1159,6 +1160,7 @@ cmd_status()\n> (\n> prefix=\"$displaypath/\"\n> clear_local_git_env\n> + wt_prefix=\n> cd \"$sm_path\" &&\n> eval cmd_status\n> ) ||\n> @@ -1240,6 +1242,7 @@ cmd_sync()\n>\n> if test -n \"$recursive\"\n> then\n> + wt_prefix=\n\nAnd here I think I see why we moved the original prefix code up some.\nThis looks good.\n\n> eval cmd_sync\n> fi\n> )\n> --\n> 2.8.0.rc4.10.g52f3f33\n>\n\nReviewed-by: Jacob Keller \n\nRegards,\nJake\n"},{"id":"281755","messageId":"CA+P7+xrZ+HrcB15nc7LhEe5MnNin7n-PXCYb_iZ2ZwSDg0smHw@mail.gmail.com","threadId":"41820","inReplyTo":"1458862468-12460-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-24T23:44:35Z","receivedAt":"2016-03-24T23:44:35Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller wrote:\n> The usual early machinery of Git is to change the directory to\n> the top level of the working tree and pass the actual path inside\n> the working tree as `prefix` to the command being run.\n> This is the case both for commands written in C (where the\n> prefix is passed into the command in a function parameter) as\n> well as in git-submodule.sh where the setup code runs\n>\n> wt_prefix=$(git rev-parse show-prefix)\n> cd_to_top_level\n>\n> So the prefix passed into the `submodule--helper list` is actually\n> the relative path inside the working tree, but we were not using\n> the standard way of passing it through.\n>\n> Adhere to Gits standard of passing the relative path inside the\n> working tree by passing it via -C.\n>\n> We do not need to pass it for `submodule foreach` as that command\n> doesn't take further arguments ('$@') to operate on a subset of\n> submodules, such that it is irrelevant for listing the submodules.\n> The computation of the displaypath ('Entering ') is done\n> separately there.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nIt is nice to see the format for doing this standardized, and reduce\nextra code in the submodule--helper. I had wondered why we used\n--prefix before.\n\nReviewed-by: Jacob Keller \n\n> builtin/submodule--helper.c | 5 +----\n> git-submodule.sh | 12 ++++++------\n> 2 files changed, 7 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ed764c9..2983783 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -68,14 +68,11 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> struct module_list list = MODULE_LIST_INIT;\n>\n> struct option module_list_options[] = {\n> - OPT_STRING(0, \"prefix\", &prefix,\n> - N_(\"path\"),\n> - N_(\"alternative anchor for relative paths\")),\n> OPT_END()\n> };\n>\n> const char *const git_submodule_helper_usage[] = {\n> - N_(\"git submodule--helper list [--prefix=] [...]\"),\n> + N_(\"git submodule--helper list [...]\"),\n> NULL\n> };\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 6b18a03..1f7ad6e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -407,7 +407,7 @@ cmd_foreach()\n> # command in the subshell (and a recursive call to this function)\n> exec 3<&0\n>\n> - git submodule--helper list --prefix \"$wt_prefix\"|\n> + git submodule--helper list |\n> while read mode sha1 stage sm_path\n> do\n> die_if_unmatched \"$mode\"\n> @@ -467,7 +467,7 @@ cmd_init()\n> shift\n> done\n>\n> - git submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n> + git -C \"$wt_prefix\" submodule--helper list \"$@\" |\n> while read mode sha1 stage sm_path\n> do\n> die_if_unmatched \"$mode\"\n> @@ -549,7 +549,7 @@ cmd_deinit()\n> die \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n> fi\n>\n> - git submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n> + git -C \"$wt_prefix\" submodule--helper list \"$@\" |\n> while read mode sha1 stage sm_path\n> do\n> die_if_unmatched \"$mode\"\n> @@ -683,7 +683,7 @@ cmd_update()\n> fi\n>\n> cloned_modules=\n> - git submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n> + git -C \"$wt_prefix\" submodule--helper list \"$@\" | {\n> err=\n> while read mode sha1 stage sm_path\n> do\n> @@ -1121,7 +1121,7 @@ cmd_status()\n> shift\n> done\n>\n> - git submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n> + git -C \"$wt_prefix\" submodule--helper list \"$@\" |\n> while read mode sha1 stage sm_path\n> do\n> die_if_unmatched \"$mode\"\n> @@ -1199,7 +1199,7 @@ cmd_sync()\n> esac\n> done\n> cd_to_toplevel\n> - git submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n> + git -C \"$wt_prefix\" submodule--helper list \"$@\" |\n> while read mode sha1 stage sm_path\n> do\n> die_if_unmatched \"$mode\"\n> --\n> 2.8.0.rc4.10.g52f3f33\n>\n\nRegards,\nJake\n"},{"id":"281754","messageId":"CAGZ79kaDwEhGB7aVVWnZEOgPCm2_yAprroz6Fn37DvLC8vNbfw@mail.gmail.com","threadId":"41820","inReplyTo":"CA+P7+xqbkK7GKf5=eFVYkBzmrjy9hOsWGTn0W31qWiT-0ZhuPQ@mail.gmail.com","subject":"Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:44:48Z","receivedAt":"2016-03-24T23:44:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 24, 2016 at 4:38 PM, Jacob Keller wrote:\n> On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller wrote:\n>> Recursing into submodules currently works by just calling\n>> (cd $submodule && eval ) for update, sync and status\n>> command.\n>>\n>> Say you have the following setup\n>>\n>> repo/ # a superproject repository\n>> repo/untracked/ # an untracked dir in repo/\n>> repo/sub/ # a submodule\n>> repo/sub/subsub # a submodule of a submodule\n>>\n>> When being in repo/untracked/ and invoking \"git submodule status\"\n>> you would expect output like:\n>>\n>> repo/untracked/$ git submodule status --recursive\n>> ../sub (version)\n>> ../sub/subsub ()\n>>\n>> We need to take into account that we are in the untracked/ dir,\n>> so we need to prepend ../ to the paths. By using relative_path\n>> to compute the prefix, we'll have that output.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> git-submodule.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 43c68de..536ba68 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -825,7 +825,7 @@ 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>> cd \"$sm_path\" &&\n>> eval cmd_update\n>> @@ -1233,13 +1233,13 @@ cmd_sync()\n>> then\n>> (\n>> clear_local_git_env\n>> + prefix=$(relative_path \"$prefix$sm_path/\")\n>\n> Not really sure why this got moved, but I don't think it hurts\n> anything, though we will have prefix defined now regardless of if\n> we're recursive or not. But I think that's correct.\n\n\"Because we need to move it before the cd\". At least I thought so at\nthe time of writing. That is actually not the case.\n\nAt the time of writing this was intertangled with the next patch,\nand we need to put the call to relative_path before the reassignment\nof wt_prefix as relative_path depends on wt_prefix, which I put at the\nsame place as the cd in an initial version when coming up with that patch.\n\nIn case of a resend, consider this fixed.\n\n>\n>> cd \"$sm_path\"\n>> remote=$(get_default_remote)\n>> git config remote.\"$remote\".url \"$sub_origin_url\"\n>>\n>> if test -n \"$recursive\"\n>> then\n>> - prefix=\"$prefix$sm_path/\"\n>> eval cmd_sync\n>> fi\n>> )\n>> --\n>> 2.8.0.rc4.10.g52f3f33\n>>\n>\n> Reviewed-by: Jacob Keller \n>\n> Regards,\n> Jake\n"},{"id":"281758","messageId":"CAPig+cS8NSDb=apvtLHQL-Ph=RB0CTashhZMnQ3DkcLfnzB9ww@mail.gmail.com","threadId":"41820","inReplyTo":"1458862468-12460-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/4] submodule: add more tests for recursive submodule behavior","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T00:25:34Z","receivedAt":"2016-03-25T00:25:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller wrote:\n> This adds a test for \"submodule update\", wich calls \"submodule update\"\n\ns/wich/which/\n\n> from an untracked repository in the superproject. When doing creating\n\nGrammo: \"doing creating\"\n\n> the parent patch a similar test failed for \"submodule sync\", but\n> all tests passed for \"submodule update\". It took me a long time\n> to figure out this was a difference in test coverage instead of\n> commands behaving differently. Let's improve the test coverage such\n> to make it a better place.\n>\n> When trying to fix the issue in the parent patch I could get\n> the test suite passing when removing the $@ argument from module_list\n> in the sync command. This also indicates a low test coverage, so\n> fix that.\n\nThese two paragraphs are almost entirely commentary, thus probably\nbelong below the \"---\" line. I'm having a difficult time trying to\ndecipher from this commit message what this patch is actually about.\nPerhaps the commit message could do a better job explaining exactly\nwhat shortcoming(s) it's addressing.\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> @@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n> +test_expect_success 'submodule update --recursive works from subdirectory' '\n> + (cd super2 &&\n> + (cd deeper/submodule/subsubmodule &&\n> + git checkout HEAD^\n> + ) &&\n\nMaybe use -C and drop the sub-subshell:\n\n git -C deeper/submodule/subsubmodule checkout HEAD^\n\n> + mkdir untracked &&\n> + cd untracked &&\n> + git submodule update --recursive >actual &&\n> + test_i18ngrep \"Submodule path .../deeper/submodule/subsubmodule.: checked out\" actual\n> + )\n> +'\n"},{"id":"281759","messageId":"CAGZ79ka9eRbqCEhE3FXpg+i6XJ03azaHYzUeJnOZGHZf_dJH=Q@mail.gmail.com","threadId":"41820","inReplyTo":"CAPig+cS8NSDb=apvtLHQL-Ph=RB0CTashhZMnQ3DkcLfnzB9ww@mail.gmail.com","subject":"Re: [PATCH 4/4] submodule: add more tests for recursive submodule behavior","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T00:33:42Z","receivedAt":"2016-03-25T00:33:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 24, 2016 at 5:25 PM, Eric Sunshine wrote:\n> On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller wrote:\n>> This adds a test for \"submodule update\", wich calls \"submodule update\"\n>\n> s/wich/which/\n>\n>> from an untracked repository in the superproject. When doing creating\n>\n> Grammo: \"doing creating\"\n>\n>> the parent patch a similar test failed for \"submodule sync\", but\n>> all tests passed for \"submodule update\". It took me a long time\n>> to figure out this was a difference in test coverage instead of\n>> commands behaving differently. Let's improve the test coverage such\n>> to make it a better place.\n>>\n>> When trying to fix the issue in the parent patch I could get\n>> the test suite passing when removing the $@ argument from module_list\n>> in the sync command. This also indicates a low test coverage, so\n>> fix that.\n>\n> These two paragraphs are almost entirely commentary, thus probably\n> belong below the \"---\" line. I'm having a difficult time trying to\n> decipher from this commit message what this patch is actually about.\n> Perhaps the commit message could do a better job explaining exactly\n> what shortcoming(s) it's addressing.\n\nThe tests on submodule commands executed not from the top level are very sparse.\nI had a hard time to developing patches 1 and 2.\nAnd I felt\n\n \"This patch adds more test coverage.\"\n\nis not a sufficient commit message.\n\nThe current tests have found issues, which\nlead to fixing them in patch 1,2, but I think only by accident, as one command\n(sync) was testing from the non root. By having more test coverage it\nis easier to\nhave a guess what is wrong with the code.\n\nAny hint on how to write that into a commit message without being commentatory\nwould be great!\n\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> @@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n>> +test_expect_success 'submodule update --recursive works from subdirectory' '\n>> + (cd super2 &&\n>> + (cd deeper/submodule/subsubmodule &&\n>> + git checkout HEAD^\n>> + ) &&\n>\n> Maybe use -C and drop the sub-subshell:\n>\n> git -C deeper/submodule/subsubmodule checkout HEAD^\n\nok\n\n>\n>> + mkdir untracked &&\n>> + cd untracked &&\n>> + git submodule update --recursive >actual &&\n>> + test_i18ngrep \"Submodule path .../deeper/submodule/subsubmodule.: checked out\" actual\n>> + )\n>> +'\n"},{"id":"281777","messageId":"xmqqmvpn5awo.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"1458862468-12460-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T06:28:23Z","receivedAt":"2016-03-25T06:28:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The usual early machinery of Git is to change the directory to\n> the top level of the working tree and pass the actual path inside\n> the working tree as `prefix` to the command being run.\n>\n> This is the case both for commands written in C (where the\n> prefix is passed into the command in a function parameter) as\n> well as in git-submodule.sh where the setup code runs...\n>\n> Adhere to Gits standard of passing the relative path inside the\n> working tree by passing it via -C.\n\nWhile -C _also_ works, I do not think it is \"standard\" in any sense.\nWhat made you think so? -C is a way to tell Git to chdir there\nbefore doing anything else, without even adjusting the command line\narguments that might be paths, and \"chdir there to go to top\" may\n(or may not--I haven't thought things thru) have the same effect as\npassing the prefix into functions, that is merely true as a side\neffect, I would think.\n\nSo this change may not be wrong per-se, but if the lossage of prefix\nis the final goal (as opposed to an approach to gain other benefits,\ne.g. \"now we do not have to use prefix, we can simplify these other\nthings\"), I do not know if it is worth it.\n"},{"id":"281821","messageId":"xmqqegay5yw0.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"xmqqmvpn5awo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T16:02:39Z","receivedAt":"2016-03-25T16:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> The usual early machinery of Git is to change the directory to\n>> the top level of the working tree and pass the actual path inside\n>> the working tree as `prefix` to the command being run.\n>>\n>> This is the case both for commands written in C (where the\n>> prefix is passed into the command in a function parameter) as\n>> well as in git-submodule.sh where the setup code runs...\n>>\n>> Adhere to Gits standard of passing the relative path inside the\n>> working tree by passing it via -C.\n>\n> While -C _also_ works, I do not think it is \"standard\" in any sense.\n> What made you think so? -C is a way to tell Git to chdir there\n> before doing anything else, without even adjusting the command line\n> arguments that might be paths, and \"chdir there to go to top\" may\n> (or may not--I haven't thought things thru) have the same effect as\n> passing the prefix into functions, that is merely true as a side\n> effect, I would think.\n>\n> So this change may not be wrong per-se, but if the lossage of prefix\n> is the final goal (as opposed to an approach to gain other benefits,\n> e.g. \"now we do not have to use prefix, we can simplify these other\n> things\"), I do not know if it is worth it.\n\nI actually do not care too deeply either way, as I understand that\nthe long term goal is to have \"git submodule\" itself rewritten in C,\nso that places that currently call submodule--helper would become an\ninternal call.\n\nThe way all the subcommand written in C works is\n\n - The start-up sequence does the repository discovery, which\n involves crawling up to the top-level of the working tree, and\n compute \"prefix\", where the end-user was when the command was\n invoked;\n\n - The subcommand implementation is called with this \"prefix\";\n\n - When the subcommand implementation interprets the command line\n arguments and option arguments, it prefixes the \"prefix\" as\n needed. If, for example, \"git grep -f patterns\" is invoked\n inside \"sub/\" subdirectory, when the command line and option\n arguments are processed, the process is already at the top level\n of the working tree, so it needs to read the patterns from\n \"sub/patterns\" file. \"git ls-files 'Makefil*'\" invoked inside\n \"sub/\" subdirectory needs to limit the output to those that match\n not \"Makefile\", but \"sub/Makefil*\".\n\nThe hope of doing an incremental rewrite of the whole thing by\nenriching submodule--helper is that the bulk of the code there will\nbe reusable when the entirety of \"git submodule\" is rewritten in C,\nso they need to take the \"prefix\" the same way, whether the caller\nis calling from \"git-submodule.sh\" script via submodule--helper, or\nthe eventual C implementation of \"git submodule\" is making direct\ncalls to them. As long as the correct \"prefix\" is passed to the\nroutines that are driven via submodule--helper, it does not matter\nand I do not care how it is done.\n\nThe current code of \"git submodule\" whose higher parts are still in\nshell would would:\n\n - The start-up sequence in shell does the cd_to_toplevel and finds\n the prefix;\n\n - \"git submodule--helper list --prefix=$prefix $other_args\" is\n called; as this is called from the top-level of the working tree,\n internally its \"prefix\" is empty, but $other_args must be\n interpreted relative to the $prefix passed with --prefix option.\n\nIf we instead call \"git -C \"$prefix\" submodule--helper list $other_args\",\nthen\n\n - This command first does another chdir(2) back to $prefix;\n\n - The start-up sequence of \"submodule--helper\" does the usual\n repository discovery again, which involves crawling up to the\n top-level of the working tree, and compute \"prefix\". This\n happens to match what -C \"$prefix\" gave the command.\n\nMaking calls to submodule--helper via \"git -C\" feels a little bit\nroundabout because of this \"caller tells to chdir, and then it has\nto again chdir back up\" only to rediscover the same information.\n\nAgain, I actually do not care too deeply either way, though, as long\nas the correct \"prefix\" is passed to the routines that are driven\nvia submodule--helper, which would assure that the C code you write\ntoday will be reusable when \"git submodule\" as a whole is redone in\nC.\n\nThanks.\n"},{"id":"281825","messageId":"xmqqy4964if7.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"1458862468-12460-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T16:43:40Z","receivedAt":"2016-03-25T16:43:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Recursing into submodules currently works by just calling\n> (cd $submodule && eval ) for update, sync and status\n> command.\n>\n> Say you have the following setup\n>\n> repo/ # a superproject repository\n> repo/untracked/ # an untracked dir in repo/\n> repo/sub/ # a submodule\n> repo/sub/subsub # a submodule of a submodule\n>\n> When being in repo/untracked/ and invoking \"git submodule status\"\n> you would expect output like:\n>\n> repo/untracked/$ git submodule status --recursive\n> ../sub (version)\n> ../sub/subsub ()\n>\n> We need to take into account that we are in the untracked/ dir,\n> so we need to prepend ../ to the paths. By using relative_path\n> to compute the prefix, we'll have that output.\n\ntests to demonstrate existing breakage and protect the fix from\nfuture breakages would be needed, no?\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> git-submodule.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43c68de..536ba68 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -825,7 +825,7 @@ 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\tcd \"$sm_path\" &&\n> \t\t\t\teval cmd_update\n> @@ -1233,13 +1233,13 @@ cmd_sync()\n> \t\t\tthen\n> \t\t\t(\n> \t\t\t\tclear_local_git_env\n> +\t\t\t\tprefix=$(relative_path \"$prefix$sm_path/\")\n> \t\t\t\tcd \"$sm_path\"\n> \t\t\t\tremote=$(get_default_remote)\n> \t\t\t\tgit config remote.\"$remote\".url \"$sub_origin_url\"\n> \n> \t\t\t\tif test -n \"$recursive\"\n> \t\t\t\tthen\n> -\t\t\t\t\tprefix=\"$prefix$sm_path/\"\n> \t\t\t\t\teval cmd_sync\n> \t\t\t\tfi\n> \t\t\t)\n"},{"id":"281826","messageId":"xmqqtwju4iaj.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"1458862468-12460-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: fix recursive execution from non root directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T16:46:28Z","receivedAt":"2016-03-25T16:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Most times that directory doesn't exist and we error out. Fix this bug\n> by clearing wt_prefix, such that any recursive instances of will assume\n> to operate from the respective root of the respective submodule.\n\nAs long as the recursive instances do not take any filenames and\npathspecs that needs adjustment with respect to the prefix, this\nwould be an OK change; I am not sure if that precondition holds,\nthough.\n\nThanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> git-submodule.sh | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 536ba68..6b18a03 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -827,6 +827,7 @@ Maybe you want to use 'update --init'?\")\"\n> \t\t\t(\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> @@ -1159,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) ||\n> @@ -1240,6 +1242,7 @@ cmd_sync()\n> \n> \t\t\t\tif test -n \"$recursive\"\n> \t\t\t\tthen\n> +\t\t\t\t\twt_prefix=\n> \t\t\t\t\teval cmd_sync\n> \t\t\t\tfi\n> \t\t\t)\n"},{"id":"281827","messageId":"CAGZ79kZ684W46df9zPQATr3zWKt+e1BhGY6DZ84psfXWH4tGNw@mail.gmail.com","threadId":"41820","inReplyTo":"xmqqmvpn5awo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T16:49:56Z","receivedAt":"2016-03-25T16:49:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 24, 2016 at 11:28 PM, Junio C Hamano wrote:\n>\n> So this change may not be wrong per-se, but if the lossage of prefix\n> is the final goal (as opposed to an approach to gain other benefits,\n> e.g. \"now we do not have to use prefix, we can simplify these other\n> things\"), I do not know if it is worth it.\n>\n\nIt is the final goal of this series. As motivation, see Jacobs comment above:\n\n I had wondered why we used --prefix before.\n\nAlso when the submodule helper got in, reviewers (Jonathan) were confused and\nasked for clarification of the prefix term. So either document the prefix term\n(and come up with a reason why we don't use the standard mechanism,\nwhich as you outlined in the other mail, may be performance as we skip one\nchdir, but that sounds like weak argument to me) or remove the confusing part\nof having a prefix, which is not the standard prefix inside C.\n\nThe other reason you gave below is also convincing: By having it in the prefix,\nthe C code is more likely correct and future proof.\n\nOn rewriting the whole submodule command in C (probably reiterating):\nIt is not my endgoal to rewrite every submodule related part in C. It\nwould be nice\nif it would happen eventually, but for now I only rewrite parts that I\nneed in C. (i.e.\nparalllelisation is hard in shell, if not impossible using posix shell\nwith no additional\ndependencies [xargs, gnu parallel], so I moved that part to C.\nCertain parts need a performance boost? Ok I'll do it in C.\n\nThat said, we may have the shell/C architecture for a longer time than planned,\nwhich makes it important to comment/document the confusing parts. Instead\nI'd rather not have confusing parts, so I see benefit in having the\ngoal of this series\nto remove the --prefix.\n"},{"id":"281829","messageId":"CAGZ79kYmVH_6=7y894woObaD4de44d8iUQtcCq6r3grGrMiaxQ@mail.gmail.com","threadId":"41820","inReplyTo":"xmqqy4964if7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T16:54:11Z","receivedAt":"2016-03-25T16:54:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 9:43 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Recursing into submodules currently works by just calling\n>> (cd $submodule && eval ) for update, sync and status\n>> command.\n>>\n>> Say you have the following setup\n>>\n>> repo/ # a superproject repository\n>> repo/untracked/ # an untracked dir in repo/\n>> repo/sub/ # a submodule\n>> repo/sub/subsub # a submodule of a submodule\n>>\n>> When being in repo/untracked/ and invoking \"git submodule status\"\n>> you would expect output like:\n>>\n>> repo/untracked/$ git submodule status --recursive\n>> ../sub (version)\n>> ../sub/subsub ()\n>>\n>> We need to take into account that we are in the untracked/ dir,\n>> so we need to prepend ../ to the paths. By using relative_path\n>> to compute the prefix, we'll have that output.\n>\n> tests to demonstrate existing breakage and protect the fix from\n> future breakages would be needed, no?\n\nI realize this is worded slightly wrong. The bugs described in patch\n1&2 would only occur if we'd apply patch 3 without them.\nSo currently there is no bug.\n\nWe currently do have tests for that, e.g. t7403,\n '\"git submodule sync\" should update submodule URLs - subdirectory'\n\nIn patch 4 I added tests for \"git submodule update\" and enhanced tests for\nthe sync command.\n\n\n>\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> git-submodule.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 43c68de..536ba68 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -825,7 +825,7 @@ 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>> cd \"$sm_path\" &&\n>> eval cmd_update\n>> @@ -1233,13 +1233,13 @@ cmd_sync()\n>> then\n>> (\n>> clear_local_git_env\n>> + prefix=$(relative_path \"$prefix$sm_path/\")\n>> cd \"$sm_path\"\n>> remote=$(get_default_remote)\n>> git config remote.\"$remote\".url \"$sub_origin_url\"\n>>\n>> if test -n \"$recursive\"\n>> then\n>> - prefix=\"$prefix$sm_path/\"\n>> eval cmd_sync\n>> fi\n>> )\n"},{"id":"281831","messageId":"xmqqlh564hm3.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"CAGZ79kZ684W46df9zPQATr3zWKt+e1BhGY6DZ84psfXWH4tGNw@mail.gmail.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T17:01:08Z","receivedAt":"2016-03-25T17:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The other reason you gave below is also convincing: By having it in the prefix,\n> the C code is more likely correct and future proof.\n>\n> On rewriting the whole submodule command in C (probably\n> reiterating): It is not my endgoal to rewrite every submodule\n> related part in C. It would be nice if it would happen eventually,\n> but for now I only rewrite parts that I need in C.\n\nWell, what you personally would want to do yourself is irrelevant.\nDoing submodule--helper in such a way that it is sufficient in a\nhalf-way conversion but cannot be reused in future full rewrite is\nsomething we would want to avoid, whether you would be doing the\nfull rewrite in the future. In fact, if you are not inclined to do\nso yourself, that is one more reason to make sure that the C pieces\nin submodule--helper are reusable (i.e. your \"correct and future\nproof\" above); otherwise you'd be making it _more_ difficult for\nother people who would want to pick it up where you left.\n"},{"id":"281833","messageId":"CAGZ79kZ=de7jh7b2OnTuEoTUSJKhoNkjb+Mw41JP7w9SVWH3iA@mail.gmail.com","threadId":"41820","inReplyTo":"xmqqlh564hm3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T17:05:55Z","receivedAt":"2016-03-25T17:05:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 10:01 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The other reason you gave below is also convincing: By having it in the prefix,\n>> the C code is more likely correct and future proof.\n>>\n>> On rewriting the whole submodule command in C (probably\n>> reiterating): It is not my endgoal to rewrite every submodule\n>> related part in C. It would be nice if it would happen eventually,\n>> but for now I only rewrite parts that I need in C.\n>\n> Well, what you personally would want to do yourself is irrelevant.\n> Doing submodule--helper in such a way that it is sufficient in a\n> half-way conversion but cannot be reused in future full rewrite is\n> something we would want to avoid, whether you would be doing the\n> full rewrite in the future. In fact, if you are not inclined to do\n> so yourself, that is one more reason to make sure that the C pieces\n> in submodule--helper are reusable (i.e. your \"correct and future\n> proof\" above); otherwise you'd be making it _more_ difficult for\n> other people who would want to pick it up where you left.\n\nExactly, sorry for not writing my chain of thoughts down completely.\n\nTo make them reusable, I'd assume we want them to be easy to\nunderstand, and by using a well known way in git it is easier to\nunderstand.\n"},{"id":"281834","messageId":"xmqqh9fu4gi7.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"xmqqegay5yw0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T17:25:04Z","receivedAt":"2016-03-25T17:25:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The way all the subcommand written in C works is\n>\n> - The start-up sequence does the repository discovery, which\n> involves crawling up to the top-level of the working tree, and\n> compute \"prefix\", where the end-user was when the command was\n> invoked;\n>\n> - The subcommand implementation is called with this \"prefix\";\n>\n> - When the subcommand implementation interprets the command line\n> arguments and option arguments, it prefixes the \"prefix\" as\n> needed. If, for example, \"git grep -f patterns\" is invoked\n> inside \"sub/\" subdirectory, when the command line and option\n> arguments are processed, the process is already at the top level\n> of the working tree, so it needs to read the patterns from\n> \"sub/patterns\" file. \"git ls-files 'Makefil*'\" invoked inside\n> \"sub/\" subdirectory needs to limit the output to those that match\n> not \"Makefile\", but \"sub/Makefil*\".\n>\n> The hope of doing an incremental rewrite of the whole thing by\n> enriching submodule--helper is that the bulk of the code there will\n> be reusable when the entirety of \"git submodule\" is rewritten in C,\n> so they need to take the \"prefix\" the same way, whether the caller\n> is calling from \"git-submodule.sh\" script via submodule--helper, or\n> the eventual C implementation of \"git submodule\" is making direct\n> calls to them. As long as the correct \"prefix\" is passed to the\n> routines that are driven via submodule--helper, it does not matter\n> and I do not care how it is done.\n>\n> The current code of \"git submodule\" whose higher parts are still in\n> shell would would:\n>\n> - The start-up sequence in shell does the cd_to_toplevel and finds\n> the prefix;\n>\n> - \"git submodule--helper list --prefix=$prefix $other_args\" is\n> called; as this is called from the top-level of the working tree,\n> internally its \"prefix\" is empty, but $other_args must be\n> interpreted relative to the $prefix passed with --prefix option.\n>\n> If we instead call \"git -C \"$prefix\" submodule--helper list $other_args\",\n> then\n>\n> - This command first does another chdir(2) back to $prefix;\n>\n> - The start-up sequence of \"submodule--helper\" does the usual\n> repository discovery again, which involves crawling up to the\n> top-level of the working tree, and compute \"prefix\". This\n> happens to match what -C \"$prefix\" gave the command.\n>\n> Making calls to submodule--helper via \"git -C\" feels a little bit\n> roundabout because of this \"caller tells to chdir, and then it has\n> to again chdir back up\" only to rediscover the same information.\n\nJust to make sure that the discussion is complete.\n\nAnother way a script can use the \"prefix\" information is to use the\n\"prefix\" as such. Knowing that the cd_to_toplevel() took you to the\nroot level of the working tree, instead of \"git -C $prefix\" or\n\"--prefix $prefix\", it could do \"git $cmd $prefix$filename\".\n\nOne consideration when choosing among the approaches is how the\n$filename is reported back to the user (e.g. as part of an error\nmessage). \"git $cmd $prefix$filename\" invocation would complain\nabout the full \"$prefix$filename\" path, but what the user gave it\nmay only be $filename part.\n"},{"id":"281835","messageId":"CAGZ79kYGbdAonKwTcQACFRKKZ-6GJRVsXHu7nO9fXaUNoToP-w@mail.gmail.com","threadId":"41820","inReplyTo":"xmqqtwju4iaj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] submodule: fix recursive execution from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T17:27:02Z","receivedAt":"2016-03-25T17:27:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 9:46 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Most times that directory doesn't exist and we error out. Fix this bug\n>> by clearing wt_prefix, such that any recursive instances of will assume\n>> to operate from the respective root of the respective submodule.\n>\n> As long as the recursive instances do not take any filenames and\n> pathspecs that needs adjustment with respect to the prefix, this\n> would be an OK change; I am not sure if that precondition holds,\n> though.\n\nIf we give filenames and pathspecs relative to the prefix in a submodule,\nthe filename is not recognized by the superproject which would produce an\nerror at the superproject. Monkey-testing locally here supports my suspicion\nand it is not possible to do things like\n\n git submodule status path/inside/submodule\n\nso if that is already not working, we should not expect to have\n\n git -C subdir submodule status ../path/inside/submodule\n\nworking.\n\n\n\n\n\n>\n> Thanks.\n>\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> git-submodule.sh | 3 +++\n>> 1 file changed, 3 insertions(+)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 536ba68..6b18a03 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -827,6 +827,7 @@ Maybe you want to use 'update --init'?\")\"\n>> (\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>> @@ -1159,6 +1160,7 @@ cmd_status()\n>> (\n>> prefix=\"$displaypath/\"\n>> clear_local_git_env\n>> + wt_prefix=\n>> cd \"$sm_path\" &&\n>> eval cmd_status\n>> ) ||\n>> @@ -1240,6 +1242,7 @@ cmd_sync()\n>>\n>> if test -n \"$recursive\"\n>> then\n>> + wt_prefix=\n>> eval cmd_sync\n>> fi\n>> )\n"},{"id":"281836","messageId":"CAGZ79kY96cx9iED=xbheByO7kzNFT2VkO6VpRX6nVJ6rov+6uw@mail.gmail.com","threadId":"41820","inReplyTo":"xmqqh9fu4gi7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T17:31:32Z","receivedAt":"2016-03-25T17:31:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 10:25 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> The way all the subcommand written in C works is\n>>\n>> - The start-up sequence does the repository discovery, which\n>> involves crawling up to the top-level of the working tree, and\n>> compute \"prefix\", where the end-user was when the command was\n>> invoked;\n>>\n>> - The subcommand implementation is called with this \"prefix\";\n>>\n>> - When the subcommand implementation interprets the command line\n>> arguments and option arguments, it prefixes the \"prefix\" as\n>> needed. If, for example, \"git grep -f patterns\" is invoked\n>> inside \"sub/\" subdirectory, when the command line and option\n>> arguments are processed, the process is already at the top level\n>> of the working tree, so it needs to read the patterns from\n>> \"sub/patterns\" file. \"git ls-files 'Makefil*'\" invoked inside\n>> \"sub/\" subdirectory needs to limit the output to those that match\n>> not \"Makefile\", but \"sub/Makefil*\".\n>>\n>> The hope of doing an incremental rewrite of the whole thing by\n>> enriching submodule--helper is that the bulk of the code there will\n>> be reusable when the entirety of \"git submodule\" is rewritten in C,\n>> so they need to take the \"prefix\" the same way, whether the caller\n>> is calling from \"git-submodule.sh\" script via submodule--helper, or\n>> the eventual C implementation of \"git submodule\" is making direct\n>> calls to them. As long as the correct \"prefix\" is passed to the\n>> routines that are driven via submodule--helper, it does not matter\n>> and I do not care how it is done.\n>>\n>> The current code of \"git submodule\" whose higher parts are still in\n>> shell would would:\n>>\n>> - The start-up sequence in shell does the cd_to_toplevel and finds\n>> the prefix;\n>>\n>> - \"git submodule--helper list --prefix=$prefix $other_args\" is\n>> called; as this is called from the top-level of the working tree,\n>> internally its \"prefix\" is empty, but $other_args must be\n>> interpreted relative to the $prefix passed with --prefix option.\n>>\n>> If we instead call \"git -C \"$prefix\" submodule--helper list $other_args\",\n>> then\n>>\n>> - This command first does another chdir(2) back to $prefix;\n>>\n>> - The start-up sequence of \"submodule--helper\" does the usual\n>> repository discovery again, which involves crawling up to the\n>> top-level of the working tree, and compute \"prefix\". This\n>> happens to match what -C \"$prefix\" gave the command.\n>>\n>> Making calls to submodule--helper via \"git -C\" feels a little bit\n>> roundabout because of this \"caller tells to chdir, and then it has\n>> to again chdir back up\" only to rediscover the same information.\n>\n> Just to make sure that the discussion is complete.\n>\n> Another way a script can use the \"prefix\" information is to use the\n> \"prefix\" as such. Knowing that the cd_to_toplevel() took you to the\n> root level of the working tree, instead of \"git -C $prefix\" or\n> \"--prefix $prefix\", it could do \"git $cmd $prefix$filename\".\n>\n> One consideration when choosing among the approaches is how the\n> $filename is reported back to the user (e.g. as part of an error\n> message). \"git $cmd $prefix$filename\" invocation would complain\n> about the full \"$prefix$filename\" path, but what the user gave it\n> may only be $filename part.\n\nRight. Using either \"git -C $prefix\" or \"git --prefix $prefix\" would report\nthe $filename only, because both cases assume $prefix was cut\nusing cd_to_toplevel and the user expects $filename only.\n"},{"id":"281859","messageId":"xmqqshze2ysx.fsf@gitster.mtv.corp.google.com","threadId":"41820","inReplyTo":"CAGZ79kZ=de7jh7b2OnTuEoTUSJKhoNkjb+Mw41JP7w9SVWH3iA@mail.gmail.com","subject":"Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T18:32:46Z","receivedAt":"2016-03-25T18:32:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Exactly, sorry for not writing my chain of thoughts down completely.\n>\n> To make them reusable, I'd assume we want them to be easy to\n> understand, and by using a well known way in git it is easier to\n> understand.\n\nI already said I do not care too deeply either way, but I have to\npoint out that \"git -C $prefix\" would put extra cognitive burden on\nthose who will be picking it up where you left off. When they start\nby first literally translating shell to C, \"git -C\" would mislead\nthem to think if they have to chdir(2) to the directory first, which\nis not the case at all. \"git -C $prefix submodule--helper\" in the\ncode after [3/4] is there only because submodule--helper code no\nlonger is told what the caller already knows (i.e. what is the\nprefix) and is forced to find it out by itself. Contrasting to\nthat, with an explicit --prefix option, the reader would know there\nis no need to chdir(2) anywhere at that point, but the paths that\nare held in variables it uses from the surrounding code are not\nrelative to the current working directory when the code is called\nand $prefix is there to help adjusting it.\n\nThe reason I do not care too deeply was that I thought people who\nwill be taking over after you are done (well, after all that might\ninclude you; plans would change) are clueful, but there is no need\nfor us to make their life more difficult than necessary.\n"}]}