Re: [PATCH 2/7] git-submodule: Extract absolute_url & move absolute url logic to module_clone

[Junio C Hamano <gitster@pobox.com>]

Ping Yin <pkufranky@gmail.com> writes:

> Extract function absolute_url to remove code redundance and inconsistence in
> cmd_init and cmd_add when resolving relative url/path to absolute one.
>
> Also move resolving absolute url logic from cmd_add to module_clone which
> results in a litte behaviour change: cmd_update originally doesn't
> resolve absolute url but now it will.

Hmmm.  Somehow I find this unreadable and hard to parse.

> This behaviour change breaks t7400 which uses relative url './.subrepo'.
> However, this test originally doesn't mean to test relative url with './',
> so fix the url as '.subrepo'.

Isn't ".subrepo" a relative URL that says "subdirectory of the current
one, whose name is .subrepo", exactly the same way as "./.subrepo" is?
Shouldn't they behave the same?

If the test found they do not behave the same, perhaps the new code is
broken in some way and isn't "fixing" the test simply hiding a bug?

I dunno...

> +# Resolve relative url/path to absolute one
> +absolute_url () {
> +	case "$1" in
> +	./*|../*)
> +		# dereference source url relative to parent's url
> +		url="$(resolve_relative_url $1)" ;;
> +	*)
> +		# Turn the source into an absolute path if it is local
> +		url=$(get_repo_base "$1") ||
> +		url=$1
> +		;;
> +	esac
> +	echo "$url"
> +}
> +
>  #
>  # Map submodule path to submodule name
>  #
> @@ -112,7 +127,7 @@ module_info() {
>  module_clone()
>  {
>  	path=$1
> -	url=$2
> +	url=$(absolute_url "$2")
>  
>  	# If there already is a directory at the submodule path,
>  	# expect it to be empty (since that is the default checkout

Why does this call-site matter?  The URL is given to "git-clone" which I
think does handle the relative URL just fine???

> @@ -195,21 +210,7 @@ cmd_add()
>  			die "'$path' already exists and is not a valid git repo"
>  		fi
>  	else
> -		case "$repo" in
> -		./*|../*)
> -			# dereference source url relative to parent's url
> -			realrepo="$(resolve_relative_url $repo)" ;;
> -		*)
> -			# Turn the source into an absolute path if
> -			# it is local
> -			if base=$(get_repo_base "$repo"); then
> -				repo="$base"
> -			fi
> -			realrepo=$repo
> -			;;
> -		esac
> -
> -		module_clone "$path" "$realrepo" || exit
> +		module_clone "$path" "$repo" || exit
>  		(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
>  		die "Unable to checkout submodule '$path'"
>  	fi

Ok.

> @@ -262,13 +263,7 @@ cmd_init()
>  		test -z "$url" &&
>  		die "No url found for submodule path '$path' in .gitmodules"
>  
> -		# Possibly a url relative to parent
> -		case "$url" in
> -		./*|../*)
> -			url="$(resolve_relative_url "$url")"
> -			;;
> -		esac
> -
> +		url=$(absolute_url "$url")
>  		git config submodule."$name".url "$url" ||
>  		die "Failed to register url for submodule path '$path'"

Ok.