# [PATCH] diff.c: guard config parser from value=NULL 4 messages from 2008-02-10 to 2008-02-11. Participants: Christian Couder, Pierre Habouzit, Martin Koegler. Thread: https://gitlist.dev/t/12012 ## Christian Couder, 2008-02-10 18:35 Subject: [PATCH] diff.c: guard config parser from value=NULL Message-ID: <20080210193534.d11d8b15.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20080210193534.d11d8b15.chriscool%40tuxfamily.org ``` In fact we also guard for value="" as it doesn't make more sense for the variables here. We do that by using a new function 'xstrdup_confval' to avoid code duplication. By the way this changes a 'strdup' into 'xstrdup'. Signed-off-by: Christian Couder --- diff.c | 23 +++++++++++++---------- 1 files changed, 13 insertions(+), 10 deletions(-) diff --git a/diff.c b/diff.c index 5b8afdc..d51aecb 100644 --- a/diff.c +++ b/diff.c @@ -60,6 +60,14 @@ static struct ll_diff_driver { char *cmd; } *user_diff, **user_diff_tail; +static int xstrdup_confval(const char *dest, const char *var, const char *value) +{ + if (!value || !*value) + return error("%s: lacks value", var); + dest = xstrdup(value); + return 0; +} + /* * Currently there is only "diff..command" variable; * because there are "diff.color." variables, we are parsing @@ -86,10 +94,7 @@ static int parse_lldiff_command(const char *var, const char *ep, const char *val user_diff_tail = &(drv->next); } - if (!value) - return error("%s: lacks value", var); - drv->cmd = strdup(value); - return 0; + return xstrdup_confval(drv->cmd, var, value); } /* @@ -123,8 +128,8 @@ static int parse_funcname_pattern(const char *var, const char *ep, const char *v } if (pp->pattern) free(pp->pattern); - pp->pattern = xstrdup(value); - return 0; + + return xstrdup_confval(pp->pattern, var, value); } /* @@ -157,10 +162,8 @@ int git_diff_ui_config(const char *var, const char *value) diff_auto_refresh_index = git_config_bool(var, value); return 0; } - if (!strcmp(var, "diff.external")) { - external_diff_cmd_cfg = xstrdup(value); - return 0; - } + if (!strcmp(var, "diff.external")) + return xstrdup_confval(external_diff_cmd_cfg, var, value); if (!prefixcmp(var, "diff.")) { const char *ep = strrchr(var, '.'); -- 1.5.4.35.g3360 ``` ## Pierre Habouzit, 2008-02-10 19:01 Subject: Re: [PATCH] diff.c: guard config parser from value=NULL Message-ID: <20080210190145.GB5129@artemis.madism.org> URL: https://gitlist.dev/e/20080210190145.GB5129%40artemis.madism.org In-Reply-To: <20080210193534.d11d8b15.chriscool@tuxfamily.org> ``` On Sun, Feb 10, 2008 at 06:35:34PM +0000, Christian Couder wrote: > In fact we also guard for value="" as it doesn't make more sense > for the variables here. > > We do that by using a new function 'xstrdup_confval' to avoid code > duplication. > > By the way this changes a 'strdup' into 'xstrdup'. > > Signed-off-by: Christian Couder > --- > diff.c | 23 +++++++++++++---------- > 1 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/diff.c b/diff.c > index 5b8afdc..d51aecb 100644 > --- a/diff.c > +++ b/diff.c > @@ -60,6 +60,14 @@ static struct ll_diff_driver { > char *cmd; > } *user_diff, **user_diff_tail; > > +static int xstrdup_confval(const char *dest, const char *var, const char *value) Why not calling that "git_config_string" in config.[hc] and be API-consistent ? -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Martin Koegler, 2008-02-10 21:13 Subject: Re: [PATCH] diff.c: guard config parser from value=NULL Message-ID: <20080210211341.GA24976@auto.tuwien.ac.at> URL: https://gitlist.dev/e/20080210211341.GA24976%40auto.tuwien.ac.at In-Reply-To: <20080210193534.d11d8b15.chriscool@tuxfamily.org> ``` On Sun, Feb 10, 2008 at 07:35:34PM +0100, Christian Couder wrote: > In fact we also guard for value="" as it doesn't make more sense > for the variables here. > > We do that by using a new function 'xstrdup_confval' to avoid code > duplication. > > By the way this changes a 'strdup' into 'xstrdup'. > > Signed-off-by: Christian Couder > --- > diff.c | 23 +++++++++++++---------- > 1 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/diff.c b/diff.c > index 5b8afdc..d51aecb 100644 > --- a/diff.c > +++ b/diff.c > @@ -60,6 +60,14 @@ static struct ll_diff_driver { > char *cmd; > } *user_diff, **user_diff_tail; > > +static int xstrdup_confval(const char *dest, const char *var, const char *value) > +{ > + if (!value || !*value) > + return error("%s: lacks value", var); > + dest = xstrdup(value); > + return 0; > +} > + > /* > * Currently there is only "diff..command" variable; > * because there are "diff.color." variables, we are parsing This function could be used in config.c too. So move it to a common file. mfg Martin Kögler ``` ## Christian Couder, 2008-02-11 04:55 Subject: Re: [PATCH] diff.c: guard config parser from value=NULL Message-ID: <200802110555.19383.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/200802110555.19383.chriscool%40tuxfamily.org In-Reply-To: <20080210190145.GB5129@artemis.madism.org> ``` Le dimanche 10 février 2008, Pierre Habouzit a écrit : > On Sun, Feb 10, 2008 at 06:35:34PM +0000, Christian Couder wrote: > > +static int xstrdup_confval(const char *dest, const char *var, const > > char *value) > > Why not calling that "git_config_string" in config.[hc] and be > API-consistent ? In this function, there is: if (!value || !*value) return error("%s: lacks value", var); but in some places an empty string is not an error, and in some other places we "die" in case of error instead of using "return error..." So I am not sure this function can be generally usefull as it is now. Maybe I should add a flag parameter to control if we should "die" or "return error" and if we should accept an empty string or not. What do you think ? Christian. ```