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

[PATCH v4 0/2] 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 handling of relative superproject origin URLs
  submodule: fix handling of relative superproject origin URLs

 git-submodule.sh           | 17 ++++++++++--
 t/t7400-submodule-basic.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
 t/t7403-submodule-sync.sh  | 64 +++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 138 insertions(+), 3 deletions(-)

-- 
1.7.10.2.594.g24e850d

[PATCH v4 1/2] submodule: document handling of 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  | 63 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..97e7a73 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_success 'relative path works with foo' "
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url foo &&
+		echo \"cannot strip one component off url 'foo'\" >expect &&
+		test_must_fail git submodule init 2>actual &&
+		cat actual &&
+		test_cmp expect actual
+	)
+"
+
+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_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_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..3784c9b 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,63 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
+test_expect_success '"git submodule sync" handles origin URL of the form foo' "
+	(cd relative-clone &&
+	 git remote set-url origin foo
+	 echo \"cannot strip one component off url 'foo'\" > expect &&
+	 test_must_fail git submodule sync 2> actual &&
+	 test_cmp expect actual
+	)
+"
+
+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 &&
+	 test "$(git config remote.origin.url)" == "foo/submodule"
+	)
+	)
+'
+
+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 &&
+	 test "$(git config remote.origin.url)" == "./submodule"
+	)
+	)
+'
+
+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 &&
+	 test "$(git config remote.origin.url)" == "./foo/submodule"
+	)
+	)
+'
+
+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 &&
+	 test "$(git config remote.origin.url)" == "../submodule"
+	)
+	)
+'
+
+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 &&
+	 test "$(git config remote.origin.url)" == "../foo/submodule"
+	)
+	)
+'
+
 test_done
-- 
1.7.10.2.594.g24e850d

