[PATCH] config: add support for --bool and --int while setting values

[PATCH] config: add support for --bool and --int while setting values

[Frank Lichtenheld]

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 Documentation/git-config.txt    |    9 +++---
 builtin-config.c                |   54 +++++++++++++++++++++++++++++---------
 t/t1300-repo-config.sh          |   48 +++++++++++++++++++++++++++++++++-
 t/t9400-git-cvsserver-server.sh |    4 +-
 4 files changed, 94 insertions(+), 21 deletions(-)

 There are probably more elegant ways to do this...
 But it seems to work.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index f2c6717..ddb1dea 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git-config' [--system | --global] name [value [value_regex]]
-'git-config' [--system | --global] --add name value
-'git-config' [--system | --global] --replace-all name [value [value_regex]]
+'git-config' [--system | --global] [type] name [value [value_regex]]
+'git-config' [--system | --global] [type] --add name value
+'git-config' [--system | --global] [type] --replace-all name [value [value_regex]]
 'git-config' [--system | --global] [type] --get name [value_regex]
 'git-config' [--system | --global] [type] --get-all name [value_regex]
 'git-config' [--system | --global] --unset name [value_regex]
@@ -36,8 +36,7 @@ prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 The type specifier can be either '--int' or '--bool', which will make
 'git-config' ensure that the variable(s) are of the given type and
 convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool).  Type specifiers currently only
-take effect for reading operations.  If no type specifier is passed,
+a "true" or "false" string for bool).  If no type specifier is passed,
 no checks or transformations are performed on the value.
 
 This command will fail if:
diff --git a/builtin-config.c b/builtin-config.c
index b2515f7..3aa4645 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -131,9 +131,32 @@ free_strings:
 	return ret;
 }
 
+char* normalize_value(const char* key, const char* value)
+{
+	char* normalized;
+
+	if (!value)
+		return NULL;
+
+	if (type == T_RAW)
+		normalized = xstrdup(value);
+	else {
+		normalized = xmalloc(64);
+		if (type == T_INT)
+			sprintf(normalized, "%d",
+				git_config_int(key, value?value:""));
+		else if (type == T_BOOL)
+			sprintf(normalized, "%s",
+				git_config_bool(key, value) ? "true" : "false");
+	}
+
+	return normalized;
+}
+
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = 0;
+	char* value;
 	setup_git_directory_gently(&nongit);
 
 	while (1 < argc) {
@@ -205,9 +228,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			use_key_regexp = 1;
 			do_all = 1;
 			return get_value(argv[2], NULL);
-		} else
-
-			return git_config_set(argv[1], argv[2]);
+		} else {
+			value = normalize_value(argv[1], argv[2]);
+			return git_config_set(argv[1], value);
+		}
 	case 4:
 		if (!strcmp(argv[1], "--unset"))
 			return git_config_set_multivar(argv[2], NULL, argv[3], 0);
@@ -223,17 +247,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			use_key_regexp = 1;
 			do_all = 1;
 			return get_value(argv[2], argv[3]);
-		} else if (!strcmp(argv[1], "--add"))
-			return git_config_set_multivar(argv[2], argv[3], "^$", 0);
-		else if (!strcmp(argv[1], "--replace-all"))
-
-			return git_config_set_multivar(argv[2], argv[3], NULL, 1);
-		else
-
-			return git_config_set_multivar(argv[1], argv[2], argv[3], 0);
+		} else if (!strcmp(argv[1], "--add")) {
+			value = normalize_value(argv[2], argv[3]);
+			return git_config_set_multivar(argv[2], value, "^$", 0);
+		} else if (!strcmp(argv[1], "--replace-all")) {
+			value = normalize_value(argv[2], argv[3]);
+			return git_config_set_multivar(argv[2], value, NULL, 1);
+		} else {
+			value = normalize_value(argv[1], argv[2]);
+			return git_config_set_multivar(argv[1], value, argv[3], 0);
+		}
 	case 5:
-		if (!strcmp(argv[1], "--replace-all"))
-			return git_config_set_multivar(argv[2], argv[3], argv[4], 1);
+		if (!strcmp(argv[1], "--replace-all")) {
+			value = normalize_value(argv[2], argv[3]);
+			return git_config_set_multivar(argv[2], value, argv[4], 1);
+		}
 	case 1:
 	default:
 		usage(git_config_set_usage);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7731fa7..4234d83 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -465,11 +465,57 @@ test_expect_success bool '
         done &&
 	cmp expect result'
 
-test_expect_failure 'invalid bool' '
+test_expect_failure 'invalid bool (--get)' '
 
 	git-config bool.nobool foobar &&
 	git-config --bool --get bool.nobool'
 
