[PATCH v6 0/9] submodule: improve robustness of path handling

[PATCH v6 0/9] submodule: improve robustness of path handling

[Jon Seymour]

This series improves the robustness of path handling by 'git submodule' by:

* detecting submodule URLs that will result in non-sensical submodule origin URLs

* improving handling of various kinds of relative superproject origin URLs

* improving handling of various kinds of denormalized superproject origin URLs

This series differs from v5 in the following ways, by:

* Adding a more extensive set of failure tests to illustrate the conditions 
being addressed.

* Modifying the ../ processing loop in resolve_relative_url to exclude the 
'invariant' parts of absolute URLs from relative URL processing and thereby 
enable earlier and more accurate detection of edits that are going to 
produce a non-sensical output.

* Simplifying relative superproject origin URL support, by taking advantage of
the modifications above.

* Adding support for normalizing denormalized superproject origin URLs.

* Adding some additional regression tests to help guard against 
unintended regressions by this series.

* Improving the source code comments to better explain the purpose
of various code functions and code blocks

Jon Seymour (9):
  submodule: additional regression tests for relative URLs
  submodule: document failure to detect invalid submodule URLs
  submodule: document failure to handle relative superproject origin
    URLs
  submodule: document failure to handle improperly normalized remote
    origin URLs
  submodule: extract normalize_path into standalone function
  submodule: fix detection of invalid submodule URL
  submodule: fix sync handling of relative superproject origin URLs
  submodule: fix handling of denormalized superproject origin URLs
  submodule: fix normalization to handle repeated ./

 git-submodule.sh           | 118 +++++++++++++-----
 t/t7400-submodule-basic.sh | 297 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t7403-submodule-sync.sh  |  84 ++++++++++++-
 3 files changed, 468 insertions(+), 31 deletions(-)

-- 
1.7.10.2.656.gb5a46db

[PATCH v6 1/9] submodule: additional regression tests for relative URLs

[Jon Seymour]

Some additional tests are added to support regression testing of the changes in the
remainder of the series.

We also add a pristine copy of .gitmodules in anticipation of this being
required by later tests.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t7400-submodule-basic.sh | 110 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 3 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..9428c7a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -483,21 +483,67 @@ test_expect_success 'set up for relative path tests' '
 		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
+		cp .git/config pristine-.git-config &&
+		cp .gitmodules pristine-.gitmodules
 	)
 '
 
-test_expect_success 'relative path works with URL' '
+test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
 		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' '
+test_expect_success '../subrepo works with port-qualified URL - ssh://hostname:22/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname:22/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ssh://hostname:22/subrepo
+	)
+'
+
+test_expect_success '../subrepo path works with local path - /foo/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url /foo/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = /foo/subrepo
+	)
+'
+
+test_expect_success '../subrepo works with file URL - file:///tmp/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url file:///tmp/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = file:///tmp/subrepo
+	)
+'
+
+test_expect_success '../subrepo works with helper URL- helper:://hostname/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url helper:://hostname/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = helper:://hostname/subrepo
+	)
+'
+
+test_expect_success '../subrepo works with scp-style URL - user@host:repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -507,6 +553,64 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_success '../subrepo works with scp-style URL - user@host:path/to/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url user@host:path/to/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = user@host:path/to/subrepo
+	)
+'
+
+test_expect_success '../subrepo works with relative local path - foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url foo/bar &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = foo/subrepo
+	)
+'
+
+test_expect_success '../subrepo works with relative local path - ../foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ../foo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../subrepo
+	)
+'
+
+test_expect_success '../subrepo works with relative local path - ../foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ../foo/bar &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../foo/subrepo
+	)
+'
+
+test_expect_success '../bar/a/b/c works with relative local path - ../foo/bar.git' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		mkdir -p a/b/c &&
+		(cd a/b/c; git init) &&
+		git config remote.origin.url ../foo/bar.git &&
+		git submodule add ../bar/a/b/c ./a/b/c &&
+		git submodule init &&
+		test "$(git config submodule.a/b/c.url)" = ../foo/bar/a/b/c
+	)
+'
+
 test_expect_success 'moving the superproject does not break submodules' '
 	(
 		cd addtest &&
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 2/9] submodule: document failure to detect invalid submodule URLs

