[RFC] submodule: fix handling of supermodules with relative origin URLs.

Prior to this change, operations such as git submodule sync produces
the wrong result when the origin URL of the super module
is itself a relative URL.

The issue arises because in this case, the origin URL of the supermodule
needs to be prepended with a prefix that navigates from the submodule to
the supermodule.

This change adds that prefix.

Signed-off-by: Jon Seymour <redacted>
---
 git-submodule.sh | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

If people are ok with the fix, I'll roll this as a patch together
with some tests.

diff --git a/git-submodule.sh b/git-submodule.sh
index 64a70d6..5008867 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -37,7 +37,8 @@ resolve_relative_url ()
 	remoteurl=$(git config "remote.$remote.url") ||
 		remoteurl=$(pwd) # the repository is its own authoritative upstream
 	url="$1"
-	remoteurl=${remoteurl%/}
+	up_path="$(echo "$2" | sed "s/[^/]*/../g")"
+	remoteurl=${remoteurl%/*}
 	sep=/
 	while test -n "$url"
 	do

@@ -45,6 +46,9 @@ resolve_relative_url ()
 		../*)
 			url="${url#../}"
 			case "$remoteurl" in
+			.*/*)
+				remoteurl="${up_path%/}/${remoteurl%/*}"
+				;;
 			*/*)
 				remoteurl="${remoteurl%/*}"
 				;;

@@ -235,11 +239,24 @@ cmd_add()
 		usage
 	fi
 
+	# normalize path:
+	# multiple //; leading ./; /./; /../; trailing /
+	sm_path=$(printf '%s/\n' "$sm_path" |
+		sed -e '
+			s|//*|/|g
+			s|^\(\./\)*||
+			s|/\./|/|g
+			:start
+			s|\([^/]*\)/\.\./||
+			tstart
+			s|/*$||
+		')
+
 	# assure repo is absolute or relative to parent
 	case "$repo" in
 	./*|../*)
 		# dereference source url relative to parent's url
-		realrepo=$(resolve_relative_url "$repo") || exit
+		realrepo=$(resolve_relative_url "$repo" "$sm_path") || exit
 		;;
 	*:*|/*)
 		# absolute url

@@ -250,18 +267,6 @@ cmd_add()
 	;;
 	esac
 
-	# normalize path:
-	# multiple //; leading ./; /./; /../; trailing /
-	sm_path=$(printf '%s/\n' "$sm_path" |
-		sed -e '
-			s|//*|/|g
-			s|^\(\./\)*||
-			s|/\./|/|g
-			:start
-			s|\([^/]*\)/\.\./||
-			tstart
-			s|/*$||
-		')
 	git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
 	die "$(eval_gettext "'\$sm_path' already exists in the index")"
 

@@ -401,13 +406,14 @@ cmd_init()
 		if test -z "$(git config "submodule.$name.url")"
 		then
 			url=$(git config -f .gitmodules submodule."$name".url)
+			sm_path=$(git config -f .gitmodules submodule."$name".path)
 			test -z "$url" &&
 			die "$(eval_gettext "No url found for submodule path '\$sm_path' in .gitmodules")"
 
 			# Possibly a url relative to parent
 			case "$url" in
 			./*|../*)
-				url=$(resolve_relative_url "$url") || exit
+				url=$(resolve_relative_url "$url" "$sm_path") || exit
 				;;
 			esac
 			git config submodule."$name".url "$url" ||

@@ -960,11 +966,12 @@ cmd_sync()
 	do
 		name=$(module_name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
+		sm_path=$(git config -f .gitmodules --get submodule."$name".path)
 
 		# Possibly a url relative to parent
 		case "$url" in
 		./*|../*)
-			url=$(resolve_relative_url "$url") || exit
+			url=$(resolve_relative_url "$url" "$sm_path") || exit
 			;;
 		esac
 

-- 
1.7.10.2.647.g63fe035.dirty

On Fri, May 18, 2012 at 10:13 PM, Jon Seymour [off-list ref] wrote:

-       remoteurl=${remoteurl%/}
+       up_path="$(echo "$2" | sed "s/[^/]*/../g")"
+       remoteurl=${remoteurl%/*}

Oops - didn't mean to change remoteurl here.

+                               remoteurl="${up_path%/}/${remoteurl%/*}"

Meant up_path%/ to be up_path%/*

jon

Am 18.05.2012 14:13, schrieb Jon Seymour:

Prior to this change, operations such as git submodule sync produces
the wrong result when the origin URL of the super module
is itself a relative URL.

The issue arises because in this case, the origin URL of the supermodule
needs to be prepended with a prefix that navigates from the submodule to
the supermodule.

This change adds that prefix.

Thanks, sounds sane. Please see my comments below.

Signed-off-by: Jon Seymour <redacted>
---
 git-submodule.sh | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

If people are ok with the fix, I'll roll this as a patch together
with some tests.

Yeah, tests would be great (and while at it please drop the trailing
'.' from the subject ;-).

This version of the patch does break some existing tests, but your
follow up suggests you already found that out yourself ;-)

Me thinks up_path should be set in the case below, which is the only
place where it is used.

+	remoteurl=${remoteurl%/*}

As you mentioned in your follow up this change is not correct.

up_path should be set here.

Isn't sm_path already set correctly here? I think this line should
be dropped.

Same here.

 
 		# Possibly a url relative to parent
 		case "$url" in
 		./*|../*)
-			url=$(resolve_relative_url "$url") || exit
+			url=$(resolve_relative_url "$url" "$sm_path") || exit
 			;;
 		esac
 

Other than that the patch looks fine.

On Sat, May 19, 2012 at 5:58 AM, Jens Lehmann [off-list ref] wrote:

This version of the patch does break some existing tests, but your
follow up suggests you already found that out yourself ;-)

Actually, that was just by reading  the patch and noticing odd-ness. I
realise now that I snuck in that bogus edit after I had run the tests
which were otherwise clean - d'oh.

Me thinks up_path should be set in the case below, which is the only
place where it is used.

Ok.

Isn't sm_path already set correctly here? I think this line should
be dropped.

Yes, good point. Thanks.

Same here.

Thanks.

Other than that the patch looks fine.

Thanks for the review - I'll add some tests for the new behaviour.

jon.