[PATCH] git-submodule.sh - Remove trailing / from URL if found

[PATCH] git-submodule.sh - Remove trailing / from URL if found

[Mark Levedahl]

git clone does not complain if a trailing '/' is included in the origin
URL, but doing so causes resolution of a submodule's URL relative to the
superproject to fail. Regardless of whether git is changed to remove the
trailing / before recording the URL, we should avoid this issue in
submodule as existing repositories can have this problem.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ea6357b..fa9dd3a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -34,7 +34,7 @@ resolve_relative_url ()
 	remote=$(get_default_remote)
 	remoteurl=$(git config "remote.$remote.url") ||
 		die "remote ($remote) does not have a url in .git/config"
-	url="$1"
+	url="${1%/}"
 	while test -n "$url"
 	do
 		case "$url" in
-- 
1.6.0.22.g2957

Re: [PATCH] git-submodule.sh - Remove trailing / from URL if found

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> git clone does not complain if a trailing '/' is included in the origin
> URL, but doing so causes resolution of a submodule's URL relative to the
> superproject to fail. Regardless of whether git is changed to remove the
> trailing / before recording the URL, we should avoid this issue in
> submodule as existing repositories can have this problem.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-submodule.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ea6357b..fa9dd3a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -34,7 +34,7 @@ resolve_relative_url ()
>  	remote=$(get_default_remote)
>  	remoteurl=$(git config "remote.$remote.url") ||
>  		die "remote ($remote) does not have a url in .git/config"
> -	url="$1"
> +	url="${1%/}"
>  	while test -n "$url"
>  	do
>  		case "$url" in

