[PATCH] Fix submodule sync with relative submodule URLs

Signed-off-by: Johan Herland <redacted>
---
 git-submodule.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 1c39b59..92be0fe 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -634,6 +634,14 @@ cmd_sync()
 	do
 		name=$(module_name "$path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
+
+		# Possibly a url relative to parent
+		case "$url" in
+		./*|../*)
+			url=$(resolve_relative_url "$url") || exit
+			;;
+		esac
+
 		if test -e "$path"/.git
 		then
 		(

-- 
1.6.0.1.400.gd2470

On Mon, Sep 22, 2008 at 9:08 AM, Johan Herland [off-list ref] wrote:

Instead of just doing an "|| exit" shouldn't it report an explanation
of the error?
Other than that, it looks good to me.

Mark, Junio?


-- 
 David

Signed-off-by: Johan Herland <redacted>
---

On Wednesday 24 September 2008, David Aguilar wrote:

Instead of just doing an "|| exit" shouldn't it report an explanation
of the error?
Other than that, it looks good to me.

Fixed. Thanks.

...Johan


 git-submodule.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 1c39b59..f89bdbe 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -634,6 +634,15 @@ cmd_sync()
 	do
 		name=$(module_name "$path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
+
+		# Possibly a url relative to parent
+		case "$url" in
+		./*|../*)
+			url=$(resolve_relative_url "$url") ||
+				die "failed to resolve relative submodule url for '$name'"
+			;;
+		esac
+
 		if test -e "$path"/.git
 		then
 		(

-- 
1.6.0.2.463.g7f0eb

Johan Herland [off-list ref] wrote:

On Wednesday 24 September 2008, David Aguilar wrote:

quoted

Instead of just doing an "|| exit" shouldn't it report an explanation
of the error?
Other than that, it looks good to me.

Fixed. Thanks.

OK, time for the drive-by patch commenting.  I've largely stayed
out of git-submodule related code, but I just looked at in the
context of applying this patch.

There are three callers to resolve_relative_url in master and next.
All three callers just "|| exit" when resolve_relative_url fails.

The only reason resolve_relative_url can fail is when there is no
remote.$remote.url configuration option set for the current default
remote ("origin"?).

I guess I'm unclear about why cmd_sync is different from the
existing callers.

-- 
Shawn.

On Wednesday 24 September 2008, Shawn O. Pearce wrote:

I guess I'm unclear about why cmd_sync is different from the
existing callers.

It's not any different as far as I'm concerned. We should probably add 
helpful error messages to the other callers as well.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

resolve_relative_url calls die() when no remote url exists so these calls to
exit can be removed.

Signed-off-by: David Aguilar <redacted>
---

This applies on top of the first
"Fix submodule sync with relative submodule URLs" patch by Johan Herland.
Shawn's comments made me realize that the resolve_relative_url callers
could be cleaned up.

 git-submodule.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 92be0fe..533d1cc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -155,7 +155,7 @@ cmd_add()
 	case "$repo" in
 	./*|../*)
 		# dereference source url relative to parent's url
-		realrepo=$(resolve_relative_url "$repo") || exit
+		realrepo=$(resolve_relative_url "$repo")
 		;;
 	*:*|/*)
 		# absolute url

@@ -184,7 +184,7 @@ cmd_add()
 
 		case "$repo" in
 		./*|../*)
-			url=$(resolve_relative_url "$repo") || exit
+			url=$(resolve_relative_url "$repo")
 		    ;;
 		*)
 			url="$repo"

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

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

-- 
1.6.0.2.307.gc4275

On  0, "Shawn O. Pearce" [off-list ref] wrote:

Johan Herland [off-list ref] wrote:

quoted

On Wednesday 24 September 2008, David Aguilar wrote:

quoted

Instead of just doing an "|| exit" shouldn't it report an explanation
of the error?
Other than that, it looks good to me.

Fixed. Thanks.

OK, time for the drive-by patch commenting.  I've largely stayed
out of git-submodule related code, but I just looked at in the
context of applying this patch.

There are three callers to resolve_relative_url in master and next.
All three callers just "|| exit" when resolve_relative_url fails.

The only reason resolve_relative_url can fail is when there is no
remote.$remote.url configuration option set for the current default
remote ("origin"?).

I guess I'm unclear about why cmd_sync is different from the
existing callers.

Right, it's not.  resolve_relative_url() is already calling
die() when that error condition is met, so the "|| exit" thing
can be removed entirely.  I sent a patch on top of Johan's
first patch to remove the "|| exit" calls in all callers of
resolve_relative_url.  That seems like the right thing to do;
in the very least it makes it easier to read.  What do you
think?

quoted

diff --git a/git-submodule.sh b/git-submodule.sh
index 1c39b59..f89bdbe 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -634,6 +634,15 @@ cmd_sync()
 	do
 		name=$(module_name "$path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
+
+		# Possibly a url relative to parent
+		case "$url" in
+		./*|../*)
+			url=$(resolve_relative_url "$url") ||
+				die "failed to resolve relative submodule url for '$name'"
+			;;
+		esac
+
 		if test -e "$path"/.git
 		then
 		(

-- 
Shawn.

-- 

	David

David Aguilar schrieb:

resolve_relative_url calls die() when no remote url exists so these calls to
exit can be removed.

...

Did you test it? The command inside $(...) is run in its own sub-process,
therefore, the die() does not exit the caller, just the sub-process, and
the || exit *is* required.

BTW, I think that || exit is sufficient; you don't need to add another
error message - the one that resolve_relative_url() prints is sufficient.

-- Hannes

Johannes Sixt [off-list ref] wrote:

David Aguilar schrieb:

quoted

resolve_relative_url calls die() when no remote url exists so these calls to
exit can be removed.

...

quoted

@@ -155,7 +155,7 @@ cmd_add()
 	case "$repo" in
 	./*|../*)
 		# dereference source url relative to parent's url
-		realrepo=$(resolve_relative_url "$repo") || exit
+		realrepo=$(resolve_relative_url "$repo")
 		;;

Did you test it? The command inside $(...) is run in its own sub-process,
therefore, the die() does not exit the caller, just the sub-process, and
the || exit *is* required.

BTW, I think that || exit is sufficient; you don't need to add another
error message - the one that resolve_relative_url() prints is sufficient.

Exactly.

I think we just need a "|| exit" after each of these
$(resolve_relative_url) calls.  The original patch that
started this discussion just needs a "|| exit".

die with an additional message is just too verbose.


-- 
Shawn.

On Thursday 25 September 2008, Shawn O. Pearce wrote:

Johannes Sixt [off-list ref] wrote:

quoted

Did you test it? The command inside $(...) is run in its own
sub-process, therefore, the die() does not exit the caller, just
the sub-process, and the || exit *is* required.

BTW, I think that || exit is sufficient; you don't need to add
another error message - the one that resolve_relative_url() prints
is sufficient.

Exactly.

I think we just need a "|| exit" after each of these
$(resolve_relative_url) calls.  The original patch that
started this discussion just needs a "|| exit".

The original patch did exactly that: 
http://article.gmane.org/gmane.comp.version-control.git/96493


Have fun!

...Johan


-- 
Johan Herland, [off-list ref]
www.herland.net