[Jon Seymour]

These tests document failures to detect submodule URLs that backtrack
past the 'invariant' part of a superproject's origin URL.

For example: if the origin URL is ssh://hostname/repo, then currently
if a submodule URL is specified as ../../subrepo, then git will
construct a submodule url of the form ssh://subrepo/ without complaint.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t7400-submodule-basic.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 9428c7a..a758c63 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -543,10 +543,80 @@ test_expect_success '../subrepo works with helper URL- helper:://hostname/repo'
 	)
 '
 
+test_expect_failure '../../subrepo fails with URL - ssh://hostname/repo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/repo &&
+		git config -f .gitmodules submodule.sub.url ../../subrepo &&
+		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
+		test_must_fail git submodule init 2>actual &&
+		#actual no failure, url configured as ssh://subrepo
+		test_cmp expected actual
+	)
+"
+
+test_expect_failure '../../subrepo fails with absolute local path - /repo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url /repo &&
+		git config -f .gitmodules submodule.sub.url ../../subrepo &&
+		echo cannot strip one component off url \'/\' > expected &&
+		test_must_fail git submodule init 2>actual &&
+		test_cmp expected actual
+	)
+"
+
+test_expect_failure '../../../subrepo fails with URL - ssh://hostname/repo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/repo &&
+		git config -f .gitmodules submodule.sub.url ../../../subrepo &&
+		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
+		test_must_fail git submodule init 2>actual &&
+		#actual no failure, url configured as ssh:/subrepo
+		test_cmp expected actual
+	)
+"
+
+test_expect_failure '../../../../subrepo fails with with URL - ssh://hostname/repo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/repo &&
+		git config -f .gitmodules submodule.sub.url ../../../../subrepo &&
+		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
+		test_must_fail git submodule init 2>actual &&
+		#actual no failure, url configured as ssh:/subrepo
+		test_cmp expected actual
+	)
+"
+
+test_expect_failure '../../../../../subrepo fails with URL - ssh://hostname/repo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/repo &&
+		git config -f .gitmodules submodule.sub.url ../../../../../subrepo &&
+		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
+		test_must_fail git submodule init 2>actual &&
+		#actual cannot strip one component off url 'ssh'
+		test_cmp expected actual
+	)
+"
+
 test_expect_success '../subrepo works with scp-style URL - user@host:repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
 		git config remote.origin.url user@host:repo &&
 		git submodule init &&
 		test "$(git config submodule.sub.url)" = user@host:subrepo
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 3/9] submodule: document failure to handle relative superproject origin URLs

[Jon Seymour]

This test case documents several cases where handling of relative
superproject origin URLs doesn't produce an expected result.

submodule.{sub}.url in the superproject is incorrect in these cases:
  foo
  ./foo
  ./foo/bar

The remote.origin.url of the submodule is incorrect in the above cases
and also when the superproject origin URL is like:
  foo/bar
  ../foo
  ../foo/bar

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

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a758c63..80ec0f7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -634,6 +634,18 @@ test_expect_success '../subrepo works with scp-style URL - user@host:path/to/rep
 	)
 '
 
+test_expect_failure '../subrepo works with relative local path - foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url foo &&
+		# actual: fails with an error
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = subrepo
+	)
+'
+
 test_expect_success '../subrepo works with relative local path - foo/bar' '
 	(
 		cd reltest &&
@@ -645,6 +657,30 @@ test_expect_success '../subrepo works with relative local path - foo/bar' '
 	)
 '
 