Hmm, the case arms outside the context looks like this:

	while test -n "$url"
	do
		case "$url" in
		../*)
			url="${url#../}"
			remoteurl="${remoteurl%/*}"
			;;
		./*)
			url="${url#./}"
			;;
		*)
			break;;
		esac
	done
	echo "$remoteurl/$url"

If you call "resolve_relative_url ../", the first arm used to fire once
and stripped one level away, but with your patch it does not do so
anymore, and returns "$remoteurl/.." instead?

I know you are primarily interested in making sure that "../foo" and
"../foo/" are handled the same way, but somehow this change does not feel
right.  How about stripping the trailing slash from the end result?

[PATCH] git-submodule.sh - Remove trailing / from URL if found

[Mark Levedahl]

git clone does not complain if a trailing '/' is included in the origin
URL, but doing so causes resolution of a submodule's URL relative to the
superproject to fail. Trailing /'s are likely when cloning locally using
tab-completion, so the slash may appear in either superproject or
submodule URL. So, ignore the trailing slash if it already exists in
the superproject's URL, and don't record one for the submodule (which
could itself have submodules...).

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 git-submodule.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b40f876..e576cd2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -36,6 +36,7 @@ resolve_relative_url ()
 	remoteurl=$(git config "remote.$remote.url") ||
 		die "remote ($remote) does not have a url defined in .git/config"
 	url="$1"
+	remoteurl=${remoteurl%/}
 	while test -n "$url"
 	do
 		case "$url" in
@@ -50,7 +51,7 @@ resolve_relative_url ()
 			break;;
 		esac
 	done
-	echo "$remoteurl/$url"
+	echo "$remoteurl"/"${url%/}"
 }
 
 #
-- 
1.6.0.22.g2957

Re: [PATCH] git-submodule.sh - Remove trailing / from URL if found

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> git clone does not complain if a trailing '/' is included in the origin
> URL, but doing so causes resolution of a submodule's URL relative to the
> superproject to fail. Trailing /'s are likely when cloning locally using
> tab-completion, so the slash may appear in either superproject or
> submodule URL. So, ignore the trailing slash if it already exists in
> the superproject's URL, and don't record one for the submodule (which
> could itself have submodules...).
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-submodule.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Hmm.  I was sort of hoping to hear "Junio you idiot you do not know what
you are talking about --- your example of using ".." as relative won't
happen because of such and such reasons; trust me I know what is going on
in the vicinity of this code."

And after looking at the callsites of the shell function, I think the
original can never pass ".." (there are case statements to pass only $url
that match "./*" or "../*"), so I think both your original and this
version are safe as long as the part that match the trailing "/*" is
sane.

So I'll queue your first patch, as it is slightly shorter ;-)

Thanks.

Re: [PATCH] git-submodule.sh - Remove trailing / from URL if found

[Mark Levedahl]

On Wed, Aug 20, 2008 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm.  I was sort of hoping to hear "Junio you idiot you do not know what
> you are talking about --- your example of using ".." as relative won't
> happen because of such and such reasons; trust me I know what is going on
> in the vicinity of this code."
>
> And after looking at the callsites of the shell function, I think the
> original can never pass ".." (there are case statements to pass only $url
> that match "./*" or "../*"), so I think both your original and this
> version are safe as long as the part that match the trailing "/*" is
> sane.
>
> So I'll queue your first patch, as it is slightly shorter ;-)
>
> Thanks.
>

Junio,

I try to avoid the "idiot" idiom, it tends to backfire ... :^)

The problem I'm trying to fix is that a number of folks have
superprojects checked out where the recorded origin URL has a trailing
/, and a submodule has its origin in a directory sitting right next to
the superproject on the server. Thus, we have:

   superproject url = server:/public/super
   submodoule url = server:/public/sub1

However, in the checked out superproject's .git/config
[remote "origin"]
   url = server:/public/super/

and for similar reasons, the submodule has its URL recorded in .gitmodules as
[submodule "sub"]
   path = submodule1
   url = ../sub1/

resolve_relative_url gets the submodule's recorded url as $1, which
the caller retrieved from .gitmodules, and retrieves the superprojects
origin from .git/config. So in this case resolve_relative_url has
that:
   url = ../sub1/
   remoteurl = server:/public/super/

So, without any patch, resolve_relative_url computes the submodule's URL as:
   server:/public/super/sub1/

with the first patch as:
   server:/public/super/sub1

and with the second as
   server:/public/sub1
rather than
   server:/public/sub1

So, the second patch I sent is the one that fixes the above problem,
and I have to say I now forget what the confused state of .gitmodules
+ .git/config I found on one machine that lead to the first patch as
being a correct fix.

In summary, it is essential that resolve_relative_url strip the
trailing / from the superproject's url before starting, while it is
nice but not necessary if it assures that the result does not contain
a trailing /.

Mark

Re: [PATCH] git-submodule.sh - Remove trailing / from URL if found

[Junio C Hamano]

"Mark Levedahl" <mlevedahl@gmail.com> writes:

> The problem I'm trying to fix is that a number of folks have
> superprojects checked out where the recorded origin URL has a trailing
> /, and a submodule has its origin in a directory sitting right next to
> the superproject on the server. Thus, we have:
>
>    superproject url = server:/public/super
>    submodoule url = server:/public/sub1
>
> However, in the checked out superproject's .git/config
> [remote "origin"]
>    url = server:/public/super/
>
> and for similar reasons, the submodule has its URL recorded in .gitmodules as
> [submodule "sub"]
>    path = submodule1
>    url = ../sub1/
>
> resolve_relative_url gets the submodule's recorded url as $1, which
> the caller retrieved from .gitmodules, and retrieves the superprojects
> origin from .git/config. So in this case resolve_relative_url has
> that:
>    url = ../sub1/
>    remoteurl = server:/public/super/
>
> So, without any patch, resolve_relative_url computes the submodule's URL as:
>    server:/public/super/sub1/
>
> with the first patch as:
>    server:/public/super/sub1
>
> and with the second as
>    server:/public/sub1
> rather than
>    server:/public/sub1
>
> So, the second patch I sent is the one that fixes the above problem,
> and I have to say I now forget what the confused state of .gitmodules
> + .git/config I found on one machine that lead to the first patch as
> being a correct fix.
>
> In summary, it is essential that resolve_relative_url strip the
> trailing / from the superproject's url before starting, while it is
> nice but not necessary if it assures that the result does not contain
> a trailing /.

Well, that is a description worth having in the proposed commit log
message, isn't it?

[PATCH] git-submodule.sh - Remove trailing / from URL if found

[Mark Levedahl]

git clone does not complain if a trailing '/' is included in the origin
URL, but doing so causes resolution of a submodule's URL relative to the
superproject to fail. Trailing /'s are likely when cloning locally using
tab-completion, so the slash may appear in either superproject or
submodule URL. So, ignore the trailing slash if it already exists in
the superproject's URL, and don't record one for the submodule (which
could itself have submodules...).

The problem I'm trying to fix is that a number of folks have
superprojects checked out where the recorded origin URL has a trailing
/, and a submodule has its origin in a directory sitting right next to
the superproject on the server. Thus, we have:

	superproject url = server:/public/super
	submodoule url = server:/public/sub1

However, in the checked out superproject's .git/config

[remote "origin"]
	url = server:/public/super/

and for similar reasons, the submodule has its URL recorded in .gitmodules as
[submodule "sub"]

	path = submodule1
	url = ../sub1/

resolve_relative_url gets the submodule's recorded url as $1, which
the caller retrieved from .gitmodules, and retrieves the superprojects
origin from .git/config. So in this case resolve_relative_url has
that:

	url = ../sub1/
	remoteurl = server:/public/super/

So, without any patch, resolve_relative_url computes the submodule's URL as:

	server:/public/super/sub1/

rather than

	server:/public/sub1

In summary, it is essential that resolve_relative_url strip the
trailing / from the superproject's url before starting, and
beneficial if it assures that the result does not contain
a trailing / as the submodule may itself also be a superproject.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 git-submodule.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2d57d60..0bc1085 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -36,6 +36,7 @@ resolve_relative_url ()
 	remoteurl=$(git config "remote.$remote.url") ||
 		die "remote ($remote) does not have a url defined in .git/config"
 	url="$1"
+	remoteurl=${remoteurl%/}
 	while test -n "$url"
 	do
 		case "$url" in
@@ -50,7 +51,7 @@ resolve_relative_url ()
 			break;;
 		esac
 	done
-	echo "$remoteurl/$url"
+	echo "$remoteurl"/"${url%/}"
 }
 
 #
-- 
1.6.0.29.g747a