[PATCH v4 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           | 17 +++++++++++++++--
 t/t7400-submodule-basic.sh | 12 +++++-------
 t/t7403-submodule-sync.sh  | 21 +++++++++++----------
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 64a70d6..3e943de 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -37,6 +37,8 @@ resolve_relative_url ()
 	remoteurl=$(git config "remote.$remote.url") ||
 		remoteurl=$(pwd) # the repository is its own authoritative upstream
 	url="$1"
+	prefix="$2"
+	remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
 	remoteurl=${remoteurl%/}
 	sep=/
 	while test -n "$url"
@@ -45,6 +47,11 @@ resolve_relative_url ()
 		../*)
 			url="${url#../}"
 			case "$remoteurl" in
+			.*/*)
+				remoteurl="${remoteurl%/*}"
+				remoteurl="${remoteurl#./}"
+				remoteurl="${prefix}${remoteurl}"
+				;;
 			*/*)
 				remoteurl="${remoteurl%/*}"
 				;;
@@ -64,7 +71,7 @@ resolve_relative_url ()
 			break;;
 		esac
 	done
-	echo "$remoteurl$sep${url%/}"
+	echo "${remoteurl%/.}$sep${url%/}"
 }
 
 #
@@ -964,8 +971,14 @@ cmd_sync()
 		# Possibly a url relative to parent
 		case "$url" in
 		./*|../*)
+			up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
+			up_path=${up_path%/}/ &&
+			remoteurl=$(resolve_relative_url "$url" "$up_path") &&
 			url=$(resolve_relative_url "$url") || exit
 			;;
+		*)
+			remoteurl="$url"
+			;;
 		esac
 
 		if git config "submodule.$name.url" >/dev/null 2>/dev/null
@@ -979,7 +992,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 "$remoteurl"
 			)
 			fi
 		fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 97e7a73..f2f907f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -507,17 +507,15 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
-test_expect_success '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 &&
-		echo \"cannot strip one component off url 'foo'\" >expect &&
-		test_must_fail git submodule init 2>actual &&
-		cat actual &&
-		test_cmp expect actual
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ./subrepo
 	)
-"
+'
 
 test_expect_success 'relative path works with foo/bar' '
 	(
@@ -545,7 +543,7 @@ test_expect_success 'relative path works with ./foo/bar' '
 		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 "$(git config submodule.sub.url)" = foo/subrepo
 	)
 '
 
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 3784c9b..583fe21 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -88,21 +88,22 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
-test_expect_success '"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
-	 echo \"cannot strip one component off url 'foo'\" > expect &&
-	 test_must_fail git submodule sync 2> actual &&
-	 test_cmp expect actual
+	 git submodule sync &&
+	(cd submodule &&
+	 test "$(git config remote.origin.url)" == "../submodule"
 	)
-"
+	)
+'
 
 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 &&
-	 test "$(git config remote.origin.url)" == "foo/submodule"
+	 test "$(git config remote.origin.url)" == "../foo/submodule"
 	)
 	)
 '
@@ -112,7 +113,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo'
 	 git remote set-url origin ./foo
 	 git submodule sync &&
 	(cd submodule &&
-	 test "$(git config remote.origin.url)" == "./submodule"
+	 test "$(git config remote.origin.url)" == "../submodule"
 	)
 	)
 '
@@ -122,7 +123,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo/b
 	 git remote set-url origin ./foo/bar
 	 git submodule sync &&
 	(cd submodule &&
-	 test "$(git config remote.origin.url)" == "./foo/submodule"
+	 test "$(git config remote.origin.url)" == "../foo/submodule"
 	)
 	)
 '
@@ -132,7 +133,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo'
 	 git remote set-url origin ../foo
 	 git submodule sync &&
 	(cd submodule &&
-	 test "$(git config remote.origin.url)" == "../submodule"
+	 test "$(git config remote.origin.url)" == "../../submodule"
 	)
 	)
 '
@@ -142,7 +143,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo/
 	 git remote set-url origin ../foo/bar
 	 git submodule sync &&
 	(cd submodule &&
-	 test "$(git config remote.origin.url)" == "../foo/submodule"
+	 test "$(git config remote.origin.url)" == "../../foo/submodule"
 	)
 	)
 '
-- 
1.7.10.2.594.g24e850d

Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs

[Jens Lehmann]

Maybe the subject should rather be:
"submodule: document failures handling relative superproject origin URLs"

Am 23.05.2012 18:45, schrieb 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

Nice test coverage!

> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  t/t7403-submodule-sync.sh  | 63 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 81827e6..97e7a73 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' '
>  	)
>  '

But please use test_expect_failure for all tests that fail due to
current bugs and document what should succeed in the commands used
inside the test case (just like you did in v3). The only change
needed when the bug is fixed should be changing each
test_expect_failure to test_expect_success.

> +test_expect_success 'relative path works with foo' "
> +	(
> +		cd reltest &&
> +		cp pristine-.git-config .git/config &&
> +		git config remote.origin.url foo &&
> +		echo \"cannot strip one component off url 'foo'\" >expect &&
> +		test_must_fail git submodule init 2>actual &&
> +		cat actual &&
> +		test_cmp expect actual
> +	)
> +"
> +
> +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_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_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..3784c9b 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,63 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
>  	)
>  '
>  
> +test_expect_success '"git submodule sync" handles origin URL of the form foo' "
> +	(cd relative-clone &&
> +	 git remote set-url origin foo
> +	 echo \"cannot strip one component off url 'foo'\" > expect &&
> +	 test_must_fail git submodule sync 2> actual &&
> +	 test_cmp expect actual
> +	)
> +"
> +
> +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 &&
> +	 test "$(git config remote.origin.url)" == "foo/submodule"
> +	)
> +	)
> +'
> +
> +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 &&
> +	 test "$(git config remote.origin.url)" == "./submodule"
> +	)
> +	)
> +'
> +
> +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 &&
> +	 test "$(git config remote.origin.url)" == "./foo/submodule"
> +	)
> +	)
> +'
> +
> +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 &&
> +	 test "$(git config remote.origin.url)" == "../submodule"
> +	)
> +	)
> +'
> +
> +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 &&
> +	 test "$(git config remote.origin.url)" == "../foo/submodule"
> +	)
> +	)
> +'
> +
>  test_done

Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Maybe the subject should rather be:
> "submodule: document failures handling relative superproject origin URLs"
>
> Am 23.05.2012 18:45, schrieb 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
>
> Nice test coverage!

I recall correctly, the original use case for relative URL entries in the
.gitmodules file (to be copied to .git/config as submodule.$name.path) was
so that by looking at the top-level, the locations of the origins for
submodule repositories can be known from where the top-level was cloned.
The above cases do not seem to be relevant, so in the sense, they are of
secondary importance (and I do not find the "sneakernet tool" example
convincing---the sneakernet tool that is distributed in the scenario can
be written differently so that it does not require the other repositories
to be named relative to it).

As long as you and submodule stakeholders believe this is a reasonable
addition and does not break the existing use cases, I am perfectly fine
with it, though.

>> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
>> ---
>>  t/t7400-submodule-basic.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++
>>  t/t7403-submodule-sync.sh  | 63 +++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 124 insertions(+), 1 deletion(-)
>> 
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 81827e6..97e7a73 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' '
>>  	)
>>  '
>
> But please use test_expect_failure for all tests that fail due to
> current bugs and document what should succeed in the commands used
> inside the test case (just like you did in v3). The only change
> needed when the bug is fixed should be changing each
> test_expect_failure to test_expect_success.

Very true.

Thanks for a review.

Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs

[Jens Lehmann]

Am 23.05.2012 23:14, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> Am 23.05.2012 18:45, schrieb 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
>>
>> Nice test coverage!
> 
> I recall correctly, the original use case for relative URL entries in the
> .gitmodules file (to be copied to .git/config as submodule.$name.path) was
> so that by looking at the top-level, the locations of the origins for
> submodule repositories can be known from where the top-level was cloned.
> The above cases do not seem to be relevant, so in the sense, they are of
> secondary importance (and I do not find the "sneakernet tool" example
> convincing---the sneakernet tool that is distributed in the scenario can
> be written differently so that it does not require the other repositories
> to be named relative to it).

Remember this patch series is not about relative /submodule/ urls but
relative /superproject/ urls, so the .gitmodules file is not involved
here. But while reviewing 2/2 I started to suspect I was a bit too hasty
judging the coverage, as I realized only "git submodule init" and "git
submodule sync" are tested. "git submodule add" should also be tested
here to make sure it behaves well too (and only when the tests cases are
not split across two commits they can be properly reviewed). My remark
aimed at the attempt to test all possible relative paths in at least two
depths, which I deem very helpful to find problems early.

> As long as you and submodule stakeholders believe this is a reasonable
> addition and does not break the existing use cases, I am perfectly fine
> with it, though.

Me thinks relative /superproject/ urls starting with ./ or ../ should
work just fine (which they don't right now). And if relative superproject
urls inside the superproject are handled too without too much hassle I
won't argue against it, even though I'm not convinced of the use case
either.

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

[Jens Lehmann]

Am 23.05.2012 18:45, schrieb 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           | 17 +++++++++++++++--
>  t/t7400-submodule-basic.sh | 12 +++++-------
>  t/t7403-submodule-sync.sh  | 21 +++++++++++----------
>  3 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 64a70d6..3e943de 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -37,6 +37,8 @@ resolve_relative_url ()
>  	remoteurl=$(git config "remote.$remote.url") ||
>  		remoteurl=$(pwd) # the repository is its own authoritative upstream
>  	url="$1"
> +	prefix="$2"

As mentioned last time I'd rather use $2 directly at the only site
where $prefix is used. (On the other hand it might also make sense
to give the parameters a descriptive name at the beginning of the
function, but then I'd vote for a descriptive name like "urlprefix"
or similar to make its meaning clearer)

> +	remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")

A comment describing what this line actually does would be nice.

>  	remoteurl=${remoteurl%/}
>  	sep=/
>  	while test -n "$url"
> @@ -45,6 +47,11 @@ resolve_relative_url ()
>  		../*)
>  			url="${url#../}"
>  			case "$remoteurl" in
> +			.*/*)
> +				remoteurl="${remoteurl%/*}"
> +				remoteurl="${remoteurl#./}"
> +				remoteurl="${prefix}${remoteurl}"
> +				;;
>  			*/*)
>  				remoteurl="${remoteurl%/*}"
>  				;;
> @@ -64,7 +71,7 @@ resolve_relative_url ()
>  			break;;
>  		esac
>  	done
> -	echo "$remoteurl$sep${url%/}"
> +	echo "${remoteurl%/.}$sep${url%/}"

Wouldn't that better be handled in the ".*/*)" case above to avoid
accidentally affecting the other cases?

