Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Marc Branchaud <marcnarc@xiplink.com>]

On 12-07-11 02:21 PM, Junio C Hamano wrote:
> marcnarc@xiplink.com writes:
> 
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> The code now has a default_remote_name and an effective_remote_name:
>>
>>  - default_remote_name is set by remote.default in the config, or is "origin"
>>    if remote.default doesn't exist ("origin" was the fallback value before
>>    this change).
>>
>>  - effective_remote_name is the name of the remote tracked by the current
>>    branch, or is default_remote_name if the current branch doesn't have a
>>    remote.
>>
>> This has a minor side effect on the default behavior of "git push".  Consider
>> the following sequence of commands:
>>
>>       git config remote.default foo                 # Set default remote
>>       git checkout somelocalbranch                  # Does not track a remote
>>       git remote add origin ssh://example.com/repo  # Add a new "origin"
>>       git push
>>
>> Prior to this change, the above "git push" would attempt to push to the new
>> "origin" repository at ssh://example.com/repo.  Now instead that "git push"
>> will attempt to push to the repository named "foo".
> 
> Hrm, is this a _minor_ side effect?

Well, I thought so...  :)

It's minor because of what you say:

> Isn't that a natural and direct consequence of "now you can set
> remote.default configuration to name the remote you want to use in
> place of traditional 'origin'" feature?  I think changing the
> behaviour of "git push" in such a way is the point (may not be the
> only but one of the important points) of this series.

I agree.  Phil Hord pointed out the change in behaviour and felt the commit
message should explain it.

Should I just remove "minor"?

>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index cb97cc1..fc17d39 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
>>  OPTIONS[[OPTIONS]]
>>  ------------------
>>  <repository>::
>> -	The "remote" repository that is destination of a push
>> +	The "remote" repository that is the destination of the push
>>  	operation.  This parameter can be either a URL
>>  	(see the section <<URLS,GIT URLS>> below) or the name
>>  	of a remote (see the section <<REMOTES,REMOTES>> below).
>> +	If this parameter is omitted, git tries to use the remote
>> +	associated with the currently checked-out branch.  If there
>> +	is no remote associated with the current branch, git uses
>> +	the remote named by the "remote.default" configuration variable.
>> +	If "remote.default" is not set, git uses the name "origin" even
>> +	if there is no actual remote named "origin".
> 
> This comment applies to other changes to the documentation in this
> patch I didn't quote, but I think the phrasing 'even if there is no
> acutual remote named "origin"' needs to be rethought, because "we
> use it even if there is no such remote determined with this logic"
> applies equally to branch.$name.remote, remote.default or built-in
> default value of remote.default which happens to be "origin".

I'm sorry, but I'm having trouble understanding what you mean.  I don't see
how the "because ..." part of your sentence suggests what aspect needs
rephrasing.

(BTW, I'm under the impression that when git sets branch.$name.remote it
doesn't ever have to fall back to the default remote name.  That is, commands
that set the value always have the user explicitly, though perhaps
indirectly, specifying the remote.  Did I miss something?)

>> diff --git a/remote.c b/remote.c
>> index 6f371e0..2ebdbbd 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -47,6 +47,7 @@ static int branches_alloc;
>>  static int branches_nr;
>>  
>>  static struct branch *current_branch;
>> +static const char *default_remote_name;
>>  static const char *effective_remote_name;
> 
> Coming from UNIX background, "effective" gives me an impression that
> it is contrasted with "real" (i.e. there is "real remote" for the
> branch, but during this particular invocation it is overridden and
> "effective remote" is used instead).  Perhaps it is just me.

I did have something like that in mind when I chose "effective":  There's a
default remote name that's normally used, but in the certain circumstances
it's overridden.  Perhaps "effective_default_remote_name" would be better,
though that's getting to be a mouthful (plus it would mean having an
"explicit_effective_default_remote_name", which IMHO is a bit much).

> Something along the line of remote-name-to-use, use-remote, might
> sound more natural.

current_remote_name?

		M.