+test_expect_failure 'invalid bool (set)' '
+
+	git-config --bool bool.nobool foobar'
+
+rm .git/config
+
+cat > expect <<\EOF
+[bool]
+	true1 = true
+	true2 = true
+	true3 = true
+	true4 = true
+	false1 = false
+	false2 = false
+	false3 = false
+	false4 = false
+EOF
+
+test_expect_success 'set --bool' '
+
+	git-config --bool bool.true1 01 &&
+	git-config --bool bool.true2 -1 &&
+	git-config --bool bool.true3 YeS &&
+	git-config --bool bool.true4 true &&
+	git-config --bool bool.false1 000 &&
+	git-config --bool bool.false2 "" &&
+	git-config --bool bool.false3 nO &&
+	git-config --bool bool.false4 FALSE &&
+	cmp expect .git/config'
+
+rm .git/config
+
+cat > expect <<\EOF
+[int]
+	val1 = 1
+	val2 = -1
+	val3 = 5242880
+EOF
+
+test_expect_success 'set --int' '
+
+	git-config --int int.val1 01 &&
+	git-config --int int.val2 -1 &&
+	git-config --int int.val3 5m &&
+	cmp expect .git/config'
+
 rm .git/config
 
 git-config quote.leading " test"
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 0331770..641303e 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -38,7 +38,7 @@ echo >empty &&
   git commit -q -m "First Commit" &&
   git clone -q --local --bare "$WORKDIR/.git" "$SERVERDIR" >/dev/null 2>&1 &&
   GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled true &&
-  GIT_DIR="$SERVERDIR" git config --bool gitcvs.logfile "$SERVERDIR/gitcvs.log" ||
+  GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" ||
   exit 1
 
 # note that cvs doesn't accept absolute pathnames
@@ -255,7 +255,7 @@ rm -fr "$SERVERDIR"
 cd "$WORKDIR" &&
 git clone -q --local --bare "$WORKDIR/.git" "$SERVERDIR" >/dev/null 2>&1 &&
 GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled true &&
-GIT_DIR="$SERVERDIR" git config --bool gitcvs.logfile "$SERVERDIR/gitcvs.log" ||
+GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" ||
 exit 1
 
 test_expect_success 'cvs update (create new file)' \
-- 
1.5.2.1

Re: [PATCH] config: add support for --bool and --int while setting values

[Johannes Sixt]

Frank Lichtenheld wrote:
> 
> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>

Please excuse if I'm missing the big picture, but why do we need this
change?

-- Hannes

Re: [PATCH] config: add support for --bool and --int while setting values

[Frank Lichtenheld]

On Mon, Jun 25, 2007 at 04:06:34PM +0200, Johannes Sixt wrote:
> Frank Lichtenheld wrote:
> > 
> > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
> 
> Please excuse if I'm missing the big picture, but why do we need this
> change?

- Of course the user or script calling git-config can do the
  normalization and error checking, if they want to. But I would
  prefer to have it available in git-config.
- I would prefer that these options wouldn't be silently ignored,
  because that can be confusing (at least it is documented now, but
  still). So we should either using them or error out. I prefer the former.

Something that I forgot to mention in the previous mail:
One real problem with the patch is that it expands the k,m,g suffixes
for integer values. It probably shouldn't do that.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] config: add support for --bool and --int while setting values

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> On Mon, Jun 25, 2007 at 04:06:34PM +0200, Johannes Sixt wrote:
>> Frank Lichtenheld wrote:
>> > 
>> > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
>> 
>> Please excuse if I'm missing the big picture, but why do we need this
>> change?
>
> - Of course the user or script calling git-config can do the
>   normalization and error checking, if they want to. But I would
>   prefer to have it available in git-config.
> - I would prefer that these options wouldn't be silently ignored,
>   because that can be confusing (at least it is documented now, but
>   still). So we should either using them or error out. I prefer the former.
>
> Something that I forgot to mention in the previous mail:
> One real problem with the patch is that it expands the k,m,g suffixes
> for integer values. It probably shouldn't do that.

How about doing something like this, then?

git_config_int() knows that a missing value is a nonsense and
barfs on such an input, so (value ? value : "") is redundant
here.  Besides, you check value == NULL much earlier in this
function.


diff --git a/builtin-config.c b/builtin-config.c
index 9973f94..33b60ec 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -148,10 +148,11 @@ char* normalize_value(const char* key, const char* value)
 	if (type == T_RAW)
 		normalized = xstrdup(value);
 	else {
-		normalized = xmalloc(64);
-		if (type == T_INT)
-			sprintf(normalized, "%d",
-				git_config_int(key, value?value:""));
+		normalized = xmalloc(64 + strlen(value));
+		if (type == T_INT) {
+			int v = git_config_int(key, value);
+			sprintf(normalized, "%d # %s", v, value);
+		}
 		else if (type == T_BOOL)
 			sprintf(normalized, "%s",
 				git_config_bool(key, value) ? "true" : "false");

Re: [PATCH] config: add support for --bool and --int while setting values

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Frank Lichtenheld <frank@lichtenheld.de> writes:
>
>> Something that I forgot to mention in the previous mail:
>> One real problem with the patch is that it expands the k,m,g suffixes
>> for integer values. It probably shouldn't do that.
>
> How about doing something like this, then?

Nah, that wouldn't work, as normalize_value's return value is
parsed and quoted.  Grumble....