>  }
>  
>  #
> @@ -964,8 +971,14 @@ cmd_sync()
>  		# Possibly a url relative to parent
>  		case "$url" in
>  		./*|../*)
> +			up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
> +			up_path=${up_path%/}/ &&
> +			remoteurl=$(resolve_relative_url "$url" "$up_path") &&
>  			url=$(resolve_relative_url "$url") || exit
>  			;;
> +		*)
> +			remoteurl="$url"
> +			;;
>  		esac
>  
>  		if git config "submodule.$name.url" >/dev/null 2>/dev/null
> @@ -979,7 +992,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 "$remoteurl"
>  			)
>  			fi
>  		fi

I still have to wrap my head around these two hunks (I suspect it's
too late for that in my timezone ;-), but I really wonder how you get
away without changing cmd_add and cmd_init like you did last time.
This looks much different than #2 and #4 of your v3 combined, which
makes me suspicious in what direction this is evolving. Maybe you could
tell us what you found out addressing the last problem you mentioned
and how you handled it?


The only changes following here should be from test_expect_failure
to test_expect_success as mentioned in my response to your first patch.

> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 97e7a73..f2f907f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -507,17 +507,15 @@ test_expect_success 'relative path works with user@host:path' '
>  	)
>  '
>  
> -test_expect_success '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 &&
> -		echo \"cannot strip one component off url 'foo'\" >expect &&
> -		test_must_fail git submodule init 2>actual &&
> -		cat actual &&
> -		test_cmp expect actual
> +		git submodule init &&
> +		test "$(git config submodule.sub.url)" = ./subrepo
>  	)
> -"
> +'
>  
>  test_expect_success 'relative path works with foo/bar' '
>  	(
> @@ -545,7 +543,7 @@ test_expect_success 'relative path works with ./foo/bar' '
>  		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 "$(git config submodule.sub.url)" = foo/subrepo
>  	)
>  '
>  
> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index 3784c9b..583fe21 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -88,21 +88,22 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
>  	)
>  '
>  
> -test_expect_success '"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
> -	 echo \"cannot strip one component off url 'foo'\" > expect &&
> -	 test_must_fail git submodule sync 2> actual &&
> -	 test_cmp expect actual
> +	 git submodule sync &&
> +	(cd submodule &&
> +	 test "$(git config remote.origin.url)" == "../submodule"
>  	)
> -"
> +	)
> +'
>  
>  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 &&
> -	 test "$(git config remote.origin.url)" == "foo/submodule"
> +	 test "$(git config remote.origin.url)" == "../foo/submodule"
>  	)
>  	)
>  '
> @@ -112,7 +113,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo'
>  	 git remote set-url origin ./foo
>  	 git submodule sync &&
>  	(cd submodule &&
> -	 test "$(git config remote.origin.url)" == "./submodule"
> +	 test "$(git config remote.origin.url)" == "../submodule"
>  	)
>  	)
>  '
> @@ -122,7 +123,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ./foo/b
>  	 git remote set-url origin ./foo/bar
>  	 git submodule sync &&
>  	(cd submodule &&
> -	 test "$(git config remote.origin.url)" == "./foo/submodule"
> +	 test "$(git config remote.origin.url)" == "../foo/submodule"
>  	)
>  	)
>  '
> @@ -132,7 +133,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo'
>  	 git remote set-url origin ../foo
>  	 git submodule sync &&
>  	(cd submodule &&
> -	 test "$(git config remote.origin.url)" == "../submodule"
> +	 test "$(git config remote.origin.url)" == "../../submodule"
>  	)
>  	)
>  '
> @@ -142,7 +143,7 @@ test_expect_success '"git submodule sync" handles origin URL of the form ../foo/
>  	 git remote set-url origin ../foo/bar
>  	 git submodule sync &&
>  	(cd submodule &&
> -	 test "$(git config remote.origin.url)" == "../foo/submodule"
> +	 test "$(git config remote.origin.url)" == "../../foo/submodule"
>  	)
>  	)
>  '

Re: [PATCH v4 1/2] submodule: document handling of relative superproject origin URLs

[Jon Seymour]

On Thu, May 24, 2012 at 7:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Maybe the subject should rather be:
>> "submodule: document failures handling relative superproject origin URLs"
>

Jens: Sure.

>
> I recall correctly, the original use case for relative URL entries in the
> .gitmodules file (to be copied to .git/config as submodule.$name.path) was
> so that by looking at the top-level, the locations of the origins for
> submodule repositories can be known from where the top-level was cloned.
> The above cases do not seem to be relevant, so in the sense, they are of
> secondary importance (and I do not find the "sneakernet tool" example
> convincing---the sneakernet tool that is distributed in the scenario can
> be written differently so that it does not require the other repositories
> to be named relative to it).

Agreed, that this is not required for the original use case.

In the 'sneakernet tool' case, it makes sense for me to have ./mnt/usb
around in any case (for the purposes of repos that are being managed
by the sneakernet tool). Since that link exists, I can use it for the
purposes of managing the tool's own origin repo and doing so does, I
think, reduce the number of configuration items I have to tweak to the
absolute minimum, but you are right that I don't absolutely have to do
it that way.

>
> As long as you and submodule stakeholders believe this is a reasonable
> addition and does not break the existing use cases, I am perfectly fine
> with it, though.

Ok, thanks for that. Whatever the use case, I think the change is a
positive if simply because, by respecting an implicit invariant, it
makes the behaviour consistent and predictable.

I'll fix the tests, per Jens and your comments and resubmit.

Jens: thanks for the review.

Regards,

jon.

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

[Jon Seymour]

On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 23.05.2012 18:45, schrieb Jon Seymour:
>
> As mentioned last time I'd rather use $2 directly at the only site
> where $prefix is used. (On the other hand it might also make sense
> to give the parameters a descriptive name at the beginning of the
> function, but then I'd vote for a descriptive name like "urlprefix"
> or similar to make its meaning clearer)

Ok, I was favouring your latter heuristic. Are you ok if I use up_path here?

>
>> +     remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>
> A comment describing what this line actually does would be nice.
>

Sure, will do. What it does is to allow URLs of the form foo/ or
foo/bar to be handled by the .*/* case in the switch below by
rewriting the URL to prefix ./ in that case.

