submodule: add tests for add,sync,init in presence of relative super origin URL

PATCH In review Jon Seymour 3 participants 10 messages Jun 15, 2016

lore

Share & tools

lore

Jon Seymour

author

2016-06-15 22:53 UTC #0 lore raw

PATCH 1/2 submodule: add tests for add,sync,init in presence of relative super origin URL

These tests are expected to fail, pending subsequent patch.

Signed-off-by Jon Seymour <[email protected]>

---
 t/t7400-submodule-basic.sh | 27 +++++++++++++++++++++++++++
 t/t7403-submodule-sync.sh  | 10 ++++++++++
 2 files changed, 37 insertions(+)

t/t7400-submodule-basic.sh +27 -0

index 81827e6..1c40951 100755
@@ -507,6 +507,33 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_failure 'relative path works with ../relative/repo' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		git config remote.origin.url ../relative/repo &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = ../../relative/subrepo
+	)
+'
+
+test_expect_failure 'test that submodule add creates the correct url when super origin url is relative' '
+	mkdir reladd &&
+	(
+		cd reladd &&
+		git init &&
+		git remote add origin ../relative/repo
+		mkdir sub &&
+		(
+			cd sub &&
+			git init &&
+			test_commit foo
+		) &&
+		git submodule add ../subrepo ./sub &&
+		test "$(git config submodule.sub.url)" = ../../relative/subrepo
+	)
+'
+
 test_expect_success 'moving the superproject does not break submodules' '
 	(
 		cd addtest &&

t/t7403-submodule-sync.sh +10 -0

index 3620215..788bc24 100755
@@ -86,4 +86,14 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
+test_expect_failure '"git submodule sync" should handle a super with a relative origin URL' '
+	git clone super relative-clone &&
+	(cd relative-clone &&
+	 git submodule update --init &&
+	 git remote set-url origin ../relative/super.git &&
+	 git submodule sync &&
+	 test "$(git config submodule.submodule.url)" == ../../relative/moved-submodule
+	)
+'
+
 test_done

Signature · 1 line

1.7.10.2.649.gec87875

Jon Seymour

author

2016-06-15 22:53 UTC #1 lore raw

PATCH 2/2 submodule: fix handling of supermodules with relative origin URLs

Prior to this change, an operation such as git submodule add, init or sync produced the wrong result when the origin URL of the supermodule was itself a relative URL.

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

This change ensures that this is done for add, sync and init.

Signed-off-by Jon Seymour <[email protected]>

---
 git-submodule.sh           | 35 ++++++++++++++++++++---------------
 t/t7400-submodule-basic.sh |  4 ++--
 t/t7403-submodule-sync.sh  |  2 +-
 3 files changed, 23 insertions(+), 18 deletions(-)

git-submodule.sh +20 ~~-15~~

index 64a70d6..230c219 100755
@@ -45,6 +45,10 @@ resolve_relative_url ()
 		../*)
 			url="${url#../}"
 			case "$remoteurl" in
+			.*/*)
+				up_path="$(echo "$2" | sed "s/[^/]*/../g")"
+				remoteurl="${up_path%/}/${remoteurl%/*}"
+				;;
 			*/*)
 				remoteurl="${remoteurl%/*}"
 				;;

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

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

@@ -407,7 +412,7 @@ cmd_init()
 			# Possibly a url relative to parent
 			case "$url" in
 			./*|../*)
-				url=$(resolve_relative_url "$url") || exit
+				url=$(resolve_relative_url "$url" "$sm_path") || exit
 				;;
 			esac
 			git config submodule."$name".url "$url" ||

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

t/t7400-submodule-basic.sh +2 -2

index 1c40951..e10abc4 100755
@@ -507,7 +507,7 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
-test_expect_failure 'relative path works with ../relative/repo' '
+test_expect_success 'relative path works with ../relative/repo' '
 	(
 		cd reltest &&
 		cp pristine-.git-config .git/config &&

@@ -517,7 +517,7 @@ test_expect_failure 'relative path works with ../relative/repo' '
 	)
 '
 
-test_expect_failure 'test that submodule add creates the correct url when super origin url is relative' '
+test_expect_success 'test that submodule add creates the correct url when super origin url is relative' '
 	mkdir reladd &&
 	(
 		cd reladd &&

t/t7403-submodule-sync.sh +1 -1

index 788bc24..35700ef 100755
@@ -86,7 +86,7 @@ test_expect_success '"git submodule sync" should not vivify uninteresting submod
 	)
 '
 
-test_expect_failure '"git submodule sync" should handle a super with a relative origin URL' '
+test_expect_success '"git submodule sync" should handle a super with a relative origin URL' '
 	git clone super relative-clone &&
 	(cd relative-clone &&
 	 git submodule update --init &&

Signature · 1 line

1.7.10.2.649.gec87875

Jens Lehmann

2016-06-15 22:53 UTC #2 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

Am 19.05.2012 06:40, schrieb Jon Seymour:

Quoted 9 lines

Prior to this change, an operation such as git submodule add, init or sync produced the wrong result when the origin URL of the supermodule was itself a relative URL.
The issue arises in these cases, because the origin URL of the supermodule needs to be prepended with a prefix that navigates from the submodule to the supermodule so that when the submodule URL is concatenated, the resulting url is relative to the working tree of the submodule.

Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".

BTW, what happened to the following comment in you other email?

Quoted 3 lines

Quoted 1 line
+ remoteurl="${up_path%/}/${remoteurl%/*}"
Meant up_path%/ to be up_path%/*

The '*' is not there (but the test suite runs fine no matter if I add a '*' there or not). Thinking about it not adding the '*' should be correct, as you just want to chop off a trailing '/' from $up_path here, right?

So no objection on the code changes from my side.

Jon Seymour

author

2016-06-15 22:53 UTC #3 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

Quoted 4 lines · Jens Lehmann

Am 19.05.2012 06:40, schrieb Jon Seymour:
Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".

Sure. I can't argue with precedent, of course, but I guess I was favouring the consistency in the suffixes used with sub and super.

Quoted 4 lines

BTW, what happened to the following comment in you other email?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
+ remoteurl="${up_path%/}/${remoteurl%/*}"
Meant up_path%/ to be up_path%/*
The '*' is not there (but the test suite runs fine no matter if I add a '*' there or not). Thinking about it not adding the '*' should be correct, as you just want to chop off a trailing '/' from $up_path here, right?

Yes %/* was actually the wrong thing to do - my original intent was to remove repeated trailing occurrences of /, but, of course, %/* doesn't do that, nor should it be necessary (assuming the sm_path was normalized during add).

Quoted 2 lines

So no objection on the code changes from my side.

I noticed one relative case that is not handled properly yet, but there is a workaround. If the superproject's origin URL is of the
form: foo/bar (a case I actually have myself for reasons I can explain if you want me to), then the correct rule doesn't get matched by .*/*). The workaround is for the user to change foo/bar style origin URLs to ./foo/bar.

