{"thread":{"id":"30555","subject":"[RFC] submodule: fix handling of supermodules with relative origin URLs.","startedAt":"2012-05-18T12:13:40Z","lastAt":"2012-05-18T21:45:42Z","messageCount":4,"participants":["Jon Seymour","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"191656","messageId":"1337343220-26717-1-git-send-email-jon.seymour@gmail.com","threadId":"30555","inReplyTo":"AH3Anrr6mLVedBPcgfVwy=5KRjUgdp5W8P0DQ3qaX_UjH-npDw@mail.gmail.com","subject":"[RFC] submodule: fix handling of supermodules with relative origin URLs.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2012-05-18T12:13:40Z","receivedAt":"2012-05-18T12:13:40Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Prior to this change, operations such as git submodule sync produces\nthe wrong result when the origin URL of the super module\nis itself a relative URL.\n\nThe issue arises because in this case, the origin URL of the supermodule\nneeds to be prepended with a prefix that navigates from the submodule to\nthe supermodule.\n\nThis change adds that prefix.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n git-submodule.sh | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\nIf people are ok with the fix, I'll roll this as a patch together\nwith some tests.\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 64a70d6..5008867 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -37,7 +37,8 @@ resolve_relative_url ()\n \tremoteurl=$(git config \"remote.$remote.url\") ||\n \t\tremoteurl=$(pwd) # the repository is its own authoritative upstream\n \turl=\"$1\"\n-\tremoteurl=${remoteurl%/}\n+\tup_path=\"$(echo \"$2\" | sed \"s/[^/]*/../g\")\"\n+\tremoteurl=${remoteurl%/*}\n \tsep=/\n \twhile test -n \"$url\"\n \tdo\n@@ -45,6 +46,9 @@ resolve_relative_url ()\n \t\t../*)\n \t\t\turl=\"${url#../}\"\n \t\t\tcase \"$remoteurl\" in\n+\t\t\t.*/*)\n+\t\t\t\tremoteurl=\"${up_path%/}/${remoteurl%/*}\"\n+\t\t\t\t;;\n \t\t\t*/*)\n \t\t\t\tremoteurl=\"${remoteurl%/*}\"\n \t\t\t\t;;\n@@ -235,11 +239,24 @@ cmd_add()\n \t\tusage\n \tfi\n \n+\t# normalize path:\n+\t# multiple //; leading ./; /./; /../; trailing /\n+\tsm_path=$(printf '%s/\\n' \"$sm_path\" |\n+\t\tsed -e '\n+\t\t\ts|//*|/|g\n+\t\t\ts|^\\(\\./\\)*||\n+\t\t\ts|/\\./|/|g\n+\t\t\t:start\n+\t\t\ts|\\([^/]*\\)/\\.\\./||\n+\t\t\ttstart\n+\t\t\ts|/*$||\n+\t\t')\n+\n \t# assure repo is absolute or relative to parent\n \tcase \"$repo\" in\n \t./*|../*)\n \t\t# dereference source url relative to parent's url\n-\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n+\t\trealrepo=$(resolve_relative_url \"$repo\" \"$sm_path\") || exit\n \t\t;;\n \t*:*|/*)\n \t\t# absolute url\n@@ -250,18 +267,6 @@ cmd_add()\n \t;;\n \tesac\n \n-\t# normalize path:\n-\t# multiple //; leading ./; /./; /../; trailing /\n-\tsm_path=$(printf '%s/\\n' \"$sm_path\" |\n-\t\tsed -e '\n-\t\t\ts|//*|/|g\n-\t\t\ts|^\\(\\./\\)*||\n-\t\t\ts|/\\./|/|g\n-\t\t\t:start\n-\t\t\ts|\\([^/]*\\)/\\.\\./||\n-\t\t\ttstart\n-\t\t\ts|/*$||\n-\t\t')\n \tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n \tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n \n@@ -401,13 +406,14 @@ cmd_init()\n \t\tif test -z \"$(git config \"submodule.$name.url\")\"\n \t\tthen\n \t\t\turl=$(git config -f .gitmodules submodule.\"$name\".url)\n+\t\t\tsm_path=$(git config -f .gitmodules submodule.\"$name\".path)\n \t\t\ttest -z \"$url\" &&\n \t\t\tdie \"$(eval_gettext \"No url found for submodule path '\\$sm_path' in .gitmodules\")\"\n \n \t\t\t# Possibly a url relative to parent\n \t\t\tcase \"$url\" in\n \t\t\t./*|../*)\n-\t\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\t\turl=$(resolve_relative_url \"$url\" \"$sm_path\") || exit\n \t\t\t\t;;\n \t\t\tesac\n \t\t\tgit config submodule.\"$name\".url \"$url\" ||\n@@ -960,11 +966,12 @@ cmd_sync()\n \tdo\n \t\tname=$(module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n+\t\tsm_path=$(git config -f .gitmodules --get submodule.\"$name\".path)\n \n \t\t# Possibly a url relative to parent\n \t\tcase \"$url\" in\n \t\t./*|../*)\n-\t\t\turl=$(resolve_relative_url \"$url\") || exit\n+\t\t\turl=$(resolve_relative_url \"$url\" \"$sm_path\") || exit\n \t\t\t;;\n \t\tesac\n \n-- \n1.7.10.2.647.g63fe035.dirty\n"},{"id":"191657","messageId":"CAH3AnrpKg0vU_T8vWMfqagQkYvpFpzF3f6qoYEq9-H8GZ6pA2w@mail.gmail.com","threadId":"30555","inReplyTo":"1337343220-26717-1-git-send-email-jon.seymour@gmail.com","subject":"Re: [RFC] submodule: fix handling of supermodules with relative origin URLs.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2012-05-18T12:16:57Z","receivedAt":"2012-05-18T12:16:57Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"On Fri, May 18, 2012 at 10:13 PM, Jon Seymour <jon.seymour@gmail.com> wrote:\n> -       remoteurl=${remoteurl%/}\n> +       up_path=\"$(echo \"$2\" | sed \"s/[^/]*/../g\")\"\n> +       remoteurl=${remoteurl%/*}\n\nOops - didn't mean to change remoteurl here.\n\n\n> +                               remoteurl=\"${up_path%/}/${remoteurl%/*}\"\n\nMeant up_path%/ to be up_path%/*\n\njon\n"},{"id":"191675","messageId":"4FB6A9CB.5050702@web.de","threadId":"30555","inReplyTo":"1337343220-26717-1-git-send-email-jon.seymour@gmail.com","subject":"Re: [RFC] submodule: fix handling of supermodules with relative origin URLs.","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-05-18T19:58:03Z","receivedAt":"2012-05-18T19:58:03Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.05.2012 14:13, schrieb Jon Seymour:\n> Prior to this change, operations such as git submodule sync produces\n> the wrong result when the origin URL of the super module\n> is itself a relative URL.\n> \n> The issue arises because in this case, the origin URL of the supermodule\n> needs to be prepended with a prefix that navigates from the submodule to\n> the supermodule.\n> \n> This change adds that prefix.\n\nThanks, sounds sane. Please see my comments below.\n\n> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>\n> ---\n>  git-submodule.sh | 39 +++++++++++++++++++++++----------------\n>  1 file changed, 23 insertions(+), 16 deletions(-)\n> \n> If people are ok with the fix, I'll roll this as a patch together\n> with some tests.\n\nYeah, tests would be great (and while at it please drop the trailing\n'.' from the subject ;-).\n\nThis version of the patch does break some existing tests, but your\nfollow up suggests you already found that out yourself ;-)\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 64a70d6..5008867 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -37,7 +37,8 @@ resolve_relative_url ()\n>  \tremoteurl=$(git config \"remote.$remote.url\") ||\n>  \t\tremoteurl=$(pwd) # the repository is its own authoritative upstream\n>  \turl=\"$1\"\n> -\tremoteurl=${remoteurl%/}\n> +\tup_path=\"$(echo \"$2\" | sed \"s/[^/]*/../g\")\"\n\nMe thinks up_path should be set in the case below, which is the only\nplace where it is used.\n\n> +\tremoteurl=${remoteurl%/*}\n\nAs you mentioned in your follow up this change is not correct.\n\n>  \tsep=/\n>  \twhile test -n \"$url\"\n>  \tdo\n> @@ -45,6 +46,9 @@ resolve_relative_url ()\n>  \t\t../*)\n>  \t\t\turl=\"${url#../}\"\n>  \t\t\tcase \"$remoteurl\" in\n> +\t\t\t.*/*)\n\nup_path should be set here.\n\n> +\t\t\t\tremoteurl=\"${up_path%/}/${remoteurl%/*}\"\n> +\t\t\t\t;;\n>  \t\t\t*/*)\n>  \t\t\t\tremoteurl=\"${remoteurl%/*}\"\n>  \t\t\t\t;;\n> @@ -235,11 +239,24 @@ cmd_add()\n>  \t\tusage\n>  \tfi\n>  \n> +\t# normalize path:\n> +\t# multiple //; leading ./; /./; /../; trailing /\n> +\tsm_path=$(printf '%s/\\n' \"$sm_path\" |\n> +\t\tsed -e '\n> +\t\t\ts|//*|/|g\n> +\t\t\ts|^\\(\\./\\)*||\n> +\t\t\ts|/\\./|/|g\n> +\t\t\t:start\n> +\t\t\ts|\\([^/]*\\)/\\.\\./||\n> +\t\t\ttstart\n> +\t\t\ts|/*$||\n> +\t\t')\n> +\n>  \t# assure repo is absolute or relative to parent\n>  \tcase \"$repo\" in\n>  \t./*|../*)\n>  \t\t# dereference source url relative to parent's url\n> -\t\trealrepo=$(resolve_relative_url \"$repo\") || exit\n> +\t\trealrepo=$(resolve_relative_url \"$repo\" \"$sm_path\") || exit\n>  \t\t;;\n>  \t*:*|/*)\n>  \t\t# absolute url\n> @@ -250,18 +267,6 @@ cmd_add()\n>  \t;;\n>  \tesac\n>  \n> -\t# normalize path:\n> -\t# multiple //; leading ./; /./; /../; trailing /\n> -\tsm_path=$(printf '%s/\\n' \"$sm_path\" |\n> -\t\tsed -e '\n> -\t\t\ts|//*|/|g\n> -\t\t\ts|^\\(\\./\\)*||\n> -\t\t\ts|/\\./|/|g\n> -\t\t\t:start\n> -\t\t\ts|\\([^/]*\\)/\\.\\./||\n> -\t\t\ttstart\n> -\t\t\ts|/*$||\n> -\t\t')\n>  \tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n>  \tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n>  \n> @@ -401,13 +406,14 @@ cmd_init()\n>  \t\tif test -z \"$(git config \"submodule.$name.url\")\"\n>  \t\tthen\n>  \t\t\turl=$(git config -f .gitmodules submodule.\"$name\".url)\n> +\t\t\tsm_path=$(git config -f .gitmodules submodule.\"$name\".path)\n\nIsn't sm_path already set correctly here? I think this line should\nbe dropped.\n\n>  \t\t\ttest -z \"$url\" &&\n>  \t\t\tdie \"$(eval_gettext \"No url found for submodule path '\\$sm_path' in .gitmodules\")\"\n>  \n>  \t\t\t# Possibly a url relative to parent\n>  \t\t\tcase \"$url\" in\n>  \t\t\t./*|../*)\n> -\t\t\t\turl=$(resolve_relative_url \"$url\") || exit\n> +\t\t\t\turl=$(resolve_relative_url \"$url\" \"$sm_path\") || exit\n>  \t\t\t\t;;\n>  \t\t\tesac\n>  \t\t\tgit config submodule.\"$name\".url \"$url\" ||\n> @@ -960,11 +966,12 @@ cmd_sync()\n>  \tdo\n>  \t\tname=$(module_name \"$sm_path\")\n>  \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n> +\t\tsm_path=$(git config -f .gitmodules --get submodule.\"$name\".path)\n\nSame here.\n\n>  \n>  \t\t# Possibly a url relative to parent\n>  \t\tcase \"$url\" in\n>  \t\t./*|../*)\n> -\t\t\turl=$(resolve_relative_url \"$url\") || exit\n> +\t\t\turl=$(resolve_relative_url \"$url\" \"$sm_path\") || exit\n>  \t\t\t;;\n>  \t\tesac\n>  \n\nOther than that the patch looks fine.\n"},{"id":"191682","messageId":"CAH3AnrqXbUuKKWzAf9_xHtu-c5sYGRf5dj+CqOmOmesnwh8=pg@mail.gmail.com","threadId":"30555","inReplyTo":"4FB6A9CB.5050702@web.de","subject":"Re: [RFC] submodule: fix handling of supermodules with relative origin URLs.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2012-05-18T21:45:42Z","receivedAt":"2012-05-18T21:45:42Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"On Sat, May 19, 2012 at 5:58 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> This version of the patch does break some existing tests, but your\n> follow up suggests you already found that out yourself ;-)\n>\n\nActually, that was just by reading  the patch and noticing odd-ness. I\nrealise now that I snuck in that bogus edit after I had run the tests\nwhich were otherwise clean - d'oh.\n\n>\n> Me thinks up_path should be set in the case below, which is the only\n> place where it is used.\n\nOk.\n\n>\n> Isn't sm_path already set correctly here? I think this line should\n> be dropped.\n>\n\nYes, good point. Thanks.\n\n>\n> Same here.\n>\n\nThanks.\n\n>\n> Other than that the patch looks fine.\n\nThanks for the review - I'll add some tests for the new behaviour.\n\njon.\n"}]}