[PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[David Bryson]

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;
 	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."))
-- 
1.6.0.2

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[Andreas Ericsson]

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

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[David Bryson]

On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote:
> 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.
>

Oh I agree entirely, it is quite vague, however like I mentioned I tried
to keep to the conventios in the file.  This strategy(v) is used in several
other places in remote.c, if this is Bad Code, then I have no problem
changing it.

Thoughts from anybody else ?

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[Alex Riesen]

2008/10/3 David Bryson <david@statichacks.org>:
> On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote:
>> David Bryson wrote:
> Oh I agree entirely, it is quite vague, however like I mentioned I tried
> to keep to the conventios in the file.  This strategy(v) is used in several
> other places in remote.c, if this is Bad Code, then I have no problem
> changing it.
>
> Thoughts from anybody else ?
>

You can redeclare of the variable in the contexts where
it is used and not even rename it: it is close to its users then.

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[Johannes Schindelin]

Hi,

On Thu, 2 Oct 2008, 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.
> 
> ---

Usually this comment goes after the --- but other than that, the form is 
as perfect as you can wish for.

> @@ -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;

What is the reason not to write

			if (git_config_string(&branch->remote_name, key, value))
				return -1;

?  (Also note that we do not like the space between the two closing 
parentheses.)

Ciao,
Dscho

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[David Bryson]

On Sun, Oct 05, 2008 at 12:46:29AM +0200 or thereabouts, Alex Riesen wrote:
> >
> 
> You can redeclare of the variable in the contexts where
> it is used and not even rename it: it is close to its users then.

I'm not sure I understand entirely, care to elaborate a bit more ?

Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"

[David Bryson]

Johannes,

On Mon, Oct 06, 2008 at 04:13:17PM +0200 or thereabouts, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 2 Oct 2008, 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.
> > 
> > ---
> 
> Usually this comment goes after the --- but other than that, the form is 
> as perfect as you can wish for.

I see, still trying to remember all the little tricks for proper
submission, thanks.

> > @@ -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;
> 
> What is the reason not to write
> 
> 			if (git_config_string(&branch->remote_name, key, value))
> 				return -1;

The only reason is it did not come to mind ;-)  But it does make the
statement somewhat clearer.

> ?  (Also note that we do not like the space between the two closing 
> parentheses.)

An oversight to be sure and not intentional, I read the CodingGuidelines
very carefully ;-)

Dave