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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2012, 22:50 UTC
Message-ID: <7v4nplrfe4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1341526277-17055-3-git-send-email-marcnarc@xiplink.com>

marcnarc@xiplink.com writes:

Show 6 quoted lines

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

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

Show 6 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)

> +{
> +	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?

> +int remote_count()

int remote_count(void)

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

Likewise. 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.

Show 13 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);