{"thread":{"id":"12128","subject":"[PATCH 2/6] config: add 'git_config_string' to refactor string config variables.","startedAt":"2008-02-16T05:00:24Z","lastAt":"2008-02-16T18:33:13Z","messageCount":4,"participants":["Christian Couder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"68867","messageId":"20080216060024.385fa360.chriscool@tuxfamily.org","threadId":"12128","inReplyTo":null,"subject":"[PATCH 2/6] config: add 'git_config_string' to refactor string config variables.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-16T05:00:24Z","receivedAt":"2008-02-16T05:00:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In many places we just check if a value from the config file is not\nNULL, then we duplicate it and return 0. This patch introduces the new\n'git_config_string' function to do that.\n\nThis function is also used to refactor some code in 'config.c'.\nRefactoring other files is left for other patches.\n\nAlso not all the code in \"config.c\" is refactored, because the function\ntakes a \"const char **\" as its first parameter, but in many places a\n\"char *\" is used instead of a \"const char *\". (And C does not allow\nusing a \"char **\" instead of a \"const char **\" without a warning.)\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n cache.h  |    1 +\n config.c |   25 ++++++++++++-------------\n 2 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3867ba7..cef058d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -625,6 +625,7 @@ extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_config_int(const char *, const char *);\n extern unsigned long git_config_ulong(const char *, const char *);\n extern int git_config_bool(const char *, const char *);\n+extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n extern int git_config_set_multivar(const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 3e72778..ca6af2d 100644\n--- a/config.c\n+++ b/config.c\n@@ -309,6 +309,14 @@ int git_config_bool(const char *name, const char *value)\n \treturn git_config_int(name, value) != 0;\n }\n \n+int git_config_string(const char **dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\t*dest = xstrdup(value);\n+\treturn 0;\n+}\n+\n int git_default_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\n@@ -421,20 +429,11 @@ int git_default_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"i18n.commitencoding\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tgit_commit_encoding = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(var, \"i18n.logoutputencoding\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tgit_log_output_encoding = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"i18n.commitencoding\"))\n+\t\treturn git_config_string(&git_commit_encoding, var, value);\n \n+\tif (!strcmp(var, \"i18n.logoutputencoding\"))\n+\t\treturn git_config_string(&git_log_output_encoding, var, value);\n \n \tif (!strcmp(var, \"pager.color\") || !strcmp(var, \"color.pager\")) {\n \t\tpager_use_color = git_config_bool(var,value);\n-- \n1.5.4.1.129.gf12af-dirty\n"},{"id":"68896","messageId":"20080216115310.GA11318@sigill.intra.peff.net","threadId":"12128","inReplyTo":"20080216060024.385fa360.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-16T11:53:10Z","receivedAt":"2008-02-16T11:53:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 16, 2008 at 06:00:24AM +0100, Christian Couder wrote:\n\n> +int git_config_string(const char **dest, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\t*dest = xstrdup(value);\n> +\treturn 0;\n> +}\n\nI'm not sure I see the point in using a 'const char *' as the\ndestination. The string isn't inherently const, since it is allocated on\nthe heap; callers are free to use it as they please (and are responsible\nfor freeing it; while constness doesn't impact calling free() at a\nlanguage level, I think that stylistically it is uncommon for a\nheap-allocated string to be stored as 'const').\n\n-Peff\n"},{"id":"68911","messageId":"200802161921.43346.chriscool@tuxfamily.org","threadId":"12128","inReplyTo":"20080216115310.GA11318@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-16T18:21:42Z","receivedAt":"2008-02-16T18:21:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le samedi 16 février 2008, Jeff King a écrit :\n> On Sat, Feb 16, 2008 at 06:00:24AM +0100, Christian Couder wrote:\n> > +int git_config_string(const char **dest, const char *var, const char\n> > *value) +{\n> > +\tif (!value)\n> > +\t\treturn config_error_nonbool(var);\n> > +\t*dest = xstrdup(value);\n> > +\treturn 0;\n> > +}\n>\n> I'm not sure I see the point in using a 'const char *' as the\n> destination. The string isn't inherently const, since it is allocated on\n> the heap; callers are free to use it as they please (and are responsible\n> for freeing it; while constness doesn't impact calling free() at a\n> language level, I think that stylistically it is uncommon for a\n> heap-allocated string to be stored as 'const').\n\nWell, in many places where this function could be used the dest string is \na \"const char *\" and in many other places it's a \"char *\", but it feels \nsafer to try to promote the latter into the former (like I did in the \nfollowing patches) rather than the other way around.\n\nChristian.\n"},{"id":"68912","messageId":"20080216183312.GA20777@sigill.intra.peff.net","threadId":"12128","inReplyTo":"200802161921.43346.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-16T18:33:13Z","receivedAt":"2008-02-16T18:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 16, 2008 at 07:21:42PM +0100, Christian Couder wrote:\n\n> Well, in many places where this function could be used the dest string is \n> a \"const char *\" and in many other places it's a \"char *\", but it feels \n> safer to try to promote the latter into the former (like I did in the \n> following patches) rather than the other way around.\n\nHrm. I suppose that is true if you are talking about removing the\n'const' from existing variables (which although pointing to non-const\nallocated space in this instance, may in other code paths point to\nactual const space).\n\nBut this is really a matter of C handling this poorly, because we have\nto pass in a pointer rather than using the return value. So I think the\nmethod that best reflects the desired behavior would be to actually cast\n\"const char *\" to \"char *\" at the callsite. But unfortunately casts in C\nare error-prone themselves, and make the code a little harder to read.\n\nSo perhaps your solution is a reasonable compromise.\n\n-Peff\n"}]}