It doesn't do it for URLs that already begin with . (don't need it), /
(mustn't have it) or : (assuming that any path beginning with : should
probably be handled by the *:* case).

>>       remoteurl=${remoteurl%/}
>>       sep=/
>>       while test -n "$url"
>> @@ -45,6 +47,11 @@ resolve_relative_url ()
>>               ../*)
>>                       url="${url#../}"
>>                       case "$remoteurl" in
>> +                     .*/*)
>> +                             remoteurl="${remoteurl%/*}"
>> +                             remoteurl="${remoteurl#./}"
>> +                             remoteurl="${prefix}${remoteurl}"
>> +                             ;;
>>                       */*)
>>                               remoteurl="${remoteurl%/*}"
>>                               ;;
>> @@ -64,7 +71,7 @@ resolve_relative_url ()
>>                       break;;
>>               esac
>>       done
>> -     echo "$remoteurl$sep${url%/}"
>> +     echo "${remoteurl%/.}$sep${url%/}"
>
> Wouldn't that better be handled in the ".*/*)" case above to avoid
> accidentally affecting the other cases?

Yes, I think you are right. Thanks.

>
>>  }
>>
>>  #
>> @@ -964,8 +971,14 @@ cmd_sync()
>>               # Possibly a url relative to parent
>>               case "$url" in
>>               ./*|../*)
>> +                     up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
>> +                     up_path=${up_path%/}/ &&
>> +                     remoteurl=$(resolve_relative_url "$url" "$up_path") &&
>>                       url=$(resolve_relative_url "$url") || exit
>>                       ;;
>> +             *)
>> +                     remoteurl="$url"
>> +                     ;;
>>               esac
>>
>>               if git config "submodule.$name.url" >/dev/null 2>/dev/null
>> @@ -979,7 +992,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 "$remoteurl"
>>                       )
>>                       fi
>>               fi
>
> I still have to wrap my head around these two hunks (I suspect it's
> too late for that in my timezone ;-), but I really wonder how you get
> away without changing cmd_add and cmd_init like you did last time.
> This looks much different than #2 and #4 of your v3 combined, which
> makes me suspicious in what direction this is evolving. Maybe you could
> tell us what you found out addressing the last problem you mentioned
> and how you handled it?

So, I subsequently noticed that v3 broke the clone done by a
subsequent update in the case of paths of the form ../foo/bar That was
because I had violated an implicit invariant - namely that the
submodule.{name}.url configuration variable in the superproject is
always a path relative to the working tree of the superproject. In v3,
I had effectively made this value relative to the working tree of the
submodule. I mostly only need to modify sync behaviour, because that
is the only path that modifies the remote.origin.url of the submodule
- the other two paths modify the submodule.{name}.url variable of the
superproject and this behaviour is mostly correct (sans normalisation
issues).

This series respects the implicit invariant that relative paths in
configuration properties (submodule.{name}.url and
remote.{remote}.url) are always relative to the working tree of the
repository in which the configuration variable is defined.

I'll try to find a way to inject aspects of this commentary into the comments.

>
>
> The only changes following here should be from test_expect_failure
> to test_expect_success as mentioned in my response to your first patch.
>

Sure.

Thanks for your review.

jon.

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

[Jon Seymour]

On Thu, May 24, 2012 at 8:17 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> Am 23.05.2012 18:45, schrieb Jon Seymour:
>> Wouldn't that better be handled in the ".*/*)" case above to avoid
>> accidentally affecting the other cases?
>
> Yes, I think you are right. Thanks.
>

Turns out this case was necessary, but I don't fully understand why as
yet. I'll work that out and see if I can avoid doing it, otherwise I
will document why it is reasonable.

jon.