threads / patch / 26248

patchsubmodule: fix relative url parsing for scp-style origin

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

## tl;dr

7 messages between Jan 10, 2011 and Jan 11, 2011. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Thomas Rast· Jan 10, 2011, 10:37 UTC · lore

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(-)
Show changes to 2 files +52 −2

git-submodule.sh, t/t7400-submodule-basic.sh

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· Jan 10, 2011, 16:48 UTC · re: Thomas Rast · lore

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

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 ()
[...]
Show 9 quoted lines
> -			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.
Thomas Rast· Jan 10, 2011, 17:05 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 14 quoted lines
> 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· Jan 10, 2011, 17:22 UTC · re: Thomas Rast · lore

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

Thomas Rast wrote:
> Jonathan Nieder wrote:
Show 5 quoted lines
>> 	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.
Thomas Rast· Jan 10, 2011, 18:18 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 8 quoted lines
> 
> 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
Junio C Hamano· Jan 10, 2011, 17:01 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@student.ethz.ch> writes:
Show 13 quoted lines
> 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.
Mark Levedahl· Jan 11, 2011, 02:59 UTC · re: Junio C Hamano · lore

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

On 01/10/2011 12:01 PM, Junio C Hamano wrote:
Show 17 quoted lines
> 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

← back to recent threads