+test_expect_failure '../subrepo works with relative local path - ./foo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ./foo &&
+		git submodule init &&
+		#actual ./subrepo
+		test "$(git config submodule.sub.url)" = subrepo
+	)
+'
+
+test_expect_failure '../subrepo works with relative local path - ./foo/bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ./foo/bar &&
+		git submodule init &&
+		#actual: ./foo/subrepo
+		test "$(git config submodule.sub.url)" = foo/subrepo
+	)
+'
+
 test_expect_success '../subrepo works with relative local path - ../foo' '
 	(
 		cd reltest &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 3620215..56b933d 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,90 @@ 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
+	 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
+	 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
+	 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
+	 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
+	 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
+	 test "$(git config remote.origin.url)" = "../../foo/submodule"
+	)
+	)
+'
+
+test_expect_failure '"git submodule sync" handles origin URL of the form ../foo/bar with deeply nested submodule' '
+	(cd relative-clone &&
+	 git remote set-url origin ../foo/bar &&
+	 mkdir -p a/b/c &&
+	 ( cd a/b/c &&
+	   git init &&
+	   :> .gitignore &&
+	   git add .gitignore &&
+	   test_tick &&
+	   git commit -m "initial commit" ) &&
+	 git submodule add ../bar/a/b/c ./a/b/c &&
+	 git submodule sync &&
+	(cd a/b/c &&
+	 #actual ../foo/bar/a/b/c
+	 test "$(git config remote.origin.url)" = "../../../../foo/bar/a/b/c"
+	)
+	)
+'
+
+
 test_done
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 4/9] submodule: document failure to handle improperly normalized remote origin URLs

[Jon Seymour]

These tests document failures to properly handle improperly normalized
remote origin URLs.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t7400-submodule-basic.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 80ec0f7..2674088 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -499,6 +499,39 @@ test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
 	)
 '
 
+test_expect_failure 'relative path works with URL - ssh://hostname/path/././repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/path/././repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ssh://hostname/path/subrepo
+	)
+'
+
+test_expect_failure 'relative path works with URL - ssh://hostname/path/detour/././../repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/path/detour/././../repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ssh://hostname/path/subrepo
+	)
+'
+
+test_expect_failure 'relative path works with URL - ssh://hostname/path/repo/.' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ssh://hostname/path/repo/. &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ssh://hostname/path/subrepo
+	)
+'
+
 test_expect_success '../subrepo works with port-qualified URL - ssh://hostname:22/repo' '
 	(
 		cd reltest &&
@@ -634,6 +667,50 @@ test_expect_success '../subrepo works with scp-style URL - user@host:path/to/rep
 	)
 '
 
+test_expect_failure 'relative path works with user@host:path/to/repo/.' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url user@host:path/to/repo/. &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = user@host:path/to/subrepo
+	)
+'
+
+test_expect_failure 'relative path works with user@host:path/to/./repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url user@host:path/to/./repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = user@host:path/to/subrepo
+	)
+'
+
+test_expect_failure 'relative path works with user@host:path/to/././repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url user@host:path/to/././repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = user@host:path/to/subrepo
+	)
+'
+
+test_expect_failure 'relative path works with user@host:path/to/detour/../repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url user@host:path/to/detour/../repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = user@host:path/to/subrepo
+	)
+'
+
 test_expect_failure '../subrepo works with relative local path - foo' '
 	(
 		cd reltest &&
@@ -703,6 +780,17 @@ test_expect_success '../subrepo works with relative local path - ../foo/bar' '
 	)
 '
 
+test_expect_failure 'relative path works with ../foo/./bar' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url ../foo/./bar &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../foo/subrepo
+	)
+'
+
 test_expect_success '../bar/a/b/c works with relative local path - ../foo/bar.git' '
 	(
 		cd reltest &&
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 5/9] submodule: extract normalize_path into standalone function

[Jon Seymour]

