From: Andreas Ericsson <ae@op5.se>
Date: Fri, 03 Oct 2008 05:28:42 GMT
Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"
Message-ID: <48E5AD8A.4070301@op5.se>
In-Reply-To: <20081003033937.GA11594@eratosthenes.cryptobackpack.org>

David Bryson wrote:
> Signed-off-by: David Bryson <david@statichacks.org>
> 
> I tried to keep with the naming/coding conventions that I found in
> remote.c.  Feedback welcome.
> 
> ---
>  remote.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index 3f3c789..893a739 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -305,6 +305,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>  {
>  	const char *name;
>  	const char *subkey;
> +	const char *v;


Not very mnemonic. I'm sure you can think up a better name, even if it's
a long one. Git is notoriously sparse when it comes to comments. We rely
instead on self-explanatory code.


>  	struct remote *remote;
>  	struct branch *branch;
>  	if (!prefixcmp(key, "branch.")) {
> @@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			return 0;
>  		branch = make_branch(name, subkey - name);
>  		if (!strcmp(subkey, ".remote")) {
> -			if (!value)
> -				return config_error_nonbool(key);
> -			branch->remote_name = xstrdup(value);
> +			if (git_config_string(&v, key, value) ) 
> +				return -1;
> +			branch->remote_name = v;
>  			if (branch == current_branch)
>  				default_remote_name = branch->remote_name;
>  		} else if (!strcmp(subkey, ".merge")) {
> -			if (!value)
> -				return config_error_nonbool(key);
> -			add_merge(branch, xstrdup(value));
> +			if (git_config_string(&v, key, value )) 
> +				return 	-1;
> +			add_merge(branch, v);
>  		}
>  		return 0;
>  	}
> @@ -334,9 +335,9 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			return 0;
>  		rewrite = make_rewrite(name, subkey - name);
>  		if (!strcmp(subkey, ".insteadof")) {
> -			if (!value)
> -				return config_error_nonbool(key);
> -			add_instead_of(rewrite, xstrdup(value));
> +			if (git_config_string(&v, key, value )) 
> +				return 	-1;
> +			add_instead_of(rewrite, v);
>  		}
>  	}
>  	if (prefixcmp(key,  "remote."))


Other than that, the patch looks good.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231