[PATCH 2/6] config: add 'git_config_string' to refactor string config variables.

[PATCH 2/6] config: add 'git_config_string' to refactor string config variables.

[Christian Couder]

In many places we just check if a value from the config file is not
NULL, then we duplicate it and return 0. This patch introduces the new
'git_config_string' function to do that.

This function is also used to refactor some code in 'config.c'.
Refactoring other files is left for other patches.

Also not all the code in "config.c" is refactored, because the function
takes a "const char **" as its first parameter, but in many places a
"char *" is used instead of a "const char *". (And C does not allow
using a "char **" instead of a "const char **" without a warning.)

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h  |    1 +
 config.c |   25 ++++++++++++-------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 3867ba7..cef058d 100644
--- a/cache.h
+++ b/cache.h
@@ -625,6 +625,7 @@ extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
 extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool(const char *, const char *);
+extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 3e72778..ca6af2d 100644
--- a/config.c
+++ b/config.c
@@ -309,6 +309,14 @@ int git_config_bool(const char *name, const char *value)
 	return git_config_int(name, value) != 0;
 }
 
+int git_config_string(const char **dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	*dest = xstrdup(value);
+	return 0;
+}
+
 int git_default_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -421,20 +429,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
-	if (!strcmp(var, "i18n.commitencoding")) {
-		if (!value)
-			return config_error_nonbool(var);
-		git_commit_encoding = xstrdup(value);
-		return 0;
-	}
-
-	if (!strcmp(var, "i18n.logoutputencoding")) {
-		if (!value)
-			return config_error_nonbool(var);
-		git_log_output_encoding = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "i18n.commitencoding"))
+		return git_config_string(&git_commit_encoding, var, value);
 
+	if (!strcmp(var, "i18n.logoutputencoding"))
+		return git_config_string(&git_log_output_encoding, var, value);
 
 	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
 		pager_use_color = git_config_bool(var,value);
-- 
1.5.4.1.129.gf12af-dirty

Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.

[Jeff King]

On Sat, Feb 16, 2008 at 06:00:24AM +0100, Christian Couder wrote:

> +int git_config_string(const char **dest, const char *var, const char *value)
> +{
> +	if (!value)
> +		return config_error_nonbool(var);
> +	*dest = xstrdup(value);
> +	return 0;
> +}

I'm not sure I see the point in using a 'const char *' as the
destination. The string isn't inherently const, since it is allocated on
the heap; callers are free to use it as they please (and are responsible
for freeing it; while constness doesn't impact calling free() at a
language level, I think that stylistically it is uncommon for a
heap-allocated string to be stored as 'const').

-Peff

Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.

[Christian Couder]

Le samedi 16 février 2008, Jeff King a écrit :
> On Sat, Feb 16, 2008 at 06:00:24AM +0100, Christian Couder wrote:
> > +int git_config_string(const char **dest, const char *var, const char
> > *value) +{
> > +	if (!value)
> > +		return config_error_nonbool(var);
> > +	*dest = xstrdup(value);
> > +	return 0;
> > +}
>
> I'm not sure I see the point in using a 'const char *' as the
> destination. The string isn't inherently const, since it is allocated on
> the heap; callers are free to use it as they please (and are responsible
> for freeing it; while constness doesn't impact calling free() at a
> language level, I think that stylistically it is uncommon for a
> heap-allocated string to be stored as 'const').

Well, in many places where this function could be used the dest string is 
a "const char *" and in many other places it's a "char *", but it feels 
safer to try to promote the latter into the former (like I did in the 
following patches) rather than the other way around.

Christian.

Re: [PATCH 2/6] config: add 'git_config_string' to refactor string config variables.

[Jeff King]

On Sat, Feb 16, 2008 at 07:21:42PM +0100, Christian Couder wrote:

> Well, in many places where this function could be used the dest string is 
> a "const char *" and in many other places it's a "char *", but it feels 
> safer to try to promote the latter into the former (like I did in the 
> following patches) rather than the other way around.

Hrm. I suppose that is true if you are talking about removing the
'const' from existing variables (which although pointing to non-const
allocated space in this instance, may in other code paths point to
actual const space).

But this is really a matter of C handling this poorly, because we have
to pass in a pointer rather than using the return value. So I think the
method that best reflects the desired behavior would be to actually cast
"const char *" to "char *" at the callsite. But unfortunately casts in C
are error-prone themselves, and make the code a little harder to read.

So perhaps your solution is a reasonable compromise.

-Peff