Let me know if I should fix this case now too.

jon.

Jens Lehmann

2016-06-15 22:53 UTC #4 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

Am 20.05.2012 00:51, schrieb Jon Seymour:

Quoted 8 lines · Jens Lehmann

Quoted 4 lines
Am 19.05.2012 06:40, schrieb Jon Seymour:
Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".
Sure. I can't argue with precedent, of course, but I guess I was favouring the consistency in the suffixes used with sub and super.

No big deal, but in recent posts "superproject" has been used and the similarity between "supermodule" and "submodule" fooled me when I read your RFC patch. So even though a superproject might be the a submodule of another superproject, I'm all for using the term "superproject" to make the distinction obvious.

Quoted 10 lines

Quoted 1 line
So no objection on the code changes from my side.
I noticed one relative case that is not handled properly yet, but there is a workaround. If the superproject's origin URL is of the
form: foo/bar (a case I actually have myself for reasons I can explain if you want me to), then the correct rule doesn't get matched by .*/*). The workaround is for the user to change foo/bar style origin URLs to ./foo/bar.
Let me know if I should fix this case now too.

Me thinks that this is subject for a subsequent patch. Having the URL of the superproject *below* the root directory of the superproject seems like a rather odd case which warrants a fix of its own ;-)

Jon Seymour

