[PATCH] diff.c: guard config parser from value=NULL

[PATCH] diff.c: guard config parser from value=NULL

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 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.<drivername>.command" variable;
  * because there are "diff.color.<slot>" 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

Re: [PATCH] diff.c: guard config parser from value=NULL

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

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 <chriscool@tuxfamily.org>
> ---
>  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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] diff.c: guard config parser from value=NULL

[Christian Couder]

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.

Re: [PATCH] diff.c: guard config parser from value=NULL

[Martin Koegler]

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 <chriscool@tuxfamily.org>
> ---
>  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.<drivername>.command" variable;
>   * because there are "diff.color.<slot>" variables, we are parsing

This function could be used in config.c too. So move it to a common file.

mfg Martin Kögler