Re: [RFC/PATCH] Introduce branch.<name>.pushremote

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Feb 9, 2013, 07:46 UTC
Message-ID: <CALkWK0kp5SvbeTQgLRKWUAT_koSi0wyp6iPTC324iH8+D8oeJg@mail.gmail.com>
In-Reply-To: <7v8v6y1sih.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 24 quoted lines

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/remote.c b/remote.c
>> index e53a6eb..d6fcfc0 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -48,6 +48,7 @@ static int branches_nr;
>>
>>  static struct branch *current_branch;
>>  static const char *default_remote_name;
>> +static const char *pushremote_name;
>>  static int explicit_default_remote_name;
>>
>>  static struct rewrites rewrites;
>> @@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
>>                               default_remote_name = branch->remote_name;
>>                               explicit_default_remote_name = 1;
>>                       }
>> +             } else if (!strcmp(subkey, ".pushremote")) {
>> +                     if (!value)
>> +                             return config_error_nonbool(key);
>> +                     branch->pushremote_name = xstrdup(value);
>
> Perhaps use git_config_string()?

I was just following the style of the surrounding code without thinking. However, it looks like the surrounding code may be dated, so I'll include a patch to update it to use git_config_string() before making the change here.

Show 11 quoted lines

>> +                     if (branch == current_branch)
>> +                             pushremote_name = branch->pushremote_name;
>
> Why is this global only when current_branch is involved?
>
> In other words, does it make sense to read branch.$name.pushremote
> for all the other irrelevant branches?
>
> In yet other words, perhaps adding pushremote_name to the branch
> structure is unneeded, and you only need this single global
> variable?

Frankly, I'm unhappy with this global. Setting a global here and subsequently reading it in pushremote_get() feels flaky. Why use it at all when we have branch->remote_name, branch->remote, and (the now introduced) branch->pushremote_name? I left the pushremote_name field around, with the expectation that other codepaths that use the remote_name field might be able to use it.

Thanks.