author

2016-06-15 22:53 UTC #5 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

Quoted 25 lines · Jens Lehmann

Am 20.05.2012 00:51, schrieb Jon Seymour:
Quoted 8 lines · Jens Lehmann
Quoted 4 lines
Am 19.05.2012 06:40, schrieb Jon Seymour:
Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".
Sure. I can't argue with precedent, of course, but I guess I was favouring the consistency in the suffixes used with sub and super.
No big deal, but in recent posts "superproject" has been used and the similarity between "supermodule" and "submodule" fooled me when I read your RFC patch. So even though a superproject might be the a submodule of another superproject, I'm all for using the term "superproject" to make the distinction obvious.
Quoted 9 lines
Quoted 1 line
So no objection on the code changes from my side.
I noticed one relative case that is not handled properly yet, but there is a workaround. If the superproject's origin URL is of the
form: foo/bar (a case I actually have myself for reasons I can explain if you want me to), then the correct rule doesn't get matched by .*/*). The workaround is for the user to change foo/bar style origin URLs to ./foo/bar.

Quoted 6 lines

Quoted 1 line
Let me know if I should fix this case now too.
Me thinks that this is subject for a subsequent patch. Having the URL of the superproject *below* the root directory of the superproject seems like a rather odd case which warrants a fix of its own ;-)

The situation arises in my case because my working directory (which has git controlled elements) contains a subdirectory called mnt with a symbolic link called git which points to the actual location of my git respositories. So, in this case:

    mnt/git/public/work.git -> /var/git/public/work.git

So, if I want to set my origin to be my public git repositories, I naturally do:

git remote set-url origin mnt/git/public/work.git

I'll wait to hear Junio's preference with regard to how I should split the patches up before posting v3 that includes support for foo/bar in addition to ../foo/bar and ./foo/bar.

Regards,

jon.

Junio C Hamano

2016-06-15 22:53 UTC #6 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

In reply to Jon Seymour

Quoted 8 lines · Jon Seymour

Quoted 4 lines · Jens Lehmann
Am 19.05.2012 06:40, schrieb Jon Seymour:
Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".
Sure. I can't argue with precedent, of course, but I guess I was favouring the consistency in the suffixes used with sub and super.

We fairly consistently say (even outside the documentation---for example, listen to the TechTalk Linus gave in May 2007) "superproject" and never "supermodule". You can tell people who were not paying attention when they say "supermodule" ;-).

Jens Lehmann

2016-06-15 22:53 UTC #7 lore raw

PATCH 1/1 Consistently use "superproject" instead of "supermodule"

We fairly consistently say "superproject" and never "supermodule" these days. But there are seven occurrences of "supermodule" left in the current work tree. Three appear in Release Notes for 1.5.3 and 1.7.7, three in test names and one in a C-code comment.

Replace all occurrences of "supermodule" outside of the Release Notes (which shouldn't be changed after the fact) with "superproject" for consistency.

Signed-off-by Jens Lehmann <[email protected]>

---

Am 20.05.2012 07:16, schrieb Junio C Hamano:

Quoted 8 lines

Quoted 6 lines · Jon Seymour
Quoted 4 lines · Jens Lehmann
Am 19.05.2012 06:40, schrieb Jon Seymour:
Just a small nit: I'd prefer to replace the 4 occurrences of the term "supermodule" with "superproject".

Quoted 7 lines

Quoted 2 lines
Sure. I can't argue with precedent, of course, but I guess I was favouring the consistency in the suffixes used with sub and super.
We fairly consistently say (even outside the documentation---for example, listen to the TechTalk Linus gave in May 2007) "superproject" and never "supermodule". You can tell people who were not paying attention when they say "supermodule" ;-).

Or they used one of the nine commit messages in current masters history or one the seven occurrences in the current work tree which use the term "supermodule" as inspiration ;-)

