{"thread":{"id":"26248","subject":"[PATCH] submodule: fix relative url parsing for scp-style origin","startedAt":"2011-01-10T10:37:26Z","lastAt":"2011-01-11T02:59:17Z","messageCount":7,"participants":["Thomas Rast","Jonathan Nieder","Junio C Hamano","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"159261","messageId":"7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch","threadId":"26248","inReplyTo":null,"subject":"[PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-10T10:37:26Z","receivedAt":"2011-01-10T10:37:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The function resolve_relative_url was not prepared to deal with an\nscp-style origin 'user@host:path' in the case where 'path' is only a\nsingle component.  Fix this by extending the logic that strips one\npath component from the $remoteurl.\n\nAlso add tests for both styles of URLs.\n\nNoticed-by: Jeffrey Phillips Freeman <jeffrey.freeman@syncleus.com>\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nIf Mark agrees with the fix, I think this should go in before 1.7.4\nsince it's a pretty annoying bug.\n\n git-submodule.sh           |   16 ++++++++++++++--\n t/t7400-submodule-basic.sh |   38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c21b77a..8b90589 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -37,12 +37,24 @@ resolve_relative_url ()\n \t\tdie \"remote ($remote) does not have a url defined in .git/config\"\n \turl=\"$1\"\n \tremoteurl=${remoteurl%/}\n+\tsep=/\n \twhile test -n \"$url\"\n \tdo\n \t\tcase \"$url\" in\n \t\t../*)\n \t\t\turl=\"${url#../}\"\n-\t\t\tremoteurl=\"${remoteurl%/*}\"\n+\t\t\tcase \"$remoteurl\" in\n+\t\t\t*/*)\n+\t\t\t\tremoteurl=\"${remoteurl%/*}\"\n+\t\t\t\t;;\n+\t\t\t*:*)\n+\t\t\t\tremoteurl=\"${remoteurl%:*}\"\n+\t\t\t\tsep=:\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tdie \"cannot strip one component off url '$remoteurl'\"\n+\t\t\t\t;;\n+\t\t\tesac\n \t\t\t;;\n \t\t./*)\n \t\t\turl=\"${url#./}\"\n@@ -51,7 +63,7 @@ resolve_relative_url ()\n \t\t\tbreak;;\n \t\tesac\n \tdone\n-\techo \"$remoteurl/${url%/}\"\n+\techo \"$remoteurl$sep${url%/}\"\n }\n \n #\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 2c49db9..874279e 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -446,4 +446,42 @@ test_expect_success 'add should fail when path is used by an existing directory'\n \t)\n '\n \n+test_expect_success 'set up for relative path tests' '\n+\tmkdir reltest &&\n+\t(\n+\t\tcd reltest &&\n+\t\tgit init &&\n+\t\tmkdir sub &&\n+\t\t(\n+\t\t\tcd sub &&\n+\t\t\tgit init &&\n+\t\t\ttest_commit foo\n+\t\t) &&\n+\t\tgit add sub &&\n+\t\tgit config -f .gitmodules submodule.sub.path sub &&\n+\t\tgit config -f .gitmodules submodule.sub.url ../subrepo &&\n+\t\tcp .git/config pristine-.git-config\n+\t)\n+'\n+\n+test_expect_success 'relative path works with URL' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tgit config remote.origin.url ssh://hostname/repo &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = ssh://hostname/subrepo\n+\t)\n+'\n+\n+test_expect_success 'relative path works with user@host:path' '\n+\t(\n+\t\tcd reltest &&\n+\t\tcp pristine-.git-config .git/config &&\n+\t\tgit config remote.origin.url user@host:repo &&\n+\t\tgit submodule init &&\n+\t\ttest \"$(git config submodule.sub.url)\" = user@host:subrepo\n+\t)\n+'\n+\n test_done\n-- \n1.7.4.rc1.309.g58aa0\n"},{"id":"159272","messageId":"20110110164801.GA7714@burratino","threadId":"26248","inReplyTo":"7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-10T16:48:01Z","receivedAt":"2011-01-10T16:48:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> The function resolve_relative_url was not prepared to deal with an\n> scp-style origin 'user@host:path' in the case where 'path' is only a\n> single component.  Fix this by extending the logic that strips one\n> path component from the $remoteurl.\n\nThe above description does not mention that the relative URL in\nquestion is \"../otherpath\".  Anyway, I think it's a great idea; thanks\nfor doing it!\n\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n[...]\n> @@ -37,12 +37,24 @@ resolve_relative_url ()\n[...]\n> -\t\t\tremoteurl=\"${remoteurl%/*}\"\n> +\t\t\tcase \"$remoteurl\" in\n> +\t\t\t*/*)\n> +\t\t\t\tremoteurl=\"${remoteurl%/*}\"\n> +\t\t\t\t;;\n> +\t\t\t*:*)\n> +\t\t\t\tremoteurl=\"${remoteurl%:*}\"\n> +\t\t\t\tsep=:\n> +\t\t\t\t;;\n\nWhat happens to\n\n\turl = ssh://example.com:1234\n\turl = ftp://ftp.example.com\n\turl = /local/path/with/a/colon:in:it\n\turl = git://example.com/path/with/a/colon:in:it\n\n> +\t\t\t*)\n> +\t\t\t\tdie \"cannot strip one component off url '$remoteurl'\"\n> +\t\t\t\t;;\n\n\turl = simple.subdir.of.cwd\n\n?  (Not a rhetorical question; I'm only curious.  If the ftp example\nis valid then the existing code already would not cope well with it.)\n\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n\nNice.\n"},{"id":"159274","messageId":"7v62tw7kx6.fsf@alter.siamese.dyndns.org","threadId":"26248","inReplyTo":"7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-10T17:01:09Z","receivedAt":"2011-01-10T17:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> The function resolve_relative_url was not prepared to deal with an\n> scp-style origin 'user@host:path' in the case where 'path' is only a\n> single component.  Fix this by extending the logic that strips one\n> path component from the $remoteurl.\n>\n> Also add tests for both styles of URLs.\n>\n> Noticed-by: Jeffrey Phillips Freeman <jeffrey.freeman@syncleus.com>\n> Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n> ---\n>\n> If Mark agrees with the fix, I think this should go in before 1.7.4\n> since it's a pretty annoying bug.\n\nSeems obvious to me from a cursory look; thanks.\n"},{"id":"159275","messageId":"201101101805.13251.trast@student.ethz.ch","threadId":"26248","inReplyTo":"20110110164801.GA7714@burratino","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-10T17:05:12Z","receivedAt":"2011-01-10T17:05:12Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder wrote:\n> Thomas Rast wrote:\n> > +\t\t\tcase \"$remoteurl\" in\n> > +\t\t\t*/*)\n> > +\t\t\t\tremoteurl=\"${remoteurl%/*}\"\n> > +\t\t\t\t;;\n> > +\t\t\t*:*)\n> > +\t\t\t\tremoteurl=\"${remoteurl%:*}\"\n> > +\t\t\t\tsep=:\n> > +\t\t\t\t;;\n> \n> What happens to\n> \n> \turl = ssh://example.com:1234\n> \turl = ftp://ftp.example.com\n\nNeither specifies a repo, unless you count \"the default dir on the\nremote side\" (do we support that?).  The existing code would have\nsnipped at the / and constructed something like ssh:/otherrepo.\n\n> \turl = /local/path/with/a/colon:in:it\n> \turl = git://example.com/path/with/a/colon:in:it\n\nThese work fine because the first case arm is \"do we still have a\nslash?\", and thus takes precedence over the colon splitting.\n\n> \turl = simple.subdir.of.cwd\n\nThat's caught by the outer 'case':\n\n\t\tcase \"$url\" in\n\t\t../*)\n\t\t\turl=\"${url#../}\"\n\t\t\t# ... what we're discussing here\n\t\t\t;;\n\t\t./*)\n\t\t\turl=\"${url#./}\"\n\t\t\t;;\n\t\t*)\n\t\t\tbreak;;\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159278","messageId":"20110110172227.GA7882@burratino","threadId":"26248","inReplyTo":"201101101805.13251.trast@student.ethz.ch","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-10T17:22:27Z","receivedAt":"2011-01-10T17:22:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n> Jonathan Nieder wrote:\n\n>> \turl = /local/path/with/a/colon:in:it\n>> \turl = git://example.com/path/with/a/colon:in:it\n>\n> These work fine because the first case arm is \"do we still have a\n> slash?\", and thus takes precedence over the colon splitting.\n\nAh, nice (sorry I missed that).  So this can't create a regression\nand for what it's worth\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n>> \turl = simple.subdir.of.cwd\n>\n> That's caught by the outer 'case':\n\nI meant if remoteurl is not fully qualified.  For what it's worth,\nthe answer seems to be that\n\n\tgit clone repo newdir\n\nreplaces \"repo\" with the absolute path \"$(pwd)/repo\" and manually\nusing a relative path as in\n\n\tgit remote add origin subdir\n\ncreates a remote that is not usable, perhaps because different parts\nof the code treat it as relative to different directories (.git or the\ntoplevel).\n\nSorry for the laziness --- I should have just checked.\n"},{"id":"159282","messageId":"201101101918.22955.trast@student.ethz.ch","threadId":"26248","inReplyTo":"20110110172227.GA7882@burratino","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-01-10T18:18:22Z","receivedAt":"2011-01-10T18:18:22Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder wrote:\n> \n> manually using a relative path as in\n> \n> \tgit remote add origin subdir\n> \n> creates a remote that is not usable, perhaps because different parts\n> of the code treat it as relative to different directories (.git or the\n> toplevel).\n\nOh, now I see.  I just assumed this was not supported without looking,\nso thanks for double-checking.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"159298","messageId":"4D2BC785.3090304@gmail.com","threadId":"26248","inReplyTo":"7v62tw7kx6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] submodule: fix relative url parsing for scp-style origin","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-01-11T02:59:17Z","receivedAt":"2011-01-11T02:59:17Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 01/10/2011 12:01 PM, Junio C Hamano wrote:\n> Thomas Rast<trast@student.ethz.ch>  writes:\n>\n>> The function resolve_relative_url was not prepared to deal with an\n>> scp-style origin 'user@host:path' in the case where 'path' is only a\n>> single component.  Fix this by extending the logic that strips one\n>> path component from the $remoteurl.\n>>\n>> Also add tests for both styles of URLs.\n>>\n>> Noticed-by: Jeffrey Phillips Freeman<jeffrey.freeman@syncleus.com>\n>> Signed-off-by: Thomas Rast<trast@student.ethz.ch>\n>> ---\n>>\n>> If Mark agrees with the fix, I think this should go in before 1.7.4\n>> since it's a pretty annoying bug.\n> Seems obvious to me from a cursory look; thanks.\n>\n\nLooks ok to me. (FWIW, note that the original function handles \nuser@host:~/path just fine :^).\n\n\nMark\n"}]}