[PATCH v5 0/2] series: submodule: fix handling of relative superproject origin URLs

[PATCH v5 0/2] series: submodule: fix handling of relative superproject origin URLs

[Jon Seymour]

This series ensures that git submodule handles relative superproject origin URLs correctly.

Current behaviour:

* does not guarantee that the submodule.{name}.url property of the superproject is 
always confgured with a valid path to the submodule's origin repo that is 
relative to the working tree of the superproject.
* does not guarantee that the remote.origin.url property of the submodule is
always configured with a valid path to the submodule's origin repo that is 
relative to the working tree of the submodule
* errors out (origin URL = foo) even though this is not strictly necessary.

These changes:

* guarantee that the configured paths are always valid relative paths
from the working tree of the repo containing to the configuration to the
origin repo of the described submodule.

Jon Seymour (2):
  submodule: document failures handling relative superproject origin
    URLs
  submodule: fix handling of relative superproject origin URLs

 git-submodule.sh           | 57 +++++++++++++++++++++++++++++++++----
 t/t7400-submodule-basic.sh | 60 +++++++++++++++++++++++++++++++++++++++
 t/t7403-submodule-sync.sh  | 70 +++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 180 insertions(+), 7 deletions(-)

v4->v5 changes:
       Used test_expect_failure for failing tests.
       Added comments to justify statements.
       Isolated changes to the relative branch.
       Used role-based variable names to help explain transformations.	

-- 
1.7.10.2.649.g5ca7d80

[PATCH v5 1/2] submodule: document failures handling relative superproject origin URLs

[Jon Seymour]

These tests document how submodule sync and init handle various
kinds of relative super project origin URLs.  The submodule URL
path is ../subrepo.

