{"thread":{"id":"12012","subject":"[PATCH] diff.c: guard config parser from value=NULL","startedAt":"2008-02-10T18:35:34Z","lastAt":"2008-02-11T04:55:19Z","messageCount":4,"participants":["Christian Couder","Pierre Habouzit","Martin Koegler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68244","messageId":"20080210193534.d11d8b15.chriscool@tuxfamily.org","threadId":"12012","inReplyTo":null,"subject":"[PATCH] diff.c: guard config parser from value=NULL","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-10T18:35:34Z","receivedAt":"2008-02-10T18:35:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In fact we also guard for value=\"\" as it doesn't make more sense\nfor the variables here.\n\nWe do that by using a new function 'xstrdup_confval' to avoid code\nduplication.\n\nBy the way this changes a 'strdup' into 'xstrdup'.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n diff.c |   23 +++++++++++++----------\n 1 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5b8afdc..d51aecb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -60,6 +60,14 @@ static struct ll_diff_driver {\n \tchar *cmd;\n } *user_diff, **user_diff_tail;\n \n+static int xstrdup_confval(const char *dest, const char *var, const char *value)\n+{\n+\tif (!value || !*value)\n+\t\treturn error(\"%s: lacks value\", var);\n+\tdest = xstrdup(value);\n+\treturn 0;\n+}\n+\n /*\n  * Currently there is only \"diff.<drivername>.command\" variable;\n  * because there are \"diff.color.<slot>\" variables, we are parsing\n@@ -86,10 +94,7 @@ static int parse_lldiff_command(const char *var, const char *ep, const char *val\n \t\tuser_diff_tail = &(drv->next);\n \t}\n \n-\tif (!value)\n-\t\treturn error(\"%s: lacks value\", var);\n-\tdrv->cmd = strdup(value);\n-\treturn 0;\n+\treturn xstrdup_confval(drv->cmd, var, value);\n }\n \n /*\n@@ -123,8 +128,8 @@ static int parse_funcname_pattern(const char *var, const char *ep, const char *v\n \t}\n \tif (pp->pattern)\n \t\tfree(pp->pattern);\n-\tpp->pattern = xstrdup(value);\n-\treturn 0;\n+\n+\treturn xstrdup_confval(pp->pattern, var, value);\n }\n \n /*\n@@ -157,10 +162,8 @@ int git_diff_ui_config(const char *var, const char *value)\n \t\tdiff_auto_refresh_index = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"diff.external\")) {\n-\t\texternal_diff_cmd_cfg = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"diff.external\"))\n+\t\treturn xstrdup_confval(external_diff_cmd_cfg, var, value);\n \tif (!prefixcmp(var, \"diff.\")) {\n \t\tconst char *ep = strrchr(var, '.');\n \n-- \n1.5.4.35.g3360\n"},{"id":"68247","messageId":"20080210190145.GB5129@artemis.madism.org","threadId":"12012","inReplyTo":"20080210193534.d11d8b15.chriscool@tuxfamily.org","subject":"Re: [PATCH] diff.c: guard config parser from value=NULL","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-02-10T19:01:45Z","receivedAt":"2008-02-10T19:01:45Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Feb 10, 2008 at 06:35:34PM +0000, Christian Couder wrote:\n> In fact we also guard for value=\"\" as it doesn't make more sense\n> for the variables here.\n> \n> We do that by using a new function 'xstrdup_confval' to avoid code\n> duplication.\n> \n> By the way this changes a 'strdup' into 'xstrdup'.\n> \n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  diff.c |   23 +++++++++++++----------\n>  1 files changed, 13 insertions(+), 10 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 5b8afdc..d51aecb 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -60,6 +60,14 @@ static struct ll_diff_driver {\n>  \tchar *cmd;\n>  } *user_diff, **user_diff_tail;\n>  \n> +static int xstrdup_confval(const char *dest, const char *var, const char *value)\n\n  Why not calling that \"git_config_string\" in config.[hc] and be\nAPI-consistent ?\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"68273","messageId":"20080210211341.GA24976@auto.tuwien.ac.at","threadId":"12012","inReplyTo":"20080210193534.d11d8b15.chriscool@tuxfamily.org","subject":"Re: [PATCH] diff.c: guard config parser from value=NULL","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-10T21:13:41Z","receivedAt":"2008-02-10T21:13:41Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 10, 2008 at 07:35:34PM +0100, Christian Couder wrote:\n> In fact we also guard for value=\"\" as it doesn't make more sense\n> for the variables here.\n> \n> We do that by using a new function 'xstrdup_confval' to avoid code\n> duplication.\n> \n> By the way this changes a 'strdup' into 'xstrdup'.\n> \n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  diff.c |   23 +++++++++++++----------\n>  1 files changed, 13 insertions(+), 10 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 5b8afdc..d51aecb 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -60,6 +60,14 @@ static struct ll_diff_driver {\n>  \tchar *cmd;\n>  } *user_diff, **user_diff_tail;\n>  \n> +static int xstrdup_confval(const char *dest, const char *var, const char *value)\n> +{\n> +\tif (!value || !*value)\n> +\t\treturn error(\"%s: lacks value\", var);\n> +\tdest = xstrdup(value);\n> +\treturn 0;\n> +}\n> +\n>  /*\n>   * Currently there is only \"diff.<drivername>.command\" variable;\n>   * because there are \"diff.color.<slot>\" variables, we are parsing\n\nThis function could be used in config.c too. So move it to a common file.\n\nmfg Martin Kögler\n"},{"id":"68323","messageId":"200802110555.19383.chriscool@tuxfamily.org","threadId":"12012","inReplyTo":"20080210190145.GB5129@artemis.madism.org","subject":"Re: [PATCH] diff.c: guard config parser from value=NULL","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-11T04:55:19Z","receivedAt":"2008-02-11T04:55:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le dimanche 10 février 2008, Pierre Habouzit a écrit :\n> On Sun, Feb 10, 2008 at 06:35:34PM +0000, Christian Couder wrote:\n> > +static int xstrdup_confval(const char *dest, const char *var, const\n> > char *value)\n>\n>   Why not calling that \"git_config_string\" in config.[hc] and be\n> API-consistent ?\n\nIn this function, there is:\n\nif (!value || !*value)\n\treturn error(\"%s: lacks value\", var);\n\nbut in some places an empty string is not an error, and in some other places \nwe \"die\" in case of error instead of using \"return error...\"\nSo I am not sure this function can be generally usefull as it is now.\n\nMaybe I should add a flag parameter to control if we should \"die\" or \"return \nerror\" and if we should accept an empty string or not.\n\nWhat do you think ?\n\nChristian.\n"}]}