threads / patch / 12128

patch, 6 partsconfig: add 'git_config_string' to refactor string config variables.

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

## tl;dr

4 messages between Feb 16, 2008 and Feb 16, 2008. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Christian Couder· Feb 16, 2008, 05:00 UTC · lore

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(-)
Show changes to 2 files +13 −13

cache.h, config.c

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
Jeff King· Feb 16, 2008, 11:53 UTC · re: Christian Couder · lore

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

On Sat, Feb 16, 2008 at 06:00:24AM +0100, Christian Couder wrote:
Show 7 quoted lines
> +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
Christian Couder· Feb 16, 2008, 18:21 UTC · re: Jeff King · lore

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

Le samedi 16 février 2008, Jeff King a écrit :
Show 15 quoted lines
> 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.
Jeff King· Feb 16, 2008, 18:33 UTC · re: Christian Couder · lore

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

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

← back to recent threads