{"thread":{"id":"41829","subject":"[PATCHv3 3/5] submodule update: add test for recursive from non root dir","startedAt":"2016-03-25T18:39:11Z","lastAt":"2016-03-28T16:56:36Z","messageCount":15,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"281862","messageId":"1458931156-29125-1-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":null,"subject":"[PATCHv3 0/5] submodule helper: cleanup prefix passing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:11Z","receivedAt":"2016-03-25T18:39:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks Junio for the in-depth discussion!\nI squashed the first two commits as they are actually fixing the same underlying\nissue. Setting wt_prefix=<empty> is the actual preparation for switching \nthe submodule--helper to git -C instead of \"--prefix$prefix\".\nBut setting wt_prefix=empty would break existing tests, so the different\ncomputation of the prefix (relative_path of sm_path instead of plain sm_path)\nis the fix of the fix.\n\nEric, I picked up the git -C in the tests, which are broken into 2 patches and\nhave a more concise commit message each.\n\nJacob, thanks for your \n  Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>\nalthough I changed the code so much, that I did not put it in this series.\n\nNew in this series is the last commit, which gets rid of the prefix for\n\"submodule--helper clone\". (It was useless to begin with; probably just\ncargo-culted in because we had it in \"submodule--helper list\")\n\nThanks,\nStefan\n\nStefan Beller (5):\n  submodule: prepare recursive path from non root directory\n  submodule--helper list: lose the extra prefix option\n  submodule update: add test for recursive from non root dir\n  submodule sync: test syncing one submodule\n  submodule--helper clone: lose the extra prefix option\n\n builtin/submodule--helper.c | 10 ++--------\n git-submodule.sh            | 23 +++++++++++++----------\n t/t7403-submodule-sync.sh   | 13 +++++++++----\n t/t7406-submodule-update.sh | 14 +++++++++++---\n 4 files changed, 35 insertions(+), 25 deletions(-)\n\n-- \n2.8.0.rc4.10.g52f3f33.dirty\n"},{"id":"281864","messageId":"1458931156-29125-2-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":"1458931156-29125-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 1/5] submodule: prepare recursive path from non root directory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:12Z","receivedAt":"2016-03-25T18:39:12Z","isPatch":false,"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\". A 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 to later as it is easier to understand for\nreaders of the code base. That way however does not just pass the\nprefix into the submodule command, but also changes into that\ndirectory. 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\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 potential\nbug by clearing wt_prefix, such that any recursive instances of will assume\nto operate from the root of the respective submodule.\n\nWhen changing wt_prefix, we need to make sure the reported paths stay\ncorrect, as paths are reported relative to the wt_prefix. We can fix\nthis by computing the relative path before changing the wt_prefix.\n\nAs we do not pass in filenames or pathspecs into submodules recursively,\nwe do not need to care about filenames being relative to wt_prefix,\nso fixing the display path for error reporting is the only thing we\nneed to do.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43c68de..d83608c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -825,8 +825,9 @@ Maybe you want to use 'update --init'?\")\"\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n \t\t\t\tclear_local_git_env\n+\t\t\t\twt_prefix=\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_update\n \t\t\t)\n@@ -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@@ -1239,7 +1241,8 @@ cmd_sync()\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\tprefix=$(relative_path \"$prefix$sm_path/\")\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.dirty\n"},{"id":"281865","messageId":"1458931156-29125-3-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":"1458931156-29125-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 2/5] submodule--helper list: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:13Z","receivedAt":"2016-03-25T18:39:13Z","isPatch":false,"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. This is more readable for future\nreaders of the code.\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 <path>') is done\nseparately there.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\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 5295b72..6c3ff91 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=<path>] [<path>...]\"),\n+\t\tN_(\"git submodule--helper list [<path>...]\"),\n \t\tNULL\n \t};\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d83608c..2dd1b18 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.dirty\n"},{"id":"281861","messageId":"1458931156-29125-4-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":"1458931156-29125-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 3/5] submodule update: add test for recursive from non root dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:14Z","receivedAt":"2016-03-25T18:39:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Test \"submodule update --recursive\" being invoked from a\nsub directory as this is currently not covered by the test\nsuite.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7406-submodule-update.sh | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 68ea31d..19d8552 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -767,11 +767,19 @@ test_expect_success 'submodule update clone shallow submodule' '\n \n test_expect_success 'submodule update --recursive drops module name before recursing' '\n \t(cd super2 &&\n-\t (cd deeper/submodule/subsubmodule &&\n-\t  git checkout HEAD^\n-\t ) &&\n+\t git -C deeper/submodule/subsubmodule checkout HEAD^\n \t git submodule update --recursive deeper/submodule >actual &&\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 git -C deeper/submodule/subsubmodule checkout HEAD^\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.dirty\n"},{"id":"281863","messageId":"1458931156-29125-5-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":"1458931156-29125-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 4/5] submodule sync: test syncing one submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:15Z","receivedAt":"2016-03-25T18:39:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The current test suite doesn't cover syncing one module only.\nInstead of adding a new test, we can change existing tests to cover this\npart as well.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7403-submodule-sync.sh | 13 +++++++++----\n 1 file changed, 9 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\n-- \n2.8.0.rc4.10.g52f3f33.dirty\n"},{"id":"281866","messageId":"1458931156-29125-6-git-send-email-sbeller@google.com","threadId":"41829","inReplyTo":"1458931156-29125-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T18:39:16Z","receivedAt":"2016-03-25T18:39:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the rewrite from shell to C (ee8838d157761, 2015-09-08, submodule:\nrewrite `module_clone` shell function in C), we never made use of the\nprefix. Probably it sneaked in as module_list which was converted in the\nsame series had the prefix as well.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 5 +----\n git-submodule.sh            | 4 ++--\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6c3ff91..73b5874 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -153,9 +153,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstruct option module_clone_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_STRING(0, \"path\", &path,\n \t\t\t   N_(\"path\"),\n \t\t\t   N_(\"where the new submodule will be cloned to\")),\n@@ -176,7 +173,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper clone [--prefix=<path>] [--quiet] \"\n+\t\tN_(\"git submodule--helper clone [--quiet] \"\n \t\t   \"[--reference <repository>] [--name <name>] [--url <url>]\"\n \t\t   \"[--depth <depth>] [--] [<path>...]\"),\n \t\tNULL\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2dd1b18..93beada 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -347,7 +347,7 @@ Use -f if you really want to add it.\" >&2\n \t\t\t\techo \"$(eval_gettext \"Reactivating local git directory for submodule '\\$sm_name'.\")\"\n \t\t\tfi\n \t\tfi\n-\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n+\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n \t\t(\n \t\t\tclear_local_git_env\n \t\t\tcd \"$sm_path\" &&\n@@ -727,7 +727,7 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n-- \n2.8.0.rc4.10.g52f3f33.dirty\n"},{"id":"281871","messageId":"xmqqk2kq2wkj.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"1458931156-29125-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 1/5] submodule: prepare recursive path from non root directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T19:21:00Z","receivedAt":"2016-03-25T19:21:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43c68de..d83608c 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -825,8 +825,9 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\tif test -n \"$recursive\"\n>  \t\tthen\n>  \t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> +\t\t\t\tprefix=\"$(relative_path $prefix$sm_path)/\"\n\nDon't you need to protect yourself from IFS chars in $prefix and\n$sm_path here (the other invocation of relative_path in this patch\ndoes not have this issue, I would think).\n\nAs I think about it more, I am more inclined to say \"-C $prefix\" is\ngoing in a wrong direction, so justifying this change solely on that\nmove is somewhat sad.\n\n - Making things relative should not hurt.\n\n - Clearing wt_prefix feels like the right thing, because we are\n   moving to the root of the working tree of a different repository\n   and wt_prefix that tells us where the user originally was in the\n   superproject is not useful in the context of the submodule, which\n   after all is a separate project.  The user however cannot refer\n   to things on the filesystem (including use of pathspecs), as they\n   are taken relative to the root level of each submodule by\n   clearing wt_prefix, though.  i.e.\n\n       $ git submodule $cmd --read-from-this-file=../m/file\n\n   that is started from t/ subdirectory of the superproject that\n   recurses into the submodule at m/ (sitting next to t/) should be\n   able to read from \"file\" sitting at the root level of the working\n   tree of submodule m/ by stripping ../m/ (and relative-path should\n   be able to help with that), but that may become impossible\n   because the fact that the user named ../m/file relative to t/\n   subdirectory is lost by clearing wt_prefix that used to hold t/\n   here.\n\nis the closest justification I can come to, which is weak (\"should\nnot hurt\" does not justify, and \"users cannot ever do this without\nundoing this change\" does not, either).\n\nI think the worst part of this change is that the log message does\nnot make it clear why it is OK not to clear wt_prefix and not to use\nrelative_path if you use --prefix, while the \"-C $prefix\" approach\nhas problems without these changes.  Without that explanation, these\nsymptoms, i.e. the fact that you need the changes in this patch,\nonly smells like an indication that \"-C $prefix\" approach is somehow\nflawed.  I cannot quite pinpoint what that \"somehow\" is, though.\n\n>  \t\t\t\tclear_local_git_env\n> +\t\t\t\twt_prefix=\n>  \t\t\t\tcd \"$sm_path\" &&\n>  \t\t\t\teval cmd_update\n>  \t\t\t)\n> @@ -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> @@ -1239,7 +1241,8 @@ cmd_sync()\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\tprefix=$(relative_path \"$prefix$sm_path/\")\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":"281873","messageId":"xmqqfuve2vln.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"1458931156-29125-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T19:41:56Z","receivedAt":"2016-03-25T19:41:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> In the rewrite from shell to C (ee8838d157761, 2015-09-08, submodule:\n> rewrite `module_clone` shell function in C), we never made use of the\n> prefix. Probably it sneaked in as module_list which was converted in the\n> same series had the prefix as well.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nHmph.\n\nThis helper is called from the root level of the superproject's\nworking tree (after cd_to_toplevel is done), and has options like\n--url.  If the user named --url with a relative pathname to a local\nrepository directory (or a bundle file), shouldn't it be adjusted,\nand wouldn't prefix the only clue what that given path is relative\nto?  Same for --reference repository's path.\n\nI am not sure removing \"--prefix=$wt_prefix\" without doing \"git -C\n$wt_prefix\" on the calling side is the right thing to do.  Even\nthough the options list used by this function does not seem to use\nOPTION_FILENAME, parse-options API takes prefix exactly because\nrelative pathnames need to be adjusted, and it smells like that the\nbreakage brought in by this change is merely hidden by existing bugs\nin the code that does not use prefix to adjust relative paths.\n"},{"id":"281875","messageId":"xmqq60wa2s7w.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"xmqqfuve2vln.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T20:54:59Z","receivedAt":"2016-03-25T20:54:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> This helper is called from the root level of the superproject's\n> working tree (after cd_to_toplevel is done), and has options like\n> --url.  If the user named --url with a relative pathname to a local\n> repository directory (or a bundle file), shouldn't it be adjusted,\n> and wouldn't prefix the only clue what that given path is relative\n> to?  Same for --reference repository's path.\n>\n> I am not sure removing \"--prefix=$wt_prefix\" without doing \"git -C\n> $wt_prefix\" on the calling side is the right thing to do.  Even\n> though the options list used by this function does not seem to use\n> OPTION_FILENAME, parse-options API takes prefix exactly because\n> relative pathnames need to be adjusted, and it smells like that the\n> breakage brought in by this change is merely hidden by existing bugs\n> in the code that does not use prefix to adjust relative paths.\n\nAs you may be able to guess from the above, I do not fundamentally\noppose to using \"-C $wt_prefix\" in place of \"--prefix $wt_prefix\".\n\nThese two should be equivalent, from the callers' point of view.\n\nIf you use the \"--prefix $wt_prefix\" thing, then the called program,\nwhich starts at the root level of the working tree, just overrides\nthe prefix that it would get from the caller, as \"prefix\" its own\nstartup sequence computes when it is invoked by its caller is not\nuseful for adjusting the things that are relative to the directory\nthe caller originally was invoked at.\n\nThe resulting behaviour should be as if the called program were\noriginally started inside $wt_prefix directory, the start-up\nsequence crawled up the directory hierarchy to find the root level\nof the working tree and derived $wt_prefix as the prefix that is\ngiven as the third parameter of any builtin cmd_foo().\n\nAnd that is what would happen if you used \"git -C $wt_prefix\" to run\nthe program.  So \"git -C $wt_prefix\" should be functionally\nequivalent to \"--prefix $wt_prefix\", even though the former is less\nefficient by having to do the discovery and series of chdir(2) only\nto discover the prefix the caller already has.\n\nWhat bugs me the most is that, even though they should be\nequivalent, you need 1/5 (or 1&2 in the old series) only when you\nuse \"git -C $wt_prefix\" but not \"--prefix $wt_prefix\".  That means\nthat they are not equivalent.  And it is not clear why, i.e. where\nthey differ.\n\nIt could be that the way \"--prefix $wt_prefix\" work is insufficient\nand \"git -C $wt_prefix\" does more complete job, and the codepath\nthat implements \"--prefix $wt_prefix\" needs to do more to become\ntruly equivalent.  If that is a case, it means that there is a bug\nin \"--prefix $wt_prefix\" codepath and the callers of the program may\nbe compensating for the bug by doing wrong things in order to work\naround the bug.  When switching to \"git -C $wt_prefix\", you may be\nseeing the effect of these wrong things the callers do, exposed as\nbugs that need to be fixed with 1/5 (or 1&2 in the old series).\n\nAnd I'd want to see the log message explain how the two ways are not\nequivalent and what \"--prefix $wt_prefix\" and the current callers\nthat use that mechanism get wrong.\n"},{"id":"281880","messageId":"CAGZ79kZotAZeKJxRsHczMp5BG7edu=GLj6mox08M6afkMu1UDg@mail.gmail.com","threadId":"41829","inReplyTo":"xmqq60wa2s7w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T22:09:29Z","receivedAt":"2016-03-25T22:09:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> This helper is called from the root level of the superproject's\n>> working tree (after cd_to_toplevel is done), and has options like\n>> --url.  If the user named --url with a relative pathname to a local\n>> repository directory (or a bundle file), shouldn't it be adjusted,\n>> and wouldn't prefix the only clue what that given path is relative\n>> to?  Same for --reference repository's path.\n>>\n>> I am not sure removing \"--prefix=$wt_prefix\" without doing \"git -C\n>> $wt_prefix\" on the calling side is the right thing to do.  Even\n>> though the options list used by this function does not seem to use\n>> OPTION_FILENAME, parse-options API takes prefix exactly because\n>> relative pathnames need to be adjusted, and it smells like that the\n>> breakage brought in by this change is merely hidden by existing bugs\n>> in the code that does not use prefix to adjust relative paths.\n>\n> As you may be able to guess from the above, I do not fundamentally\n> oppose to using \"-C $wt_prefix\" in place of \"--prefix $wt_prefix\".\n>\n> These two should be equivalent, from the callers' point of view.\n>\n> If you use the \"--prefix $wt_prefix\" thing, then the called program,\n> which starts at the root level of the working tree, just overrides\n> the prefix that it would get from the caller, as \"prefix\" its own\n> startup sequence computes when it is invoked by its caller is not\n> useful for adjusting the things that are relative to the directory\n> the caller originally was invoked at.\n>\n> The resulting behaviour should be as if the called program were\n> originally started inside $wt_prefix directory, the start-up\n> sequence crawled up the directory hierarchy to find the root level\n> of the working tree and derived $wt_prefix as the prefix that is\n> given as the third parameter of any builtin cmd_foo().\n>\n> And that is what would happen if you used \"git -C $wt_prefix\" to run\n> the program.  So \"git -C $wt_prefix\" should be functionally\n> equivalent to \"--prefix $wt_prefix\", even though the former is less\n> efficient by having to do the discovery and series of chdir(2) only\n> to discover the prefix the caller already has.\n\nAnd the chdir is the only difference.\nSo you may easily be tempted to think \"git -C $wt_prefix\" is the\nsame as  \"--prefix $wt_prefix\", like I did in the very first attempt\nof this series.\n\nThe replacement works fine in all tests except for the recursive\ntests as then the chdir is an important detail. In the submodule\nthere is no $wt_prefix (as it is the parents' wt_prefix we passed in),\n\nSo here is the example from before:\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\nWhen calling \"git submodule <cmd> --recursive\" from within\nrepo/untracked/, the recursed instance will end up in\nrepo/sub/ with the parents prefix (\"untracked/\")\n\nIn case of git -C $wt_prefix we would try to chdir into\nrepo/sub/untracked/ which doesn't exist and our journey ends here.\n\nIn case of \"--prefix $wt_prefix\" we do not chdir in there, but execute\nthe command in the root of the submodule i.e. \"submodule--helper\nlist\" in repo/sub/. As we do not path any path into the listing\nin the recursed submodule, there will be no error reporting from\ninside of  \"submodule--helper list\". All output is done in shell\nwhere we have the parents \"wt_prefix\" and \"prefix\" from those two\nwe can derive the \"displaypath\" which is $(relative_path \"$prefix$sm_path\"\nwhich relates to \"wt_prefix\".\n\nSo conclusion at this point is: wt_prefix and prefix must stay in sync.\n\n\"wt_prefix\" is the relative path inside the repository from where the original\ncommand started, and \"prefix\" is building up when recursing. So once\nwe recursed into repo/sub/subsub  we would have \"prefix\" = sub/subsub\nand wt_prefix=untracked/, such that we can compute the displaypath to\nbe \"../sub/subsub\".\n\nObservation:\nIf we had wt_prefix=(null) and prefix= \"../sub/subsub\", the computed\ndisplay path would be the same.\n\nSo what needs to happen to make git -C $wt_prefix behave the same as\n\"--prefix $wt_prefix\"?\n\n1) in recursing instances we have to have wt_prefix=(null), so that the\n  execution is the same as in \"--prefix $wt_prefix\". This is the same, as\n  we don't try to chdir into non existent directories then.\n2) Because we have to keep \"wt_prefix\" and \"prefix\" in sync, we need to\n  subtract the \"wt_prefix\" from the prefix, such that we end up with\n  passing (wt_prefix=(null), prefix= \"../sub/subsub\") instead of\n  (wt_prefix=untracked/, prefix=sub/subsub\")\n  I wrote subtract, because wt_prefix is always a \"positive\" path going\n  deeper into a path of the superproject. So we can count the number of\n  directories and append so many ../ to prefix, which is roughly what happens\n  in relative_path.\n\n>\n> What bugs me the most is that, even though they should be\n> equivalent, you need 1/5 (or 1&2 in the old series) only when you\n> use \"git -C $wt_prefix\" but not \"--prefix $wt_prefix\".  That means\n> that they are not equivalent.  And it is not clear why, i.e. where\n> they differ.\n\nAs explained above, the chdir is the difference.\nNote that I rewrote the commit message in 1/5, to explain that. Maybe\nI need to be even more explicit there. The paragraph which I thought would\nconvey the message:\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>\n> It could be that the way \"--prefix $wt_prefix\" work is insufficient\n> and \"git -C $wt_prefix\" does more complete job, and the codepath\n> that implements \"--prefix $wt_prefix\" needs to do more to become\n> truly equivalent.  If that is a case, it means that there is a bug\n> in \"--prefix $wt_prefix\" codepath and the callers of the program may\n> be compensating for the bug by doing wrong things in order to work\n> around the bug.  When switching to \"git -C $wt_prefix\", you may be\n> seeing the effect of these wrong things the callers do, exposed as\n> bugs that need to be fixed with 1/5 (or 1&2 in the old series).\n>\n> And I'd want to see the log message explain how the two ways are not\n> equivalent and what \"--prefix $wt_prefix\" and the current callers\n> that use that mechanism get wrong.\n>\n"},{"id":"281883","messageId":"xmqqmvpm18sw.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"CAGZ79kZotAZeKJxRsHczMp5BG7edu=GLj6mox08M6afkMu1UDg@mail.gmail.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T22:39:43Z","receivedAt":"2016-03-25T22:39:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The replacement works fine in all tests except for the recursive\n> tests as then the chdir is an important detail. In the submodule\n> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),\n\nSo the real reason is that we may tweak $wt_prefix to refer to a\nnon-existing directory, that submodule--helper is buggy and does not\nnotice it, and that using \"-C $wt_prefix\" would catch it because it\nfirst tries to chdir to it.\n\nThe calling script \"git submodule\" first sets $wt_prefix to the\noriginal directory, so there is no way chdir'ing back there would\nnot work, but if we update it (e.g. by appending a path to a\nsubmodule we want to work in), it may very well end up referring to\na directory that does not exist (e.g. it may not have been\n\"init\"ed).  Is that what is going on?\n\nIf that is the case, it makes a lot more sense as an explanation.\nThe justification for the main change 4/5 in the log message,\ni.e. \"-C $wt_prefix\" is more familiar, was an irrelevant subjective\nstatement that only said \"we could change it to use -C\" without\nexplaining why \"--prefix $wt_prefix\" was wrong, and that was why I\nwas unhappy about it.\n\n> So here is the example from before:\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 calling \"git submodule <cmd> --recursive\" from within\n> repo/untracked/, the recursed instance will end up in\n> repo/sub/ with the parents prefix (\"untracked/\")\n>\n> In case of git -C $wt_prefix we would try to chdir into\n> repo/sub/untracked/ which doesn't exist and our journey ends here.\n\nThat sounds like an unrelated bug, though.  Whether -C or --prefix\nis used, we shouldn't be using \"repo/sub/untracked\" after going to\n\"repo/sub\".  If the <cmd> somehow wanted to refer to a relative path\n(e.g. \"file\") in the original directory, it should be using\n../untracked as the base (e.g. it would use \"../untracked/file\").\n\nOf course by using -C, you might notice that repo/sub/untracked does\nnot exist, but that is not a proper error checking---what if the\nsubmodule at repo/sub does have a directory with that name?  IOW,\nthe computation that gave repo/sub/untracked instead of ../untracked\nis wrong and using -C would not make it right.\n\nAnd if you clear the prefix you originally obtained in calling\nscript \"git submodule\", which is \"untracked/\", even if <cmd> somehow\nwanted to refer to the \"file\" in that directory, the only clue to do\nso is forever lost.  Again, this is unrelated to -C/--prefix, but\nthat is what the patch 2 in the original series (which was rolled\ninto patch 1 in the update) was about.\n\nSo I am not sure what the value of using -C is.  At least that\n\"example from before\" does not serve as a good justification.\n"},{"id":"281888","messageId":"CAGZ79kbbVN4kQyQoTk+3o5yPZPAdGULtOhKisOLUf9-u7ssh7A@mail.gmail.com","threadId":"41829","inReplyTo":"xmqqmvpm18sw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T23:02:11Z","receivedAt":"2016-03-25T23:02:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The replacement works fine in all tests except for the recursive\n>> tests as then the chdir is an important detail. In the submodule\n>> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),\n>\n> So the real reason is that we may tweak $wt_prefix to refer to a\n> non-existing directory, that submodule--helper is buggy and does not\n> notice it, and that using \"-C $wt_prefix\" would catch it because it\n> first tries to chdir to it.\n\nI agree that we tweak wt_prefix to refer to an existing path.\nHowever I would claim that it's git-submodule.sh which is buggy, not\nthe submodule--helper, because the shell script gives the non-extistent\npath to submodule--helper in the first place.\n\n \"-C $wt_prefix\" would catch it indeed, so we need to fix that before we\nswitch to -C (if at all)\n\n>\n> The calling script \"git submodule\" first sets $wt_prefix to the\n> original directory, so there is no way chdir'ing back there would\n> not work,\n\nright,\n\n> but if we update it (e.g. by appending a path to a\n> submodule we want to work in), it may very well end up referring to\n> a directory that does not exist (e.g. it may not have been\n> \"init\"ed).  Is that what is going on?\n\nOr by changing dirs in the shell script, i.e.\n\nwt_prefix=$(git rev-parse --show-toplevel)\n# wt_prefix is correct until we do:\ncd sm_path\n\n# If we now would \"chdir'ing back there\", we try to\n(in $sm_path) >  cd ./$wt_prefix\n# which would be  $sm_path/$wt_prefix and that may not exists.\n\n>\n> If that is the case, it makes a lot more sense as an explanation.\n> The justification for the main change 4/5 in the log message,\n> i.e. \"-C $wt_prefix\" is more familiar, was an irrelevant subjective\n> statement that only said \"we could change it to use -C\" without\n> explaining why \"--prefix $wt_prefix\" was wrong, and that was why I\n> was unhappy about it.\n>\n>> So here is the example from before:\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 calling \"git submodule <cmd> --recursive\" from within\n>> repo/untracked/, the recursed instance will end up in\n>> repo/sub/ with the parents prefix (\"untracked/\")\n>>\n>> In case of git -C $wt_prefix we would try to chdir into\n>> repo/sub/untracked/ which doesn't exist and our journey ends here.\n>\n> That sounds like an unrelated bug, though.\n\nIt is the thing described above.\n\n>  Whether -C or --prefix\n> is used, we shouldn't be using \"repo/sub/untracked\" after going to\n> \"repo/sub\".  If the <cmd> somehow wanted to refer to a relative path\n> (e.g. \"file\") in the original directory, it should be using\n> ../untracked as the base (e.g. it would use \"../untracked/file\").\n\nIn case of --prefix we do not chdir into \"repo/sub/untracked\"; however\nwe have cwd=\"repo/sub/\" and wt_prefix=untracked (as it's stale from\nthe parent). We have no way of knowing where \"untracked/\" applies to.\nAs the root of the repository is now \"repo/sub/\" (It is a different repo,\nthe submodule), we may assume there is a \"repo/sub/untracked/\",\nbut we never compute that value or chdir there. It's just cwd and wt_prefix\nbeing from different repositories. (superproject&submodule)\n\n>\n> Of course by using -C, you might notice that repo/sub/untracked does\n> not exist, but that is not a proper error checking---what if the\n> submodule at repo/sub does have a directory with that name?  IOW,\n> the computation that gave repo/sub/untracked instead of ../untracked\n> is wrong and using -C would not make it right.\n\nThere is no explicit computation of repo/sub/untracked, but it would happen\nimplicitely in the -C case as we'd be in the repo/sub and try to chdir\nto 'untracked'\n(interpreted as a relative path)\n\n>\n> And if you clear the prefix you originally obtained in calling\n> script \"git submodule\", which is \"untracked/\", even if <cmd> somehow\n> wanted to refer to the \"file\" in that directory, the only clue to do\n> so is forever lost.  Again, this is unrelated to -C/--prefix, but\n> that is what the patch 2 in the original series (which was rolled\n> into patch 1 in the update) was about.\n\nAs of now this file would also be lost I would assume, as it is unclear\nwhich repository you refer to.\n\nIf you are in the \"subsub\" submodule and know that the $wt_prefix=untracked,\nyou still don't know if the original command was invoked from the root super\nproject or the intermediate submodule.\n\nIn the current situation you /may/ be able to reconstruct that by going back\n\"prefix\" as that seems to be the path we traveled so far.\n\n\n>\n> So I am not sure what the value of using -C is.  At least that\n> \"example from before\" does not serve as a good justification.\n>\n"},{"id":"281890","messageId":"xmqqegay175q.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"CAGZ79kbbVN4kQyQoTk+3o5yPZPAdGULtOhKisOLUf9-u7ssh7A@mail.gmail.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T23:15:13Z","receivedAt":"2016-03-25T23:15:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> Of course by using -C, you might notice that repo/sub/untracked does\n>> not exist, but that is not a proper error checking---what if the\n>> submodule at repo/sub does have a directory with that name?  IOW,\n>> the computation that gave repo/sub/untracked instead of ../untracked\n>> is wrong and using -C would not make it right.\n>\n> There is no explicit computation of repo/sub/untracked, but it would happen\n> implicitely in the -C case as we'd be in the repo/sub and try to chdir\n> to 'untracked'\n> (interpreted as a relative path)\n\nYou are looking at repo/sub/untracked that does not have anything to\ndo with the reality, and it does not matter if you have an explicit\ncode to construct \"char *\" that points at such a pathname, or it\nhappens implicitly.  Looking for 'untracked' directory after going\ninside 'repo/sub/untracked' is simply wrong, just like looking for\n'sub/untracked' diretory while staying at 'repo/' is wrong.\n\nIf anything, ../tracked might have some relevance to the reality but\nnobody is computing it, which may be a bug in \"git submodule\" if\n<cmd> wants to have an access to the original place.\n\nIn either case, that is true with either -C/--prefix, no?\n\n>> And if you clear the prefix you originally obtained in calling\n>> script \"git submodule\", which is \"untracked/\", even if <cmd> somehow\n>> wanted to refer to the \"file\" in that directory, the only clue to do\n>> so is forever lost.  Again, this is unrelated to -C/--prefix, but\n>> that is what the patch 2 in the original series (which was rolled\n>> into patch 1 in the update) was about.\n>\n> As of now this file would also be lost I would assume, as it is unclear\n> which repository you refer to.\n>\n> If you are in the \"subsub\" submodule and know that the $wt_prefix=untracked,\n> you still don't know if the original command was invoked from the root super\n> project or the intermediate submodule.\n\nI am talking about a case where\n\n        cd repo\n        cd untracked\n\tgit submodule <cmd> --recurse-submodules --read-from=file\n\nwants to run <cmd>, using information stored in repo/untracked/file,\nand work on submodules repo/sub and repo/sub/subsub.  The reference\nto the original location somehow needs to be carried through if we\nwanted to allow this kind of thing.\n\n>> So I am not sure what the value of using -C is.  At least that\n>> \"example from before\" does not serve as a good justification.\n\nAnd I do not think your reply does not change anything with respect\nto this statement.\n"},{"id":"281897","messageId":"CAGZ79kYgORCf2n5UtjRqmbFic62fSZkN4gOaGPzh7Herk8Otig@mail.gmail.com","threadId":"41829","inReplyTo":"xmqqegay175q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-25T23:51:40Z","receivedAt":"2016-03-25T23:51:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 25, 2016 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> Of course by using -C, you might notice that repo/sub/untracked does\n>>> not exist, but that is not a proper error checking---what if the\n>>> submodule at repo/sub does have a directory with that name?  IOW,\n>>> the computation that gave repo/sub/untracked instead of ../untracked\n>>> is wrong and using -C would not make it right.\n>>\n>> There is no explicit computation of repo/sub/untracked, but it would happen\n>> implicitely in the -C case as we'd be in the repo/sub and try to chdir\n>> to 'untracked'\n>> (interpreted as a relative path)\n>\n> You are looking at repo/sub/untracked that does not have anything to\n> do with the reality, and it does not matter if you have an explicit\n> code to construct \"char *\" that points at such a pathname, or it\n> happens implicitly.  Looking for 'untracked' directory after going\n> inside 'repo/sub/untracked' is simply wrong, just like looking for\n> 'sub/untracked' diretory while staying at 'repo/' is wrong.\n\nRight. But we don't do it. We just keep around stale information, such\nthat it is easily tempted to do such a thing. But we don't currently.\n\nIf we were to switch to -C we would do it, if that bug is not fixed.\n\n>\n> If anything, ../tracked might have some relevance to the reality but\n> nobody is computing it, which may be a bug in \"git submodule\" if\n> <cmd> wants to have an access to the original place.\n>\n> In either case, that is true with either -C/--prefix, no?\n\nWe don't compute  ../tracked in either case.\n\nWe don't have to compute that because there is no --read-from=file\nswitch.\n\n>\n>>> And if you clear the prefix you originally obtained in calling\n>>> script \"git submodule\", which is \"untracked/\", even if <cmd> somehow\n>>> wanted to refer to the \"file\" in that directory, the only clue to do\n>>> so is forever lost.  Again, this is unrelated to -C/--prefix, but\n>>> that is what the patch 2 in the original series (which was rolled\n>>> into patch 1 in the update) was about.\n>>\n>> As of now this file would also be lost I would assume, as it is unclear\n>> which repository you refer to.\n>>\n>> If you are in the \"subsub\" submodule and know that the $wt_prefix=untracked,\n>> you still don't know if the original command was invoked from the root super\n>> project or the intermediate submodule.\n>\n> I am talking about a case where\n>\n>         cd repo\n>         cd untracked\n>         git submodule <cmd> --recurse-submodules --read-from=file\n>\n> wants to run <cmd>, using information stored in repo/untracked/file,\n> and work on submodules repo/sub and repo/sub/subsub.  The reference\n> to the original location somehow needs to be carried through if we\n> wanted to allow this kind of thing.\n\nI fully agree.\n\n1)\nTo carry that trough, we need:\n* filepath, \"the path of the file\" (i.e. file)\n* wt_prefix, \"the path where the user typed the command relative to\nrepo root\" (\"untracked\")\n* prefix, \"where we traveled already from repo root\" (e.g. sub/)\n\nThen the construction is easy as\n    reverse(prefix) + / +  wt_prefix + / + filepath\n    ..     /    untracked    /   file\n\nThat path is a relative path to the current working directory of the\ncommand, such that we can access file.\n\n\n2)\nAnother way to get the same:\n* \"filepath\" , i.e. file\n[* wt_prefix \"relative path to the repo root\", (null) as relative to repo/sub]\n* prefix, \"path traveled so far from where the user typed the\ncommand\", \"../untracked\"\n\nThen the construction is easy as\n    prefix + / + filepath\n    ../untracked   /   file\n\nWe need to decide to stick with one of both interpretations. Don't mix them!\n\n\nNotes on 1)\n* easy model, because everything is relative to\n  the superprojects repo root.\n* It may be more work though as we need to resolve\n  everything to that superprojects root.\n* --prefix $wt_prefix works with that, as we don't try to cd $wt_prefix\n\nNotes on 2)\n* we loose the superproject as the reference, which sounds scary!\n* it doesn't matter, as we have everything in the prefix\n* This model works with both git -C as well as --prefix as has the right\n  properties for wt_prefix.\n\nAnd patch 1/5 switches from model 1) to 2).\n\nI think model 2 is better in this context\n* because it works with either git -C or --prefix\n  (because of wt_prefix = (null) in all submdirectories\n* only need 'prefix' to reference to 'file' (less work!)\n* is easier to get right [1]\n\n\n[1] Looking closer at patch1/5, cmd_status already has\n  prefix=\"$displaypath/\" with displaypath=\"$(relative_path $prefix$sm_path)/\"\n  as is done for cmd_sync and cmd_update. So before patch 1/5\n  we have a bug in cmd_status as it mixes the two models\n  (wt_prefix from 1) and prefix from 2)\n  After patch 1/5 every command uses model 2)\n\n\n\n>\n>>> So I am not sure what the value of using -C is.  At least that\n>>> \"example from before\" does not serve as a good justification.\n>\n> And I do not think your reply does not change anything with respect\n> to this statement.\n"},{"id":"281971","messageId":"xmqqh9fqzgl7.fsf@gitster.mtv.corp.google.com","threadId":"41829","inReplyTo":"xmqqegay175q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-28T16:56:36Z","receivedAt":"2016-03-28T16:56:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am talking about a case where\n>\n>       cd repo\n>       cd untracked\n> \tgit submodule <cmd> --recurse-submodules --read-from=file\n>\n> wants to run <cmd>, using information stored in repo/untracked/file,\n> and work on submodules repo/sub and repo/sub/subsub.  The reference\n> to the original location somehow needs to be carried through if we\n> wanted to allow this kind of thing.\n\nLet us step back a bit and try a simplified scenario to help us\nthink \"prefix\" through from the first principle.\n\nImagine there is a project P like this:\n\n\t/leading/path/Q/P/.git/\n\t/leading/path/Q/P/one/file\n\t/leading/path/Q/P/one/msg\n\nand we do\n\n\tcd /leading/path/Q/P/one\n\tedit file\n        git commit -F msg file\n\nThis example illustrates that there are two quite different uses of\n\"prefix\".  It is used to serve as a name of the path that is used in\nthe history of the project (i.e. \"file\" is prefixed with \"one/\" to\nform \"one/file\" that is the full path recorded in the history).  It\nalso is used to name an entity on the filesystem that does not have\nanything to do with any path recorded in the history (i.e. the\nmessage is read from \"one/msg\", even though the argument given to\n\"-F\" is \"msg\").\n\nThis distinction may be hard to realize until we start doing some\nunusual thing.  Suppose there is a directory Q/R just next to P and\nwe did this:\n\n\tcd /leading/path/Q/R\n\tcp ../P/one/file ../P/one/msg .\n        edit file msg\n\tGIT_WORK_TREE=/leading/path/Q/P\n        GIT_DIR=$GIT_WORK_TREE/.git\n\texport GIT_WORK_TREE GIT_DIR\n        git commit -F msg file\n\nWhat should happen (we are trying to think things through from the\nfirst principle, so this is not asking about the implemented\nbehaviour)?\n\nAs this \"file\" refers to /leading/path/Q/R/file that is outside of\nthe working tree of the project, the reference to it by \"git commit\"\ndoes not make any sense.  The reference to \"msg\", however, does make\nsense--there is no reason why the contents of the message read by\n\"-F\" option have to be inside the working tree.  So it probably is\nsensible to fail this command.  It is the same story if you replace\n\"file\" with \".\", i.e. \"git commit -F msg .\".  The reference to\n\"current directory\" the latter makes to name set of paths to be\nrecorded in the history, made outside the working tree, makes the\ncommand a nonsense.\n\n\tSide note: I suspect that the current implementation of\n\t\"commit\" actually does wrong things to these two uses of the\n\tprefix in that (1) it would not find \"msg\" and (2) it would\n\thappily use an empty string as the \"prefix\" without failing.\n\tThe former is something we may want to fix, but I suspect\n\tthis is not a low-hanging fruit.  The latter is taking the\n\tonly sensible fallback position and I do not feel a strong\n\tneed to change it to error out.\n\nWithout \"file\" parameter, however, it may make sense to allow making\nthe commit.  After all, the command is not talking about any paths\nat the point, and telling us to work on the entire working tree.\n\nUp to this point, the scenario is simplified by not having any\nsubmodule.  Now, imagine the case where Q is the top-level superproject\nof P, i.e.\n\n\t/leading/path/Q/.git/\n\t/leading/path/Q/P/.git/\n\t/leading/path/Q/P/one/file\n\t/leading/path/Q/P/one/msg\n\t/leading/path/Q/R/file\n\t/leading/path/Q/R/msg\n\nWhen \"submodule <cmd> --recurse-submodules\" does the work of <cmd>\nin a submodule, as far as the submodule repository that gets\nrecursed into (i.e. P) is concerned, the situation is exactly like\nthe above example: a command that is meant to work on project P is\nlaunched from ../R that is outside its working tree.\n\nSo a parameter like \"--read-from=file\" in the original example may\nmake sense, while taking any pathspec to be applied inside the\nvisited submodules does not, and instead we should declare that the\nwork of <cmd> for each visited submodule is to be done on the entire\nworking tree of the submodule.\n\nIn other words, the correct value of \"prefix\" for its two different\nuses are different.  The \"prefix\" for the paths recorded in the\nhistory (i.e. those that correspond to \"file\" in the above example)\nhas to be an empty string to denote that the work of <cmd> applies\nto the entire tree, while the \"prefix\" for the filesystem entities\nthat does not have anything to do with the paths in the recorded\nhistory (i.e. those that correspond to \"msg\" in the above example)\nwould be pointing outside the working tree (e.g. \"../R\").\n\nNow, \"git submodule\" uses $wt_prefix to grab the return value of\n\"git rev-parse --show-prefix\" upfront, and it _is_ possible to use\nthe variable to refer to the \"file\" in the original location in \"git\nsubmodule <cmd> --recurse-submodules --read-from=file\" if we wanted\nto, if its mechanism is taught to correctly maintain the variable\nrelative to the root of the working tree of submodules as it visits\nthem.\n\nBut even if we did so, we shouldn't be using that same variable for\nthe other use of \"prefix\", i.e. \"which subdirectory in the working\ntree of the submodule being visited is the one that is being worked\non and paths should be understood relative to\".  That \"prefix\" has\nto be empty string, and for this reason, your patch 1&2 is the right\nthing to do within the context of the current code.  As we'd need\ntwo correct values for \"prefix\" for two different needs, if we ever\nwant to allow an option like \"--read-from=file\", we'd need to add a\nnew variable different from $wt_prefix that is not cleared to an\nempty string but instead is maintained to serve as \"the original\nlocation\".\n\nTwo pieces of concluding remarks:\n\n - I am OK if we decide to postpone introducing a new variable to\n   hold the value of \"prefix\" suitable for the other use, so that we\n   do not close the door to submodule enhancements that need an\n   option like \"--read-from=file\" in the example.\n\n - I suspect that the fix in your 1&2 may be demonstratable without\n   forcing an early failure by switching to \"git -C\".  If the\n   command gets a wrong prefix, its pathspec limitation would become\n   incorrect, so the work of \"<cmd> --recurse-submodules\" that is\n   done on P when run from /leading/path/Q/R/ directory with \"R/\" as\n   a wrong prefix, would try to limit its operation to subdirectory\n   \"R/\" in project P (which does not exist) and would miss things in\n   directory \"one/\", even though we would want the entire tree of P/\n   to be affected.\n"}]}