# [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" 7 messages from 2008-10-03 to 2008-10-06. Participants: David Bryson, Andreas Ericsson, Alex Riesen, Johannes Schindelin. Thread: https://gitlist.dev/t/15764 ## David Bryson, 2008-10-03 03:39 Subject: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <20081003033937.GA11594@eratosthenes.cryptobackpack.org> URL: https://gitlist.dev/e/20081003033937.GA11594%40eratosthenes.cryptobackpack.org ``` Signed-off-by: David Bryson 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 ``` ## Andreas Ericsson, 2008-10-03 05:28 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <48E5AD8A.4070301@op5.se> URL: https://gitlist.dev/e/48E5AD8A.4070301%40op5.se In-Reply-To: <20081003033937.GA11594@eratosthenes.cryptobackpack.org> ``` David Bryson wrote: > Signed-off-by: David Bryson > > 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 ``` ## David Bryson, 2008-10-03 20:06 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <20081003200613.GO20571@eratosthenes.cryptobackpack.org> URL: https://gitlist.dev/e/20081003200613.GO20571%40eratosthenes.cryptobackpack.org In-Reply-To: <48E5AD8A.4070301@op5.se> ``` On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote: > David Bryson wrote: >> Signed-off-by: David Bryson >> 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, 2008-10-04 22:46 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <81b0412b0810041546w5eed8859vd8266bb66425ebd3@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0810041546w5eed8859vd8266bb66425ebd3%40mail.gmail.com In-Reply-To: <20081003200613.GO20571@eratosthenes.cryptobackpack.org> ``` 2008/10/3 David Bryson : > 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. ``` ## Johannes Schindelin, 2008-10-06 14:13 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0810061610400.22125%40pacific.mpi-cbg.de.mpi-cbg.de In-Reply-To: <20081003033937.GA11594@eratosthenes.cryptobackpack.org> ``` Hi, On Thu, 2 Oct 2008, David Bryson wrote: > > Signed-off-by: David Bryson > > 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 ``` ## David Bryson, 2008-10-06 19:49 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <20081006194906.GR5774@eratosthenes.cryptobackpack.org> URL: https://gitlist.dev/e/20081006194906.GR5774%40eratosthenes.cryptobackpack.org In-Reply-To: <81b0412b0810041546w5eed8859vd8266bb66425ebd3@mail.gmail.com> ``` 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 ? ``` ## David Bryson, 2008-10-06 19:53 Subject: Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config" Message-ID: <20081006195352.GS5774@eratosthenes.cryptobackpack.org> URL: https://gitlist.dev/e/20081006195352.GS5774%40eratosthenes.cryptobackpack.org In-Reply-To: ``` 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 > > > > 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 ```