On 12-07-05 06:50 PM, Junio C Hamano wrote:
> marcnarc@xxxxxxxxxxx writes:
>
>> From: Marc Branchaud <marcnarc@xxxxxxxxxxx>
>>
>> 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).
>> @@ -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.
>> 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.
>> +{
>> + 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.
>> +{
>> + 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.
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html