Re: [PATCH 1/3] git-submodule - Follow top-level remote on init/update/clone

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 11, 2008, 22:09 UTC
Message-ID: <alpine.LSU.1.00.0802112152050.3870@racer.site>
In-Reply-To: <1202576245-284-1-git-send-email-mlevedahl@gmail.com>

Hi,

On Sat, 9 Feb 2008, Mark Levedahl wrote:

Show 7 quoted lines

> This change allows the remote used for relative submodules (those defined
> using a url that is relative to their parent) to be defined by explicit
> definition on the command line or through the top-level project's
> branch.<name>.remote configuration.  This override is applied *only* to
> submodules defined using a url relative to the top-level project's url,
> under the assumption that such modules are logically part of the same
> project and managed as a unit.

That makes sense.

Show 9 quoted lines

> @@ -40,11 +42,13 @@ get_repo_base() {
>  # Resolve relative url by appending to parent's url
>  resolve_relative_url ()
>  {
> -	branch="$(git symbolic-ref HEAD 2>/dev/null)"
> -	remote="$(git config branch.${branch#refs/heads/}.remote)"
> -	remote="${remote:-origin}"
> -	remoteurl="$(git config remote.$remote.url)" ||
> -		die "remote ($remote) does not have a url in .git/config"

I did not really look at this code before, but does that not mean that git-submodule does already what you want?

Because usually, you clone the superproject from the URL that you actually want to use, and in that case, the initial branch's default remote will have exactly that URL.

So I have to admit that I do not see the reason why you remove that code, replace it with another (that I think does the same), and claim that you introduce that behaviour.

Your patch seems to change only one thing: you can specify "-r <remote>" with "git submodule add/init/update" -- except for some peculiarity; see below.

Show 9 quoted lines

> @@ -107,7 +112,7 @@ module_clone()
>  	test -e "$path" &&
>  	die "A file already exist at path '$path'"
>  
> -	git-clone -n "$url" "$path" ||
> +	git-clone -n -o "$remote" "$url" "$path" ||
>  	die "Clone of '$url' into submodule path '$path' failed"
>  }
>

If you do _that_, you will _force_ the submodule to have no "origin" remote. As discussed _at length_, this is not what you should do. The only reason to use "-o <other-nick-name>" is if you plan _not_ to use the same URL for the default remote.

IOW I do not like this hunk _at_ _all_. Because I get the impression that I really wasted my time explaining the same issue over and over and over again.

Show 8 quoted lines

> @@ -156,13 +169,16 @@ cmd_add()
>  	case "$repo" in
>  	./*|../*)
>  		# dereference source url relative to parent's url
> -		realrepo="$(resolve_relative_url $repo)" ;;
> +		realremote=${remote:-$(get_default_remote)}
> +		realrepo=$(resolve_relative_url $repo) || exit 1
> +		;;

Why do you need the "realremote" here?  Why is "$remote" not enough?

Show 6 quoted lines

> @@ -235,7 +259,7 @@ cmd_init()
>  		# Possibly a url relative to parent
>  		case "$url" in
>  		./*|../*)
> -			url="$(resolve_relative_url "$url")"
> +			url=$(resolve_relative_url "$url") || exit 1

Yes for the "|| exit 1". No for the removal of the quotes: keep in mind: you are possibly getting a url from the _config_, which is supposed to be user-editable.

Show 5 quoted lines

> @@ -274,6 +308,7 @@ cmd_update()
>  		shift
>  	done
>  
> +	remote=${remote:-$(get_default_remote)}

You have this paradigm so often, but AFAICS you only use it for the call to module_clone. Why not let module_clone figure it out, if $remote is empty?

> @@ -298,9 +333,24 @@ cmd_update()
>  			die "Unable to find current revision in submodule path '$path'"

Show 21 quoted lines

>  		fi
>  
> +		baseurl="$(GIT_CONFIG=.gitmodules git config submodule."$name".url)"
> +		case "$baseurl" in
> +		./*|../*)
> +			fetch_remote=$remote
> +			absurl=$(resolve_relative_url $baseurl) || exit 1
> +			(unset GIT_DIR ; cd "$path" && git config remote."$fetch_remote".url > /dev/null) ||
> +			(
> +				unset GIT_DIR; cd "$path" && git remote add "$fetch_remote" "$absurl"
> +			) || die "Unable to define remote '$fetch_remote' in submodule path '$path'"
> +			;;
> +		*)
> +			fetch_remote=
> +			;;
> +		esac
> +
>  		if test "$subsha1" != "$sha1"
>  		then
> -			(unset GIT_DIR; cd "$path" && git-fetch &&
> +			(unset GIT_DIR; cd "$path" && git-fetch $fetch_remote &&

Wasn't the whole _point_ of having a two-stage init/update that you could _change_ the remote in the config?

Now you override those settings if .gitmodules says that the path is relative? Shouldn't you respect the setting in the config (which the user can change freely), rather than .gitmodules (which the user cannot change without either committing it or having a permanently dirty working directory)?

Ciao, Dscho