{"thread":{"id":"15764","subject":"[PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","startedAt":"2008-10-03T03:39:37Z","lastAt":"2008-10-06T19:53:52Z","messageCount":7,"participants":["David Bryson","Andreas Ericsson","Alex Riesen","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92217","messageId":"20081003033937.GA11594@eratosthenes.cryptobackpack.org","threadId":"15764","inReplyTo":null,"subject":"[PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"David Bryson","fromEmail":"david@statichacks.org","sentAt":"2008-10-03T03:39:37Z","receivedAt":"2008-10-03T03:39:37Z","isPatch":true,"sender":{"key":"david@statichacks.org","avatar":"https://gravatar.com/avatar/b8796a0b286799d99dcbaea3fd3e8675648cc094ff10b07bae8fe3bc0ac40b9c?d=mp&s=160"},"body":"\nSigned-off-by: David Bryson <david@statichacks.org>\n\nI tried to keep with the naming/coding conventions that I found in\nremote.c.  Feedback welcome.\n\n---\n remote.c |   19 ++++++++++---------\n 1 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3f3c789..893a739 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -305,6 +305,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n {\n \tconst char *name;\n \tconst char *subkey;\n+\tconst char *v;\n \tstruct remote *remote;\n \tstruct branch *branch;\n \tif (!prefixcmp(key, \"branch.\")) {\n@@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\treturn 0;\n \t\tbranch = make_branch(name, subkey - name);\n \t\tif (!strcmp(subkey, \".remote\")) {\n-\t\t\tif (!value)\n-\t\t\t\treturn config_error_nonbool(key);\n-\t\t\tbranch->remote_name = xstrdup(value);\n+\t\t\tif (git_config_string(&v, key, value) ) \n+\t\t\t\treturn -1;\n+\t\t\tbranch->remote_name = v;\n \t\t\tif (branch == current_branch)\n \t\t\t\tdefault_remote_name = branch->remote_name;\n \t\t} else if (!strcmp(subkey, \".merge\")) {\n-\t\t\tif (!value)\n-\t\t\t\treturn config_error_nonbool(key);\n-\t\t\tadd_merge(branch, xstrdup(value));\n+\t\t\tif (git_config_string(&v, key, value )) \n+\t\t\t\treturn \t-1;\n+\t\t\tadd_merge(branch, v);\n \t\t}\n \t\treturn 0;\n \t}\n@@ -334,9 +335,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\treturn 0;\n \t\trewrite = make_rewrite(name, subkey - name);\n \t\tif (!strcmp(subkey, \".insteadof\")) {\n-\t\t\tif (!value)\n-\t\t\t\treturn config_error_nonbool(key);\n-\t\t\tadd_instead_of(rewrite, xstrdup(value));\n+\t\t\tif (git_config_string(&v, key, value )) \n+\t\t\t\treturn \t-1;\n+\t\t\tadd_instead_of(rewrite, v);\n \t\t}\n \t}\n \tif (prefixcmp(key,  \"remote.\"))\n-- \n1.6.0.2\n"},{"id":"92219","messageId":"48E5AD8A.4070301@op5.se","threadId":"15764","inReplyTo":"20081003033937.GA11594@eratosthenes.cryptobackpack.org","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-10-03T05:28:42Z","receivedAt":"2008-10-03T05:28:42Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"David Bryson wrote:\n> Signed-off-by: David Bryson <david@statichacks.org>\n> \n> I tried to keep with the naming/coding conventions that I found in\n> remote.c.  Feedback welcome.\n> \n> ---\n>  remote.c |   19 ++++++++++---------\n>  1 files changed, 10 insertions(+), 9 deletions(-)\n> \n> diff --git a/remote.c b/remote.c\n> index 3f3c789..893a739 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -305,6 +305,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n>  {\n>  \tconst char *name;\n>  \tconst char *subkey;\n> +\tconst char *v;\n\n\nNot very mnemonic. I'm sure you can think up a better name, even if it's\na long one. Git is notoriously sparse when it comes to comments. We rely\ninstead on self-explanatory code.\n\n\n>  \tstruct remote *remote;\n>  \tstruct branch *branch;\n>  \tif (!prefixcmp(key, \"branch.\")) {\n> @@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\tbranch = make_branch(name, subkey - name);\n>  \t\tif (!strcmp(subkey, \".remote\")) {\n> -\t\t\tif (!value)\n> -\t\t\t\treturn config_error_nonbool(key);\n> -\t\t\tbranch->remote_name = xstrdup(value);\n> +\t\t\tif (git_config_string(&v, key, value) ) \n> +\t\t\t\treturn -1;\n> +\t\t\tbranch->remote_name = v;\n>  \t\t\tif (branch == current_branch)\n>  \t\t\t\tdefault_remote_name = branch->remote_name;\n>  \t\t} else if (!strcmp(subkey, \".merge\")) {\n> -\t\t\tif (!value)\n> -\t\t\t\treturn config_error_nonbool(key);\n> -\t\t\tadd_merge(branch, xstrdup(value));\n> +\t\t\tif (git_config_string(&v, key, value )) \n> +\t\t\t\treturn \t-1;\n> +\t\t\tadd_merge(branch, v);\n>  \t\t}\n>  \t\treturn 0;\n>  \t}\n> @@ -334,9 +335,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\trewrite = make_rewrite(name, subkey - name);\n>  \t\tif (!strcmp(subkey, \".insteadof\")) {\n> -\t\t\tif (!value)\n> -\t\t\t\treturn config_error_nonbool(key);\n> -\t\t\tadd_instead_of(rewrite, xstrdup(value));\n> +\t\t\tif (git_config_string(&v, key, value )) \n> +\t\t\t\treturn \t-1;\n> +\t\t\tadd_instead_of(rewrite, v);\n>  \t\t}\n>  \t}\n>  \tif (prefixcmp(key,  \"remote.\"))\n\n\nOther than that, the patch looks good.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"92273","messageId":"20081003200613.GO20571@eratosthenes.cryptobackpack.org","threadId":"15764","inReplyTo":"48E5AD8A.4070301@op5.se","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"David Bryson","fromEmail":"david@statichacks.org","sentAt":"2008-10-03T20:06:13Z","receivedAt":"2008-10-03T20:06:13Z","isPatch":true,"sender":{"key":"david@statichacks.org","avatar":"https://gravatar.com/avatar/b8796a0b286799d99dcbaea3fd3e8675648cc094ff10b07bae8fe3bc0ac40b9c?d=mp&s=160"},"body":"On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote:\n> David Bryson wrote:\n>> Signed-off-by: David Bryson <david@statichacks.org>\n>> I tried to keep with the naming/coding conventions that I found in\n>> remote.c.  Feedback welcome.\n>> ---\n>>  remote.c |   19 ++++++++++---------\n>>  1 files changed, 10 insertions(+), 9 deletions(-)\n>> diff --git a/remote.c b/remote.c\n>> index 3f3c789..893a739 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -305,6 +305,7 @@ static int handle_config(const char *key, const char \n>> *value, void *cb)\n>>  {\n>>  \tconst char *name;\n>>  \tconst char *subkey;\n>> +\tconst char *v;\n>\n>\n> Not very mnemonic. I'm sure you can think up a better name, even if it's\n> a long one. Git is notoriously sparse when it comes to comments. We rely\n> instead on self-explanatory code.\n>\n\nOh I agree entirely, it is quite vague, however like I mentioned I tried\nto keep to the conventios in the file.  This strategy(v) is used in several\nother places in remote.c, if this is Bad Code, then I have no problem\nchanging it.\n\nThoughts from anybody else ?\n"},{"id":"92306","messageId":"81b0412b0810041546w5eed8859vd8266bb66425ebd3@mail.gmail.com","threadId":"15764","inReplyTo":"20081003200613.GO20571@eratosthenes.cryptobackpack.org","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-04T22:46:29Z","receivedAt":"2008-10-04T22:46:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/3 David Bryson <david@statichacks.org>:\n> On Fri, Oct 03, 2008 at 07:28:42AM +0200 or thereabouts, Andreas Ericsson wrote:\n>> David Bryson wrote:\n> Oh I agree entirely, it is quite vague, however like I mentioned I tried\n> to keep to the conventios in the file.  This strategy(v) is used in several\n> other places in remote.c, if this is Bad Code, then I have no problem\n> changing it.\n>\n> Thoughts from anybody else ?\n>\n\nYou can redeclare of the variable in the contexts where\nit is used and not even rename it: it is close to its users then.\n"},{"id":"92406","messageId":"alpine.DEB.1.00.0810061610400.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15764","inReplyTo":"20081003033937.GA11594@eratosthenes.cryptobackpack.org","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-06T14:13:17Z","receivedAt":"2008-10-06T14:13:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Oct 2008, David Bryson wrote:\n\n> \n> Signed-off-by: David Bryson <david@statichacks.org>\n> \n> I tried to keep with the naming/coding conventions that I found in\n> remote.c.  Feedback welcome.\n> \n> ---\n\nUsually this comment goes after the --- but other than that, the form is \nas perfect as you can wish for.\n\n> @@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)\n>  \t\t\treturn 0;\n>  \t\tbranch = make_branch(name, subkey - name);\n>  \t\tif (!strcmp(subkey, \".remote\")) {\n> -\t\t\tif (!value)\n> -\t\t\t\treturn config_error_nonbool(key);\n> -\t\t\tbranch->remote_name = xstrdup(value);\n> +\t\t\tif (git_config_string(&v, key, value) ) \n> +\t\t\t\treturn -1;\n> +\t\t\tbranch->remote_name = v;\n\nWhat is the reason not to write\n\n\t\t\tif (git_config_string(&branch->remote_name, key, value))\n\t\t\t\treturn -1;\n\n?  (Also note that we do not like the space between the two closing \nparentheses.)\n\nCiao,\nDscho\n"},{"id":"92443","messageId":"20081006194906.GR5774@eratosthenes.cryptobackpack.org","threadId":"15764","inReplyTo":"81b0412b0810041546w5eed8859vd8266bb66425ebd3@mail.gmail.com","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"David Bryson","fromEmail":"david@statichacks.org","sentAt":"2008-10-06T19:49:06Z","receivedAt":"2008-10-06T19:49:06Z","isPatch":true,"sender":{"key":"david@statichacks.org","avatar":"https://gravatar.com/avatar/b8796a0b286799d99dcbaea3fd3e8675648cc094ff10b07bae8fe3bc0ac40b9c?d=mp&s=160"},"body":"On Sun, Oct 05, 2008 at 12:46:29AM +0200 or thereabouts, Alex Riesen wrote:\n> >\n> \n> You can redeclare of the variable in the contexts where\n> it is used and not even rename it: it is close to its users then.\n\nI'm not sure I understand entirely, care to elaborate a bit more ?\n"},{"id":"92444","messageId":"20081006195352.GS5774@eratosthenes.cryptobackpack.org","threadId":"15764","inReplyTo":"alpine.DEB.1.00.0810061610400.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] Use \"git_config_string\" to simplify \"remote.c\" code in \"handle_config\"","fromName":"David Bryson","fromEmail":"david@statichacks.org","sentAt":"2008-10-06T19:53:52Z","receivedAt":"2008-10-06T19:53:52Z","isPatch":true,"sender":{"key":"david@statichacks.org","avatar":"https://gravatar.com/avatar/b8796a0b286799d99dcbaea3fd3e8675648cc094ff10b07bae8fe3bc0ac40b9c?d=mp&s=160"},"body":"Johannes,\n\nOn Mon, Oct 06, 2008 at 04:13:17PM +0200 or thereabouts, Johannes Schindelin wrote:\n> Hi,\n> \n> On Thu, 2 Oct 2008, David Bryson wrote:\n> \n> > \n> > Signed-off-by: David Bryson <david@statichacks.org>\n> > \n> > I tried to keep with the naming/coding conventions that I found in\n> > remote.c.  Feedback welcome.\n> > \n> > ---\n> \n> Usually this comment goes after the --- but other than that, the form is \n> as perfect as you can wish for.\n\nI see, still trying to remember all the little tricks for proper\nsubmission, thanks.\n\n> > @@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)\n> >  \t\t\treturn 0;\n> >  \t\tbranch = make_branch(name, subkey - name);\n> >  \t\tif (!strcmp(subkey, \".remote\")) {\n> > -\t\t\tif (!value)\n> > -\t\t\t\treturn config_error_nonbool(key);\n> > -\t\t\tbranch->remote_name = xstrdup(value);\n> > +\t\t\tif (git_config_string(&v, key, value) ) \n> > +\t\t\t\treturn -1;\n> > +\t\t\tbranch->remote_name = v;\n> \n> What is the reason not to write\n> \n> \t\t\tif (git_config_string(&branch->remote_name, key, value))\n> \t\t\t\treturn -1;\n\nThe only reason is it did not come to mind ;-)  But it does make the\nstatement somewhat clearer.\n\n> ?  (Also note that we do not like the space between the two closing \n> parentheses.)\n\nAn oversight to be sure and not intentional, I read the CodingGuidelines\nvery carefully ;-)\n\nDave\n"}]}