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

[Jens Lehmann <Jens.Lehmann@web.de>]

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"
>  	)
>  	)
>  '