Extract the normalize_path function so that it can be re-used elsewhere.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 64a70d6..dbbc905 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -176,6 +176,21 @@ module_clone()
 	(clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
+normalize_path()
+{
+	# normalize path:
+	# multiple //; leading ./; /./; /../; trailing /
+	printf '%s/\n' "$1" |
+		sed -e '
+			s|//*|/|g
+			s|^\(\./\)*||
+			s|/\./|/|g
+			:start
+			s|\([^/]*\)/\.\./||
+			tstart
+			s|/*$||
+		'
+}
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
@@ -250,18 +265,7 @@ 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|/*$||
-		')
+	sm_path="$(normalize_path "$sm_path")"
 	git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
 	die "$(eval_gettext "'\$sm_path' already exists in the index")"
 
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 6/9] submodule: fix detection of invalid submodule URL

[Jon Seymour]

Currently the superproject origin URL is progressively transformed
by stepping through parts of the submodule URL and removing parts
from the superproject URL for each leading ../ found in the
submodule URL. No attempt is made to check that the edited URL still
has a path part left to remove. This can result in the construction
of an absolute submodule URL where the hostname part of the URL
has been replaced by path components of the submodule URL.

For example: if the origin URL is ssh://hostname/repo and the
submodule URL is ../../subrepo, then the origin URL of the subrepo
will be calculated as ssh://subrepo.

With this change, editing is only performed on the path part
of the superproject origin URL. Any attempt by to consume the
non-path parts of the origin URL results in a failure.

As a side effect of preserving correct handling of support for
URLs of the form user@host:repo, this change also fixes handling,
by submodule init, of origin super project URLs of the form:
foo, foo/bar, ./foo and ./foo/bar.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh           | 41 ++++++++++++++++++++++++++++++++---------
 t/t7400-submodule-basic.sh | 23 ++++++++---------------
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index dbbc905..2550681 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -37,23 +37,42 @@ resolve_relative_url ()
 	remoteurl=$(git config "remote.$remote.url") ||
 		remoteurl=$(pwd) # the repository is its own authoritative upstream
 	url="$1"
-	remoteurl=${remoteurl%/}
-	sep=/
+	remoteurl="${remoteurl%/}"
+
+	case "$remoteurl" in
+		*//*/*)
+			variant="${remoteurl#*//*/}"
+		;;
+		*::*)
+			variant="${remoteurl#*::}"
+		;;
+		*:*)
+			variant="${remoteurl#*:}"
+		;;
+		/*)
+			variant="${remoteurl#/}"
+		;;
+		*)
+			variant="${remoteurl}"
+		;;
+	esac
+	invariant="${remoteurl%$variant}"
+
 	while test -n "$url"
 	do
 		case "$url" in
 		../*)
 			url="${url#../}"
-			case "$remoteurl" in
+			case "$variant" in
 			*/*)
-				remoteurl="${remoteurl%/*}"
+				variant="${variant%/*}"
 				;;
-			*:*)
-				remoteurl="${remoteurl%:*}"
-				sep=:
+			.)
+				die "$(eval_gettext "cannot strip one component off url '\${invariant}'")"
 				;;
 			*)
-				die "$(eval_gettext "cannot strip one component off url '\$remoteurl'")"
+				# add a sentinel when .. matchs foo
+				variant=.
 				;;
 			esac
 			;;
@@ -64,7 +83,11 @@ resolve_relative_url ()
 			break;;
 		esac
 	done
-	echo "$remoteurl$sep${url%/}"
+	# ensure a trailing path separator
+	variant="${variant}/"
+	# strip the sentinel, if present
+	variant="${variant#./}"
+	echo "$invariant$variant${url%/}"
 }
 
 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2674088..a94c5e9 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -576,7 +576,7 @@ test_expect_success '../subrepo works with helper URL- helper:://hostname/repo'
 	)
 '
 
-test_expect_failure '../../subrepo fails with URL - ssh://hostname/repo' "
+test_expect_success '../../subrepo fails with URL - ssh://hostname/repo' "
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -585,12 +585,11 @@ test_expect_failure '../../subrepo fails with URL - ssh://hostname/repo' "
 		git config -f .gitmodules submodule.sub.url ../../subrepo &&
 		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
 		test_must_fail git submodule init 2>actual &&
-		#actual no failure, url configured as ssh://subrepo
 		test_cmp expected actual
 	)
 "
 
-test_expect_failure '../../subrepo fails with absolute local path - /repo' "
+test_expect_success '../../subrepo fails with absolute local path - /repo' "
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -603,7 +602,7 @@ test_expect_failure '../../subrepo fails with absolute local path - /repo' "
 	)
 "
 
-test_expect_failure '../../../subrepo fails with URL - ssh://hostname/repo' "
+test_expect_success '../../../subrepo fails with URL - ssh://hostname/repo' "
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -612,12 +611,11 @@ test_expect_failure '../../../subrepo fails with URL - ssh://hostname/repo' "
 		git config -f .gitmodules submodule.sub.url ../../../subrepo &&
 		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
 		test_must_fail git submodule init 2>actual &&