While we can't change history and shouldn't change Release Notes, the other uses of "supermodule" should be removed.

 t/t7408-submodule-reference.sh | 4 ++--
 t/t9300-fast-import.sh         | 2 +-
 unpack-trees.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

t/t7408-submodule-reference.sh +2 -2

index a45fadc..b770b2f 100755
@@ -28,7 +28,7 @@ git prune'
 
 cd "$base_dir"
 
-test_expect_success 'preparing supermodule' \
+test_expect_success 'preparing superproject' \
 'test_create_repo super && cd super &&
 echo file > file &&
 git add file &&

@@ -55,7 +55,7 @@ diff expected current'
 
 cd "$base_dir"
 
-test_expect_success 'cloning supermodule' \
+test_expect_success 'cloning superproject' \
 'git clone super super-clone'
 
 cd "$base_dir"

t/t9300-fast-import.sh +1 -1

index 7da0e8d..2aa1824 100755
@@ -1657,7 +1657,7 @@ M 160000 :6 sub
 INPUT_END
 
 test_expect_success \
-	'P: supermodule & submodule mix' \
+	'P: superproject & submodule mix' \
 	'git fast-import <input &&
 	 git checkout subuse1 &&
 	 rm -rf sub && mkdir sub && (cd sub &&

unpack-trees.c +1 -1

index bcee99c..ad40109 100644
@@ -1210,7 +1210,7 @@ static int verify_uptodate_1(struct cache_entry *ce,
 			return 0;
 		/*
 		 * NEEDSWORK: the current default policy is to allow
-		 * submodule to be out of sync wrt the supermodule
+		 * submodule to be out of sync wrt the superproject
 		 * index.  This needs to be tightened later for
 		 * submodules that are marked to be automatically
 		 * checked out.

Signature · 1 line

1.7.10.2.548.g9de9681.dirty

Jon Seymour

author

2016-06-15 22:53 UTC #8 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

In reply to Jon Seymour

Quoted 10 lines · Jon Seymour

Quoted 1 line · Jens Lehmann
Am 19.05.2012 06:40, schrieb Jon Seymour:
I noticed one relative case that is not handled properly yet, but there is a workaround. If the superproject's origin URL is of the
form: foo/bar (a case I actually have myself for reasons I can explain if you want me to), then the correct rule doesn't get matched by .*/*). The workaround is for the user to change foo/bar style origin URLs to ./foo/bar.
Let me know if I should fix this case now too.

I think this expression:

    remoteurl=$(echo "remoteurl" | sed "s|^[^/][^:]*\$|./&|" )
    remoteurl=${remoteurl%/}

would normalize remoteurl correctly for the foo/bar case and not any other.

I'd also need to add at least one new test (or do I actually need 3?), of course.

jon.

Jon Seymour

author

2016-06-15 22:53 UTC #9 lore raw

Re: [PATCH 2/2] submodule: fix handling of supermodules with relative origin URLs

Quoted 14 lines · Jon Seymour

Quoted 10 lines · Jon Seymour
Quoted 1 line · Jens Lehmann
Am 19.05.2012 06:40, schrieb Jon Seymour:
I noticed one relative case that is not handled properly yet, but there is a workaround. If the superproject's origin URL is of the
form: foo/bar (a case I actually have myself for reasons I can explain if you want me to), then the correct rule doesn't get matched by .*/*). The workaround is for the user to change foo/bar style origin URLs to ./foo/bar.
Let me know if I should fix this case now too.
I think this expression:

Quoted 7 lines

remoteurl=$(echo "remoteurl" | sed "s|^[^/][^:]*\$|./&|" ) remoteurl=${remoteurl%/}
would normalize remoteurl correctly for the foo/bar case and not any other.
I'd also need to add at least one new test (or do I actually need 3?), of course.

Ok. I have a version that implements this fix:

        url="$1"
+       remoteurl=$(echo "$remoteurl" | sed "s|^[^/][^:]*\$|./&|")
        remoteurl=${remoteurl%/}

and...

+ .*/*)

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

with a single test that demonstrates that it works. The additional #./ tweak is so that submodule URLs end up like:

../foo/sub

instead of:

../foo/./sub

jon.