threads / patch / 15764

patchUse "git_config_string" to simplify "remote.c" code in "handle_config"

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

## tl;dr

7 messages between Oct 3, 2008 and Oct 6, 2008. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

David Bryson· Oct 3, 2008, 03:39 UTC · lore
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(-)
Show changes to remote.c +10 −9
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
Andreas Ericsson· Oct 3, 2008, 05:28 UTC · re: David Bryson · lore

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

David Bryson wrote:
Show 18 quoted lines
> 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.

Show 38 quoted lines
>  	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
David Bryson· Oct 3, 2008, 20:06 UTC · re: Andreas Ericsson · lore

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

On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote:
Show 23 quoted lines
> 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 ?
Alex Riesen· Oct 4, 2008, 22:46 UTC · re: David Bryson · lore

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

2008/10/3 David Bryson <david@statichacks.org>:
Show 9 quoted lines
> 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.

David Bryson· Oct 6, 2008, 19:49 UTC · re: Alex Riesen · lore

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

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 ?
Johannes Schindelin· Oct 6, 2008, 14:13 UTC · re: David Bryson · lore

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

Hi,
On Thu, 2 Oct 2008, David Bryson wrote:
Show 7 quoted lines
> 
> 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.

Show 10 quoted lines
> @@ -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

David Bryson· Oct 6, 2008, 19:53 UTC · re: Johannes Schindelin · lore

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

Johannes,
On Mon, Oct 06, 2008 at 04:13:17PM +0200 or thereabouts, Johannes Schindelin wrote:
Show 14 quoted lines
> 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.

Show 15 quoted lines
> > @@ -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

← back to recent threads