-		#actual no failure, url configured as ssh:/subrepo
 		test_cmp expected actual
 	)
 "
 
-test_expect_failure '../../../../subrepo fails with with URL - ssh://hostname/repo' "
+test_expect_success '../../../../subrepo fails with with URL - ssh://hostname/repo' "
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -626,12 +624,11 @@ test_expect_failure '../../../../subrepo fails with with URL - ssh://hostname/re
 		git config -f .gitmodules submodule.sub.url ../../../../subrepo &&
 		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
 		test_must_fail git submodule init 2>actual &&
-		#actual no failure, url configured as ssh:/subrepo
 		test_cmp expected actual
 	)
 "
 
-test_expect_failure '../../../../../subrepo fails with URL - ssh://hostname/repo' "
+test_expect_success '../../../../../subrepo fails with URL - ssh://hostname/repo' "
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -640,7 +637,6 @@ test_expect_failure '../../../../../subrepo fails with URL - ssh://hostname/repo
 		git config -f .gitmodules submodule.sub.url ../../../../../subrepo &&
 		echo cannot strip one component off url \'ssh://hostname/\' > expected &&
 		test_must_fail git submodule init 2>actual &&
-		#actual cannot strip one component off url 'ssh'
 		test_cmp expected actual
 	)
 "
@@ -711,13 +707,12 @@ test_expect_failure 'relative path works with user@host:path/to/detour/../repo'
 	)
 '
 
-test_expect_failure '../subrepo works with relative local path - foo' '
+test_expect_success '../subrepo works with relative local path - foo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
 		cp pristine-.gitmodules .gitmodules &&
 		git config remote.origin.url foo &&
-		# actual: fails with an error
 		git submodule init &&
 		test "$(git config submodule.sub.url)" = subrepo
 	)
@@ -734,26 +729,24 @@ test_expect_success '../subrepo works with relative local path - foo/bar' '
 	)
 '
 
-test_expect_failure '../subrepo works with relative local path - ./foo' '
+test_expect_success '../subrepo works with relative local path - ./foo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
 		cp pristine-.gitmodules .gitmodules &&
 		git config remote.origin.url ./foo &&
 		git submodule init &&
-		#actual ./subrepo
 		test "$(git config submodule.sub.url)" = subrepo
 	)
 '
 
-test_expect_failure '../subrepo works with relative local path - ./foo/bar' '
+test_expect_success '../subrepo works with relative local path - ./foo/bar' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
 		cp pristine-.gitmodules .gitmodules &&
 		git config remote.origin.url ./foo/bar &&
 		git submodule init &&
-		#actual: ./foo/subrepo
 		test "$(git config submodule.sub.url)" = foo/subrepo
 	)
 '
-- 
1.7.10.2.656.gb5a46db

[PATCH v6 7/9] submodule: fix sync handling of relative superproject origin URLs

[Jon Seymour]

When the origin URL of the superproject is itself relative, git sync
configures the remote.origin.url configuration property of the submodule
with a path that is relative to the work tree of the super project
rather than the work tree of the submodule.

To fix this an 'up_path' that navigates from the work tree of the submodule
to the work tree of the supermodule needs to be prepended to the URL
othrewise calculated.

This change fixes handling for relative superproject origin URLs like
the following:
      foo
      foo/bar
      ./foo
      ./foo/bar
      ../foo
      ../foo/bar

This change also renames the url variable used by git sync to module_url
and introduces to new variables super_config_url and sub_origin_url to refer
to the different URLs derived from the module_url.

The function blurb is expanded to give a more thorough description of what
resolve_relative_url()'s function is intended to be.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh          | 48 ++++++++++++++++++++++++++++++++++++++++-------
 t/t7403-submodule-sync.sh | 23 ++++++++---------------
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2550681..9ca2ffe 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -30,14 +30,32 @@ nofetch=
 update=
 prefix=
 
-# Resolve relative url by appending to parent's url
+# 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.
 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"
+
 	remoteurl="${remoteurl%/}"