6 cases are documented:
  foo
  foo/bar
  ./foo
  ./foo/bar
  ../foo
  ../foo/bar

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++
 t/t7403-submodule-sync.sh  | 76 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 137 insertions(+), 1 deletion(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..08b8d2f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -507,6 +507,68 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_failure 'relative path works with foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url foo &&
+		# actual: fails with an error
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ./subrepo
+	)
+'
+
+test_expect_success 'relative path works with foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url foo/bar &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = foo/subrepo
+	)
+'
+
+test_expect_success 'relative path works with ./foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url ./foo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ./subrepo
+	)
+'
+
+test_expect_failure 'relative path works with ./foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url ./foo/bar &&
+		git submodule init &&
+		#actual: ./foo/subrepo
+		test "$(git config submodule.sub.url)" = foo/subrepo
+	)
+'
+
+test_expect_success 'relative path works with ../foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url ../foo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../subrepo
+	)
+'
+
+test_expect_success 'relative path works with ../foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url ../foo/bar &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../foo/subrepo
+	)
+'
+
 test_expect_success 'moving the superproject does not break submodules' '
 	(
 		cd addtest &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 3620215..9fa4e58 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -26,7 +26,9 @@ test_expect_success setup '
 	(cd super-clone && git submodule update --init) &&
 	git clone super empty-clone &&
 	(cd empty-clone && git submodule init) &&
-	git clone super top-only-clone
+	git clone super top-only-clone &&
+	git clone super relative-clone &&
+	(cd relative-clone && git submodule update --init)
 '
 
 test_expect_success 'change submodule' '
@@ -86,4 +88,76 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
+test_expect_failure '"git submodule sync" handles origin URL of the form foo' '
+	(cd relative-clone &&
+	 git remote set-url origin foo
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual fails with: "cannot strip off url foo
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form foo/bar' '
+	(cd relative-clone &&
+	 git remote set-url origin foo/bar
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual foo/submodule
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../foo/submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form ./foo' '
+	(cd relative-clone &&
+	 git remote set-url origin ./foo
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual ./submodule
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form ./foo/bar' '
+	(cd relative-clone &&
+	 git remote set-url origin ./foo/bar
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual ./foo/submodule
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../foo/submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form ../foo' '
+	(cd relative-clone &&
+	 git remote set-url origin ../foo
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual ../submodule
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../../submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form ../foo/bar' '
+	(cd relative-clone &&
+	 git remote set-url origin ../foo/bar
+	 git submodule sync &&
+	(cd submodule &&
+	 #actual ../foo/submodule
+	 git config remote.origin.url &&
+	 test "$(git config remote.origin.url)" == "../../foo/submodule"
+	)
+	)
+'
+
 test_done
-- 
1.7.10.2.649.g5ca7d80

[PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs

[Jon Seymour]

When the origin URL of the superproject is itself relative, an operation
such as git submodule add, init or sync may result in either the
submodule.{name}.url configuration property of the superproject
referring to the incorrect location or remote.origin.url configuration
property of the submodule referring to the incorrect location or both
these conditions. In some cases, git submodule fails to update
the configuration and fails with an error condition.

The issue arises in these cases because the origin URL of
the superproject needs to be prepended with a prefix that navigates
from the submodule to the superproject so that when the submodule
URL is concatenated the resulting URL is relative to the working tree
of the submodule.

This change fixes handling for relative superproject origin URLs
for 6 cases:
  foo
  foo/bar
  ./foo
  ./foo/bar
  ../foo
  ../foo/bar

In each case, the configuration properties in the superproject's
configuration and the submodule's configuration refer to the
correct, relative, location of the submodule's origin repo. In all cases,
the configured paths are relative to the working trees of the
repositories containing the configuration.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh           | 57 +++++++++++++++++++++++++++++++++++++++++-----
 t/t7400-submodule-basic.sh |  6 ++---
 t/t7403-submodule-sync.sh  | 18 +++++----------
 3 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 64a70d6..738eba3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -30,13 +30,35 @@ nofetch=
 update=
 prefix=
 
-# Resolve relative url by appending to parent's url
+# Resolve relative url by appending the submodule url
+# to the superproject's origin URL
+#
+# If the origin URL is itself a relative URL prepend
+# an additional prefix, if present, that represents
+# the relative path from the submodule's working tree
+# to the superprojects' working tree.
+#
+# This behaviour is required to ensure that the origin URL
+# of a submodule, when relative, is relative to the
+# submodule's work tree and not to the superproject's work tree.
+#
 resolve_relative_url ()
 {
 	remote=$(get_default_remote)
 	remoteurl=$(git config "remote.$remote.url") ||
 		remoteurl=$(pwd) # the repository is its own authoritative upstream
 	url="$1"
+	up_path="$2"
+
+	#
+	# ensure all relative paths begin with ./ to enable
+	# selection relative branch of subsequent case "$remoteurl"
+	# statement.
+	#
+	# rewrites foo/bar to ./foo/bar but leaves /foo, :foo ./foo
+	# and ../foo untouched.
+	#
+	remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
 	remoteurl=${remoteurl%/}
 	sep=/
 	while test -n "$url"
@@ -45,6 +67,16 @@ resolve_relative_url ()
 		../*)
 			url="${url#../}"
 			case "$remoteurl" in
+			.*/*)
+				# remove last part
+				remoteurl="${remoteurl%/*}"
+				# remove redundant leading ./
+				remoteurl="${remoteurl#./}"
+				# prefix path from submodule work tree to superproject work tree
+				remoteurl="${up_path}${remoteurl}"
+				# remove trailing /.
+				remoteurl="${remoteurl%/.}"
+				;;
 			*/*)
 				remoteurl="${remoteurl%/*}"
 				;;
@@ -959,19 +991,32 @@ cmd_sync()
 	while read mode sha1 stage sm_path
 	do
 		name=$(module_name "$sm_path")
-		url=$(git config -f .gitmodules --get submodule."$name".url)
+		# path from superproject origin repo to submodule origin repo
+		module_url=$(git config -f .gitmodules --get submodule."$name".url)
 
 		# Possibly a url relative to parent
-		case "$url" in
+		case "$module_url" in
 		./*|../*)
-			url=$(resolve_relative_url "$url") || exit
+			# rewrite foo/bar as ../.. to find path from
+			# submodule work tree to superproject work tree
+			up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
+			# guarantee a trailing /
+			up_path=${up_path%/}/ &&
+			# path from submodule work tree to submodule origin repo
+			sub_origin_url=$(resolve_relative_url "$module_url" "$up_path") &&
+			# path from superproject work tree to submodule origin repo
+			super_config_url=$(resolve_relative_url "$module_url") || exit
+			;;
+		*)
+			sub_origin_url="$module_url"
+			super_config_url="$module_url"
 			;;
 		esac
 
 		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
 			say "$(eval_gettext "Synchronizing submodule url for '\$name'")"
-			git config submodule."$name".url "$url"
+			git config submodule."$name".url "$super_config_url"
 
 			if test -e "$sm_path"/.git
 			then
@@ -979,7 +1024,7 @@ cmd_sync()
 				clear_local_git_env
 				cd "$sm_path"
 				remote=$(get_default_remote)
-				git config remote."$remote".url "$url"
+				git config remote."$remote".url "$sub_origin_url"
 			)
 			fi
 		fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 08b8d2f..f2f907f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -507,12 +507,11 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
-test_expect_failure 'relative path works with foo' '
+test_expect_success 'relative path works with foo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
 		git config remote.origin.url foo &&
-		# actual: fails with an error
 		git submodule init &&
 		test "$(git config submodule.sub.url)" = ./subrepo
 	)
@@ -538,13 +537,12 @@ test_expect_success 'relative path works with ./foo' '
 	)
 '
 
-test_expect_failure 'relative path works with ./foo/bar' '
+test_expect_success 'relative path works with ./foo/bar' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
 		git config remote.origin.url ./foo/bar &&
 		git submodule init &&
-		#actual: ./foo/subrepo
 		test "$(git config submodule.sub.url)" = foo/subrepo
 	)
 '
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 9fa4e58..9526f8d 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -88,72 +88,66 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form foo' '
+test_expect_success '"git submodule sync" handles origin URL of the form foo' '
 	(cd relative-clone &&
 	 git remote set-url origin foo
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual fails with: "cannot strip off url foo
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form foo/bar' '
+test_expect_success '"git submodule sync" handles origin URL of the form foo/bar' '
 	(cd relative-clone &&
 	 git remote set-url origin foo/bar
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual foo/submodule
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../foo/submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form ./foo' '
+test_expect_success '"git submodule sync" handles origin URL of the form ./foo' '
 	(cd relative-clone &&
 	 git remote set-url origin ./foo
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual ./submodule
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form ./foo/bar' '
+test_expect_success '"git submodule sync" handles origin URL of the form ./foo/bar' '
 	(cd relative-clone &&
 	 git remote set-url origin ./foo/bar
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual ./foo/submodule
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../foo/submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form ../foo' '
+test_expect_success '"git submodule sync" handles origin URL of the form ../foo' '
 	(cd relative-clone &&
 	 git remote set-url origin ../foo
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual ../submodule
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../../submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form ../foo/bar' '
+test_expect_success '"git submodule sync" handles origin URL of the form ../foo/bar' '
 	(cd relative-clone &&
 	 git remote set-url origin ../foo/bar
 	 git submodule sync &&
 	(cd submodule &&
-	 #actual ../foo/submodule
 	 git config remote.origin.url &&
 	 test "$(git config remote.origin.url)" == "../../foo/submodule"
 	)
-- 
1.7.10.2.649.g5ca7d80

Re: [PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs

[Jon Seymour]

On Thu, May 24, 2012 at 1:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> When the origin URL of the superproject is itself relative, an operation
> such as git submodule add, init or sync may result in either the
> submodule.{name}.url configuration property of the superproject
> referring to the incorrect location or remote.origin.url configuration
> property of the submodule referring to the incorrect location or both
> these conditions. In some cases, git submodule fails to update
> the configuration and fails with an error condition.
>
> The issue arises in these cases because the origin URL of
> the superproject needs to be prepended with a prefix that navigates
> from the submodule to the superproject so that when the submodule
> URL is concatenated the resulting URL is relative to the working tree
> of the submodule.
>
> This change fixes handling for relative superproject origin URLs
> for 6 cases:
>  foo
>  foo/bar
>  ./foo
>  ./foo/bar
>  ../foo
>  ../foo/bar
>
> In each case, the configuration properties in the superproject's
> configuration and the submodule's configuration refer to the
> correct, relative, location of the submodule's origin repo. In all cases,
> the configured paths are relative to the working trees of the
> repositories containing the configuration.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  git-submodule.sh           | 57 +++++++++++++++++++++++++++++++++++++++++-----
>  t/t7400-submodule-basic.sh |  6 ++---
>  t/t7403-submodule-sync.sh  | 18 +++++----------
>  3 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 64a70d6..738eba3 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -30,13 +30,35 @@ nofetch=
>  update=
>  prefix=
>
> -# Resolve relative url by appending to parent's url
> +# Resolve relative url by appending the submodule url
> +# to the superproject's origin URL
> +#
> +# If the origin URL is itself a relative URL prepend
> +# an additional prefix, if present, that represents
> +# the relative path from the submodule's working tree
> +# to the superprojects' working tree.
> +#
> +# This behaviour is required to ensure that the origin URL
> +# of a submodule, when relative, is relative to the
> +# submodule's work tree and not to the superproject's work tree.
> +#
>  resolve_relative_url ()
>  {
>        remote=$(get_default_remote)
>        remoteurl=$(git config "remote.$remote.url") ||
>                remoteurl=$(pwd) # the repository is its own authoritative upstream
>        url="$1"
> +       up_path="$2"
> +
> +       #
> +       # ensure all relative paths begin with ./ to enable
> +       # selection relative branch of subsequent case "$remoteurl"
> +       # statement.
> +       #
> +       # rewrites foo/bar to ./foo/bar but leaves /foo, :foo ./foo
> +       # and ../foo untouched.
> +       #
> +       remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>        remoteurl=${remoteurl%/}
>        sep=/
>        while test -n "$url"
> @@ -45,6 +67,16 @@ resolve_relative_url ()
>                ../*)
>                        url="${url#../}"
>                        case "$remoteurl" in
> +                       .*/*)
> +                               # remove last part
> +                               remoteurl="${remoteurl%/*}"
> +                               # remove redundant leading ./
> +                               remoteurl="${remoteurl#./}"
> +                               # prefix path from submodule work tree to superproject work tree
> +                               remoteurl="${up_path}${remoteurl}"
> +                               # remove trailing /.
> +                               remoteurl="${remoteurl%/.}"
> +                               ;;
>                        */*)
>                                remoteurl="${remoteurl%/*}"
>                                ;;
> @@ -959,19 +991,32 @@ cmd_sync()
>        while read mode sha1 stage sm_path
>        do
>                name=$(module_name "$sm_path")
> -               url=$(git config -f .gitmodules --get submodule."$name".url)
> +               # path from superproject origin repo to submodule origin repo
> +               module_url=$(git config -f .gitmodules --get submodule."$name".url)
>
>                # Possibly a url relative to parent
> -               case "$url" in
> +               case "$module_url" in
>                ./*|../*)
> -                       url=$(resolve_relative_url "$url") || exit
> +                       # rewrite foo/bar as ../.. to find path from
> +                       # submodule work tree to superproject work tree
> +                       up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
> +                       # guarantee a trailing /
> +                       up_path=${up_path%/}/ &&
> +                       # path from submodule work tree to submodule origin repo
> +                       sub_origin_url=$(resolve_relative_url "$module_url" "$up_path") &&
> +                       # path from superproject work tree to submodule origin repo
> +                       super_config_url=$(resolve_relative_url "$module_url") || exit
> +                       ;;
> +               *)
> +                       sub_origin_url="$module_url"
> +                       super_config_url="$module_url"
>                        ;;
>                esac
>
>                if git config "submodule.$name.url" >/dev/null 2>/dev/null
>                then
>                        say "$(eval_gettext "Synchronizing submodule url for '\$name'")"
> -                       git config submodule."$name".url "$url"
> +                       git config submodule."$name".url "$super_config_url"
>
>                        if test -e "$sm_path"/.git
>                        then
> @@ -979,7 +1024,7 @@ cmd_sync()
>                                clear_local_git_env
>                                cd "$sm_path"
>                                remote=$(get_default_remote)
> -                               git config remote."$remote".url "$url"
> +                               git config remote."$remote".url "$sub_origin_url"
>                        )
>                        fi
>                fi
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 08b8d2f..f2f907f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -507,12 +507,11 @@ test_expect_success 'relative path works with user@host:path' '
>        )
>  '
>
> -test_expect_failure 'relative path works with foo' '
> +test_expect_success 'relative path works with foo' '
>        (
>                cd reltest &&
>                cp pristine-.git-config .git/config &&
>                git config remote.origin.url foo &&
> -               # actual: fails with an error
>                git submodule init &&
>                test "$(git config submodule.sub.url)" = ./subrepo
>        )
> @@ -538,13 +537,12 @@ test_expect_success 'relative path works with ./foo' '
>        )
>  '
>
> -test_expect_failure 'relative path works with ./foo/bar' '
> +test_expect_success 'relative path works with ./foo/bar' '
>        (
>                cd reltest &&
>                cp pristine-.git-config .git/config &&
>                git config remote.origin.url ./foo/bar &&
>                git submodule init &&
> -               #actual: ./foo/subrepo
>                test "$(git config submodule.sub.url)" = foo/subrepo
>        )
>  '
> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index 9fa4e58..9526f8d 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -88,72 +88,66 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
>        )
>  '
>
> -test_expect_failure '"git submodule sync" handles origin URL of the form foo' '
> +test_expect_success '"git submodule sync" handles origin URL of the form foo' '
>        (cd relative-clone &&
>         git remote set-url origin foo
>         git submodule sync &&
>        (cd submodule &&
> -        #actual fails with: "cannot strip off url foo
>         git config remote.origin.url &&

Sorry, it appears I left some debug output in the tests. Will roll
this into a v6 with any updates you suggest.

Regards,

jon.

Re: [PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs

[Phil Hord]

On Wed, May 23, 2012 at 11:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> -# Resolve relative url by appending to parent's url
> +# Resolve relative url by appending the submodule url
> +# to the superproject's origin URL
> +#
> +# If the origin URL is itself a relative URL prepend
> +# an additional prefix, if present, that represents
> +# the relative path from the submodule's working tree
> +# to the superprojects' working tree.
> +#
> +# This behaviour is required to ensure that the origin URL

"Required behavior" always seems overstated to me when I
read it in comments and so I tend to distrust it.  I'd prefer to
see "This behaviour is intended to ensure..."  But this is
only my personal preference.


> +# of a submodule, when relative, is relative to the
> +# submodule's work tree and not to the superproject's work tree.
> +#
>  resolve_relative_url ()
>  {
>        remote=$(get_default_remote)
>        remoteurl=$(git config "remote.$remote.url") ||
>                remoteurl=$(pwd) # the repository is its own authoritative upstream
>        url="$1"
> +       up_path="$2"
> +
> +       #
> +       # ensure all relative paths begin with ./ to enable

Are we talking about remote urls or local filesystem paths?  I think this
is all confusing enough without the comments also using terminology
inconsistently.  Would it be more correct to say "all relatiive urls" here?
Or is this function only interested in local filesystem paths?

I realize that urls are paths of a different nature.  I pick this nit
only in the cause of clarity.

> +       # selection relative branch of subsequent case "$remoteurl"
> +       # statement.
> +       #
> +       # rewrites foo/bar to ./foo/bar but leaves /foo, :foo ./foo
> +       # and ../foo untouched.
> +       #

In many filesystems, ":foo" and "\foo" are valid filenames.
I suspect it is unwise to employ them and expect them not
to cause trouble, so I don't know if we should make
special efforts to accept them here.  But I think it
is worth noting.

On the other hand, I do not know how these special
characters are represented in urls.

> +       remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>        remoteurl=${remoteurl%/}
>        sep=/
>        while test -n "$url"
> @@ -45,6 +67,16 @@ resolve_relative_url ()
>                ../*)
>                        url="${url#../}"
>                        case "$remoteurl" in
> +                       .*/*)
> +                               # remove last part
> +                               remoteurl="${remoteurl%/*}"
> +                               # remove redundant leading ./
> +                               remoteurl="${remoteurl#./}"
> +                               # prefix path from submodule work tree to superproject work tree
> +                               remoteurl="${up_path}${remoteurl}"

Here we seem to be talking about paths since we are concerned
with the worktrees.  So maybe my earlier concern about "urls"
vs. "paths" was miguided.  My head swims.


> +                               # remove trailing /.
> +                               remoteurl="${remoteurl%/.}"
> +                               ;;
>                        */*)
>                                remoteurl="${remoteurl%/*}"
>                                ;;

I haven't tested it, but the rest of this is making more sense to me now.

Phil

Re: [PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs

[Jon Seymour]

On Fri, May 25, 2012 at 4:58 AM, Phil Hord <phil.hord@gmail.com> wrote:
> On Wed, May 23, 2012 at 11:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
>> +#
>> +# This behaviour is required to ensure that the origin URL
>
> "Required behavior" always seems overstated to me when I
> read it in comments and so I tend to distrust it.  I'd prefer to
> see "This behaviour is intended to ensure..."  But this is
> only my personal preference.

Sure.

>> +       #
>> +       # ensure all relative paths begin with ./ to enable
>

For clarity, this should be:

    ensure all relative superproject origin URLs begin with ./ to enable ...

> Are we talking about remote urls or local filesystem paths?  I think this
> is all confusing enough without the comments also using terminology
> inconsistently.  Would it be more correct to say "all relatiive urls" here?
> Or is this function only interested in local filesystem paths?
>
> I realize that urls are paths of a different nature.  I pick this nit
> only in the cause of clarity.

How about I rewrite the function blurb as something like:

"The function takes at most 2 arguments. The first argument is the
relative URL that navigates from the superproject origin repo to the
submodule origin repo. The second up_path argument, if specified, is
the relative path that navigates from the submodule working tree to
the superproject working tree.

The output of the function is the origin URL of the submodule.

The output will either be an absolute URL or filesystem path (if the
superproject origin URL is an absolute URL or filesystem path,
respectively) or a relative file system path (if the superproject
origin URL is a relative file system path).

When the output is a relative file system path, the path is either
relative to the submodule working tree, if up_path is specified, or to
the superproject working tree otherwise."

Phil/Jens: Perhaps we should rename the function to be:
submodule_origin_url to reflect its intended use?

>
>> +       # selection relative branch of subsequent case "$remoteurl"
>> +       # statement.
>> +       #
>> +       # rewrites foo/bar to ./foo/bar but leaves /foo, :foo ./foo
>> +       # and ../foo untouched.
>> +       #
>
> In many filesystems, ":foo" and "\foo" are valid filenames.
> I suspect it is unwise to employ them and expect them not
> to cause trouble, so I don't know if we should make
> special efforts to accept them here.  But I think it
> is worth noting.
>
> On the other hand, I do not know how these special
> characters are represented in urls.

Perhaps I should indicate 'why' these paths are not touched? Something like:

"Rewrites foo/bar foo/bar as ./foo/bar but leaves ./foo, ../foo, /foo,
foo:bar, :bar untouched because the first 2 forms do not require
additional qualification and the last forms
are assumed to be absolute URLs or paths."

Perhaps I shouldn't be treating :foo as a possible absolute file
system path ? My knowledge of filesystems is not good enough to know
if there are filesystems that git supports where :foo might be
regarded a valid absolute path?

>
>> +       remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>>        remoteurl=${remoteurl%/}
>>        sep=/
>>        while test -n "$url"
>> @@ -45,6 +67,16 @@ resolve_relative_url ()
>>                ../*)
>>                        url="${url#../}"
>>                        case "$remoteurl" in
>> +                       .*/*)
>> +                               # remove last part
>> +                               remoteurl="${remoteurl%/*}"
>> +                               # remove redundant leading ./
>> +                               remoteurl="${remoteurl#./}"
>> +                               # prefix path from submodule work tree to superproject work tree
>> +                               remoteurl="${up_path}${remoteurl}"
>
> Here we seem to be talking about paths since we are concerned
> with the worktrees.  So maybe my earlier concern about "urls"
> vs. "paths" was miguided.  My head swims.
>

This branch of the case statement is specifically when the origin
superproject URL is a relative file system path.

>
>> +                               # remove trailing /.
>> +                               remoteurl="${remoteurl%/.}"
>> +                               ;;
>>                        */*)
>>                                remoteurl="${remoteurl%/*}"
>>                                ;;
>
> I haven't tested it, but the rest of this is making more sense to me now.
>
> Phil

Thanks for your review and suggestions.

jon.

Re: [PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs

[Jon Seymour]

On Thu, May 24, 2012 at 1:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
>        sep=/
>        while test -n "$url"
> @@ -45,6 +67,16 @@ resolve_relative_url ()
>                ../*)
>                        url="${url#../}"
>                        case "$remoteurl" in
> +                       .*/*)
> +                               # remove last part
> +                               remoteurl="${remoteurl%/*}"
> +                               # remove redundant leading ./
> +                               remoteurl="${remoteurl#./}"
> +                               # prefix path from submodule work tree to superproject work tree
> +                               remoteurl="${up_path}${remoteurl}"
> +                               # remove trailing /.
> +                               remoteurl="${remoteurl%/.}"
> +                               ;;

Mmmm. Probably shouldn't be doing these _all_ of these edits on each
iteration of the while loop, methinks.

I'll write a test, and fix.

jon.