# [PATCH] submodule: fix relative url parsing for scp-style origin 7 messages from 2011-01-10 to 2011-01-11. Participants: Thomas Rast, Jonathan Nieder, Junio C Hamano, Mark Levedahl. Thread: https://gitlist.dev/t/26248 ## Thomas Rast, 2011-01-10 10:37 Subject: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast%40student.ethz.ch ``` The function resolve_relative_url was not prepared to deal with an scp-style origin 'user@host:path' in the case where 'path' is only a single component. Fix this by extending the logic that strips one path component from the $remoteurl. Also add tests for both styles of URLs. Noticed-by: Jeffrey Phillips Freeman Signed-off-by: Thomas Rast --- If Mark agrees with the fix, I think this should go in before 1.7.4 since it's a pretty annoying bug. git-submodule.sh | 16 ++++++++++++++-- t/t7400-submodule-basic.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index c21b77a..8b90589 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -37,12 +37,24 @@ resolve_relative_url () die "remote ($remote) does not have a url defined in .git/config" url="$1" remoteurl=${remoteurl%/} + sep=/ while test -n "$url" do case "$url" in ../*) url="${url#../}" - remoteurl="${remoteurl%/*}" + case "$remoteurl" in + */*) + remoteurl="${remoteurl%/*}" + ;; + *:*) + remoteurl="${remoteurl%:*}" + sep=: + ;; + *) + die "cannot strip one component off url '$remoteurl'" + ;; + esac ;; ./*) url="${url#./}" @@ -51,7 +63,7 @@ resolve_relative_url () break;; esac done - echo "$remoteurl/${url%/}" + echo "$remoteurl$sep${url%/}" } # diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 2c49db9..874279e 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -446,4 +446,42 @@ test_expect_success 'add should fail when path is used by an existing directory' ) ' +test_expect_success 'set up for relative path tests' ' + mkdir reltest && + ( + cd reltest && + git init && + mkdir sub && + ( + cd sub && + git init && + test_commit foo + ) && + git add sub && + git config -f .gitmodules submodule.sub.path sub && + git config -f .gitmodules submodule.sub.url ../subrepo && + cp .git/config pristine-.git-config + ) +' + +test_expect_success 'relative path works with URL' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url ssh://hostname/repo && + git submodule init && + test "$(git config submodule.sub.url)" = ssh://hostname/subrepo + ) +' + +test_expect_success 'relative path works with user@host:path' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + git config remote.origin.url user@host:repo && + git submodule init && + test "$(git config submodule.sub.url)" = user@host:subrepo + ) +' + test_done -- 1.7.4.rc1.309.g58aa0 ``` ## Jonathan Nieder, 2011-01-10 16:48 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <20110110164801.GA7714@burratino> URL: https://gitlist.dev/e/20110110164801.GA7714%40burratino In-Reply-To: <7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch> ``` Thomas Rast wrote: > The function resolve_relative_url was not prepared to deal with an > scp-style origin 'user@host:path' in the case where 'path' is only a > single component. Fix this by extending the logic that strips one > path component from the $remoteurl. The above description does not mention that the relative URL in question is "../otherpath". Anyway, I think it's a great idea; thanks for doing it! > --- a/git-submodule.sh > +++ b/git-submodule.sh [...] > @@ -37,12 +37,24 @@ resolve_relative_url () [...] > - remoteurl="${remoteurl%/*}" > + case "$remoteurl" in > + */*) > + remoteurl="${remoteurl%/*}" > + ;; > + *:*) > + remoteurl="${remoteurl%:*}" > + sep=: > + ;; What happens to url = ssh://example.com:1234 url = ftp://ftp.example.com url = /local/path/with/a/colon:in:it url = git://example.com/path/with/a/colon:in:it > + *) > + die "cannot strip one component off url '$remoteurl'" > + ;; url = simple.subdir.of.cwd ? (Not a rhetorical question; I'm only curious. If the ftp example is valid then the existing code already would not cope well with it.) > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh Nice. ``` ## Junio C Hamano, 2011-01-10 17:01 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <7v62tw7kx6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v62tw7kx6.fsf%40alter.siamese.dyndns.org In-Reply-To: <7a578ef3384a891cebd4a39421302983499771e0.1294655635.git.trast@student.ethz.ch> ``` Thomas Rast writes: > The function resolve_relative_url was not prepared to deal with an > scp-style origin 'user@host:path' in the case where 'path' is only a > single component. Fix this by extending the logic that strips one > path component from the $remoteurl. > > Also add tests for both styles of URLs. > > Noticed-by: Jeffrey Phillips Freeman > Signed-off-by: Thomas Rast > --- > > If Mark agrees with the fix, I think this should go in before 1.7.4 > since it's a pretty annoying bug. Seems obvious to me from a cursory look; thanks. ``` ## Thomas Rast, 2011-01-10 17:05 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <201101101805.13251.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101101805.13251.trast%40student.ethz.ch In-Reply-To: <20110110164801.GA7714@burratino> ``` Jonathan Nieder wrote: > Thomas Rast wrote: > > + case "$remoteurl" in > > + */*) > > + remoteurl="${remoteurl%/*}" > > + ;; > > + *:*) > > + remoteurl="${remoteurl%:*}" > > + sep=: > > + ;; > > What happens to > > url = ssh://example.com:1234 > url = ftp://ftp.example.com Neither specifies a repo, unless you count "the default dir on the remote side" (do we support that?). The existing code would have snipped at the / and constructed something like ssh:/otherrepo. > url = /local/path/with/a/colon:in:it > url = git://example.com/path/with/a/colon:in:it These work fine because the first case arm is "do we still have a slash?", and thus takes precedence over the colon splitting. > url = simple.subdir.of.cwd That's caught by the outer 'case': case "$url" in ../*) url="${url#../}" # ... what we're discussing here ;; ./*) url="${url#./}" ;; *) break;; -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Jonathan Nieder, 2011-01-10 17:22 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <20110110172227.GA7882@burratino> URL: https://gitlist.dev/e/20110110172227.GA7882%40burratino In-Reply-To: <201101101805.13251.trast@student.ethz.ch> ``` Thomas Rast wrote: > Jonathan Nieder wrote: >> url = /local/path/with/a/colon:in:it >> url = git://example.com/path/with/a/colon:in:it > > These work fine because the first case arm is "do we still have a > slash?", and thus takes precedence over the colon splitting. Ah, nice (sorry I missed that). So this can't create a regression and for what it's worth Reviewed-by: Jonathan Nieder >> url = simple.subdir.of.cwd > > That's caught by the outer 'case': I meant if remoteurl is not fully qualified. For what it's worth, the answer seems to be that git clone repo newdir replaces "repo" with the absolute path "$(pwd)/repo" and manually using a relative path as in git remote add origin subdir creates a remote that is not usable, perhaps because different parts of the code treat it as relative to different directories (.git or the toplevel). Sorry for the laziness --- I should have just checked. ``` ## Thomas Rast, 2011-01-10 18:18 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <201101101918.22955.trast@student.ethz.ch> URL: https://gitlist.dev/e/201101101918.22955.trast%40student.ethz.ch In-Reply-To: <20110110172227.GA7882@burratino> ``` Jonathan Nieder wrote: > > manually using a relative path as in > > git remote add origin subdir > > creates a remote that is not usable, perhaps because different parts > of the code treat it as relative to different directories (.git or the > toplevel). Oh, now I see. I just assumed this was not supported without looking, so thanks for double-checking. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Mark Levedahl, 2011-01-11 02:59 Subject: Re: [PATCH] submodule: fix relative url parsing for scp-style origin Message-ID: <4D2BC785.3090304@gmail.com> URL: https://gitlist.dev/e/4D2BC785.3090304%40gmail.com In-Reply-To: <7v62tw7kx6.fsf@alter.siamese.dyndns.org> ``` On 01/10/2011 12:01 PM, Junio C Hamano wrote: > Thomas Rast writes: > >> The function resolve_relative_url was not prepared to deal with an >> scp-style origin 'user@host:path' in the case where 'path' is only a >> single component. Fix this by extending the logic that strips one >> path component from the $remoteurl. >> >> Also add tests for both styles of URLs. >> >> Noticed-by: Jeffrey Phillips Freeman >> Signed-off-by: Thomas Rast >> --- >> >> If Mark agrees with the fix, I think this should go in before 1.7.4 >> since it's a pretty annoying bug. > Seems obvious to me from a cursory look; thanks. > Looks ok to me. (FWIW, note that the original function handles user@host:~/path just fine :^). Mark ```