+	is_relative=
 
 	case "$remoteurl" in
 		*//*/*)
@@ -54,6 +72,7 @@ resolve_relative_url ()
 		;;
 		*)
 			variant="${remoteurl}"
+			is_relative=t
 		;;
 	esac
 	invariant="${remoteurl%$variant}"
@@ -83,11 +102,13 @@ resolve_relative_url ()
 			break;;
 		esac
 	done
+
 	# ensure a trailing path separator
 	variant="${variant}/"
 	# strip the sentinel, if present
 	variant="${variant#./}"
-	echo "$invariant$variant${url%/}"
+
+	echo "$invariant${is_relative:+$up_path}$variant${url%/}"
 }
 
 #
@@ -986,19 +1007,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
@@ -1006,7 +1040,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/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 56b933d..b7466ba 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -88,73 +88,67 @@ 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
 	 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
 	 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
 	 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
 	 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
 	 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
 	 test "$(git config remote.origin.url)" = "../../foo/submodule"
 	)
 	)
 '
 
-test_expect_failure '"git submodule sync" handles origin URL of the form ../foo/bar with deeply nested submodule' '
+test_expect_success '"git submodule sync" handles origin URL of the form ../foo/bar with deeply nested submodule' '
 	(cd relative-clone &&
 	 git remote set-url origin ../foo/bar &&
 	 mkdir -p a/b/c &&
@@ -167,11 +161,10 @@ test_expect_failure '"git submodule sync" handles origin URL of the form ../foo/
 	 git submodule add ../bar/a/b/c ./a/b/c &&
 	 git submodule sync &&
 	(cd a/b/c &&
-	 #actual ../foo/bar/a/b/c
 	 test "$(git config remote.origin.url)" = "../../../../foo/bar/a/b/c"
 	)
 	)
 '
 
-
 test_done
+<
-- 
1.7.10.2.656.gb5a46db

Re: [PATCH v6 7/9] submodule: fix sync handling of relative superproject origin URLs

[Jon Seymour]

On Sun, May 27, 2012 at 11:43 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> When the origin URL of the superproject is itself relative, git sync
> configures the remote.origin.url configuration property of the submodule
> with a path that is relative to the work tree of the super project

sorry! super project -> superproject

> rather than the work tree of the submodule.
>
> To fix this an 'up_path' that navigates from the work tree of the submodule
> to the work tree of the supermodule needs to be prepended to the URL

sorry! supermodule -> superproject.

[PATCH v6 8/9] submodule: fix handling of denormalized superproject origin URLs

[Jon Seymour]

Currently git calculates the submodule origin URL incorrectly in
the case that the superproject origin URL is denormalized.

So, we normalize the path part of the superproject URL before iterating
over the leading ../ parts of the submodule URL.

A remaining problem related to the handling of consecutive repeated ./'s
in the superproject origin URL is deferred to a subsequent commit.

This change also fixes a subtle error in the setup of t7403 which was
masked by the denormalization issue. Previous behaviour was
relying on submodule add to clone trash/submodule into super/submodule,
however from the perspective of super's origin (i.e. trash), the origin
submodule is actually located at ./submodule not ../submodule. However,
because the origin URL of super was denormalized (it had a trailing /.)
the incorrect handling of denormalized super URLs actually produced
the correct result - a case of two errors cancelling out each other's
effects.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh           |  1 +
 t/t7400-submodule-basic.sh | 10 +++++-----
 t/t7403-submodule-sync.sh  |  1 +
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9ca2ffe..1f0983c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -76,6 +76,7 @@ resolve_relative_url ()
 		;;
 	esac
 	invariant="${remoteurl%$variant}"
+	variant="$(normalize_path "$variant")"
 
 	while test -n "$url"
 	do
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a94c5e9..b01f479 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -521,7 +521,7 @@ test_expect_failure 'relative path works with URL - ssh://hostname/path/detour/.
 	)
 '
 
-test_expect_failure 'relative path works with URL - ssh://hostname/path/repo/.' '
+test_expect_success 'relative path works with URL - ssh://hostname/path/repo/.' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -663,7 +663,7 @@ test_expect_success '../subrepo works with scp-style URL - user@host:path/to/rep
 	)
 '
 
