[PATCH] submodule: fix relative url parsing for scp-style origin

[PATCH] submodule: fix relative url parsing for scp-style origin

[Thomas Rast]

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 <jeffrey.freeman@syncleus.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

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

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Jonathan Nieder]

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.

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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 <jeffrey.freeman@syncleus.com>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> 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.

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Thomas Rast]

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

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Jonathan Nieder]

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 <jrnieder@gmail.com>

>> 	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.

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Thomas Rast]

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

Re: [PATCH] submodule: fix relative url parsing for scp-style origin

[Mark Levedahl]

On 01/10/2011 12:01 PM, Junio C Hamano wrote:
> Thomas Rast<trast@student.ethz.ch>  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<jeffrey.freeman@syncleus.com>
>> Signed-off-by: Thomas Rast<trast@student.ethz.ch>
>> ---
>>
>> 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