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

From: Marc Branchaud <marcnarc@xiplink.com>
Date: Jul 6, 2012, 14:36 UTC
Message-ID: <4FF6F805.20403@xiplink.com>
In-Reply-To: <7v4nplrfe4.fsf@alter.siamese.dyndns.org>

On 12-07-05 06:50 PM, Junio C Hamano wrote:

Show 17 quoted lines

> 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.
> 
> The explanation of the latter belongs to the previous step, I think.
> I am not sure if "effective" is the best name for the concept the
> above explains, though.

Well, the previous commit removes default_remote_name, so the explanation wouldn't be valid verbatim.

How about keeping the above here, and I could add the following to the previous commit's message:

	effective_remote_name is the remote name that is currently "in
	effect".  This is the currently checked-out branch's remote, or
	"origin" if the branch has no remote (or the working tree is a
	detached HEAD).

Show 7 quoted lines

>> @@ -390,6 +391,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>>  	}
>>  	if (prefixcmp(key,  "remote."))
>>  		return 0;
>> +
> 
> Why?

Oops, just a newline I neglected to clean up from some earlier hacking.  Sorry.

Show 8 quoted lines

>>  	name = key + 7;
>> @@ -671,6 +680,18 @@ static int valid_remote_nick(const char *name)
>>  	return !strchr(name, '/'); /* no slash */
>>  }
>>  
>> +const char *remote_get_default_name()
> 
> const char *remote_get_default_name(void)

OK.

Show 15 quoted lines

>> +{
>> +	read_config();
>> +	return default_remote_name;
>> +}
> 
> Hrmph.  I am too lazy to read outside the context of your patch to
> make sure, but isn't the root cause of the problem that when we try
> to find which remote the current branch is configured to interact
> with, we grab branch->remote_name (and this is done by calling
> git_config() to open and read the configuration file once already)
> and if it is empty we default to "origin"?  Wouldn't the callback
> function that is used for that invocation of git_config() a much
> better place to set "default_remote_name" variable, instead of
> having us to read the entire configuration file one more time only
> to get the value of this variable?

The read_config() function already has logic to avoid re-parsing the entire config over and over again. There are many places in remote.c that call read_config(), and I thought I was just following that pattern.

Also, making the code be

	if (!default_remote_name)
		read_config();
	return default_remote_name;

would just replicate the check that read_config() already does.

>> +int remote_count()
> 
> int remote_count(void)

OK.

Show 6 quoted lines

>> +{
>> +	read_config();
>> +	return remotes_nr;
>> +}
> 
> Likewise.

Doing something like

	if (!remotes_nr)
		read_config():
	return remotes_nr;

would be wrong, since having 0 remotes is perfectly fine.  And making the
check here be
	if (!default_remote_name)
seems confusing, and again it duplicates the check read_config() already does.

> Especially it is unclear who benefits from the function
> until a new caller is introduced.  I would prefer not to see the
> addition of this function in this patch.

OK, I can move it to the "git remote" patch. The same could be said of remote_get_default_name() though.

Show 16 quoted lines

>>  struct remote *remote_get(const char *name)
>>  {
>>  	struct remote *ret;
>> diff --git a/remote.h b/remote.h
>> index 251d8fd..f9aac87 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -52,6 +52,8 @@ struct remote {
>>  
>>  struct remote *remote_get(const char *name);
>>  int remote_is_configured(const char *name);
>> +const char *remote_get_default_name();
>> +int remote_count();
> 
> const char *remote_get_default_name(void);
> int remote_count(void);

Got it.

I'll make these changes in a few days, after folks have had a chance to review. If things settle down I'll re-roll with the documentation updates too.

M.