-test_expect_failure 'relative path works with user@host:path/to/repo/.' '
+test_expect_success 'relative path works with user@host:path/to/repo/.' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -674,7 +674,7 @@ test_expect_failure 'relative path works with user@host:path/to/repo/.' '
 	)
 '
 
-test_expect_failure 'relative path works with user@host:path/to/./repo' '
+test_expect_success 'relative path works with user@host:path/to/./repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -696,7 +696,7 @@ test_expect_failure 'relative path works with user@host:path/to/././repo' '
 	)
 '
 
-test_expect_failure 'relative path works with user@host:path/to/detour/../repo' '
+test_expect_success 'relative path works with user@host:path/to/detour/../repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -773,7 +773,7 @@ test_expect_success '../subrepo works with relative local path - ../foo/bar' '
 	)
 '
 
-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 &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index b7466ba..0152969 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
 	git clone . super &&
 	git clone super submodule &&
 	(cd super &&
+	 git clone ../submodule submodule &&
 	 git submodule add ../submodule submodule &&
 	 test_tick &&
 	 git commit -m "submodule"
-- 
1.7.10.2.656.gb5a46db

Re: [PATCH v6 8/9] submodule: fix handling of denormalized superproject origin URLs

[Jon Seymour]

On Sun, May 27, 2012 at 11:43 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> This change also fixes a subtle error in the setup of t7403 which was
> masked by the denormalization issue. Previous behaviour was
> relying on submodule add to clone trash/submodule into super/submodule,
> however from the perspective of super's origin (i.e. trash), the origin
> submodule is actually located at ./submodule not ../submodule. However,
> because the origin URL of super was denormalized (it had a trailing /.)
> the incorrect handling of denormalized super URLs actually produced
> the correct result - a case of two errors cancelling out each other's
> effects.
>

Forgot to check the other test scripts for a similar issue. This
change breaks the following tests:

   t7406-submodule-update.sh
   t7407-submodule-foreach.sh
   t7506-status-submodule.sh

possibly because of the same issue mentioned in regard to t7403.

In any case, better not to apply this to pu until I have had a chance
to re-roll a v7

jon.

[PATCH v6 9/9] submodule: fix normalization to handle repeated ./

[Jon Seymour]

Currently path/./foo/./bar is denormalized correctly, but path/foo/././bar
is not.

We fix the normalization script to allow repeated application of the
./ -> / normalization in the same way that foo/.. is handled - by
moving it inside a sed loop.

The existing sed label, start, is renamed to fooslashdotdotslash to
indicate which of the two loops is being refered to by the second
branch directive.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-submodule.sh           | 8 +++++---
 t/t7400-submodule-basic.sh | 6 +++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 1f0983c..8f3bc71 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -229,10 +229,12 @@ normalize_path()
 		sed -e '
 			s|//*|/|g
 			s|^\(\./\)*||
-			s|/\./|/|g
-			:start
+			:slashdotslash
+			s|/\./|/|
+			tslashdotslash
+			:fooslashdotdotslash
 			s|\([^/]*\)/\.\./||
-			tstart
+			tfooslashdotdotslash
 			s|/*$||
 		'
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b01f479..61887b2 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -499,7 +499,7 @@ test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
 	)
 '
 
-test_expect_failure 'relative path works with URL - ssh://hostname/path/././repo' '
+test_expect_success 'relative path works with URL - ssh://hostname/path/././repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -510,7 +510,7 @@ test_expect_failure 'relative path works with URL - ssh://hostname/path/././repo
 	)
 '
 
-test_expect_failure 'relative path works with URL - ssh://hostname/path/detour/././../repo' '
+test_expect_success 'relative path works with URL - ssh://hostname/path/detour/././../repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
@@ -685,7 +685,7 @@ test_expect_success 'relative path works with user@host:path/to/./repo' '
 	)
 '
 
-test_expect_failure 'relative path works with user@host:path/to/././repo' '
+test_expect_success 'relative path works with user@host:path/to/././repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&
-- 
1.7.10.2.656.gb5a46db