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

From: Mark Levedahl <mlevedahl@gmail.com>
Date: Feb 12, 2008, 01:00 UTC
Message-ID: <47B0EFA2.9070903@gmail.com>
In-Reply-To: <alpine.LSU.1.00.0802112152050.3870@racer.site>

Johannes Schindelin wrote:

Show 23 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.
>
>

There were three changes:
1) Eliminate die() in a subshell - it doesn't work. Instead, must have a 
return code and pass that up to top-level shell before exit can be done.
2) Eliminated duplication of get_default_remote(), instead using the 
definition in git-parse-remote.
Suggestions? (Separate patches, don't do 2)?
3) Use the user specified $remote if given.

Show 16 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.
>

This *must* define the remote using the same name as flowed down from top-level, whatever that name is. Otherwise, subsequent fetch / update following top-level just won't work. Consider if top-level branch.<name>.remote = frotz and we define not frotz but origin in the submodule. Later, on different branch, top-level branch.<name>.remote = origin. The submodule has origin defined but it points to a server different than top-level's origin. Now what?

The same holds true if user gave -r frotz: ignoring that and defining origin instead is an outright bug.

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

Good catch - there was a reason in an earlier version of this, no longer relevant.

Show 13 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.
>

Works fine as is (You need the quotes when using the variable, not when defining it):

git>git config foo.name "some string
 > with cr"
git>z=$(git config foo.name)
git>echo "$z"
some string
with cr

As written, the old code had the perverse effect of *not* quoting $url, and that was the part that needed to be quoted.

Show 12 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?
>
>

Will do.

Show 12 quoted lines

>> @@ -298,9 +333,24 @@ cmd_update()
>>  			die "Unable to find current revision in submodule path '$path'"
>>     
> 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)?
>

Ok, I was trying to avoid defining another config variable. The 
"relativeness" of a submodule is not knowable from  submodule.<path>.url 
as that is always absolute (if relative in .gitmodules, it is resolved 
into an absolute url during git init). I see two choices:
    1) define variable  submodule.<path>.isrelative during init, and use 
that instead. The user would have to modify that and the url to override.
    2) always resolve the relative url from .gitmodules, compare with 
submodule.<path>.url and decide it was relative if those two agree. More 
overhead, enough complexity that I fear for strange failure modes later on.

Any preferences or other suggestions?

Mark