config: improve error message for boolean config

Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value.

The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out fromdie_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values.

Andrew Klotz (2):
config: improve error message for boolean config formatting for error messages

 config.c                  | 26 ++++++++++++++------------
 t/t0205-gettext-poison.sh |  2 +-
 2 files changed, 15 insertions(+), 13 deletions(-)

base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v1
Pull-Request: https://github.com/git/git/pull/841

gitgitgadget

From: Andrew Klotz <[email protected]>

Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int.

GIT_TEST_GETTEXT_POISON is a boolean so we no longer fail on evaluating it as an int in git_config_int. Because of that we can move the special translation case into the boolean config check where we are now failing with an updated message

before with an invalid boolean value of non-boolean, its unclear what numeric is referring to:

```
fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit
```

now the error message mentions non-boolean is a bad boolean value:

```
fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'
```

---
 config.c                  | 22 ++++++++++++----------
 t/t0205-gettext-poison.sh |  2 +-
 2 files changed, 13 insertions(+), 11 deletions(-)

index 2bdff4457b..198d0d3216 100644
@@ -996,15 +996,6 @@ static void die_bad_number(const char *name, const char *value)
 	if (!value)
 		value = "";
 
-	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
-		/*
-		 * We explicitly *don't* use _() here since it would
-		 * cause an infinite loop with _() needing to call
-		 * use_gettext_poison(). This is why marked up
-		 * translations with N_() above.
-		 */
-		die(bad_numeric, value, name, error_type);
-
 	if (!(cf && cf->name))
 		die(_(bad_numeric), value, name, _(error_type));
 

@@ -1097,7 +1088,18 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 		return v;
 	}
 	*is_bool = 0;
-	return git_config_int(name, value);
+	if (git_parse_int(value, &v))
+		return v;
+
+	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+		/*
+		 * We explicitly *don't* use _() here since it would
+		 * cause an infinite loop with _() needing to call
+		 * use_gettext_poison().
+		 */
+		die("bad boolean config value '%s' for '%s'", value, name);
+	else
+		die(_("bad boolean config value '%s' for '%s'"), value, name);
 }
 
 int git_config_bool(const char *name, const char *value)

index f9fa16ad83..b66d34c6f2 100755
@@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
 
 test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
 	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
-	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
+	grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error
 "
 
 test_done

gitgitgadget

From: Andrew Klotz <[email protected]>

we no longer need to use N_ instead of _ here.

we had been using N_ instead of _ in the case this function was being called by with GIT_TEST_GETTEXT_POISON leading to an infinite loop.

Since we have moved GIT_TEST_GETTEXT_POISON out of this function, (it is evaluated as a boolean, not an int) we no longer need to handle the special case and can use _.

---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 198d0d3216..624fb30fdd 100644
@@ -990,8 +990,8 @@ NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
 	const char *error_type = (errno == ERANGE) ?
-		N_("out of range") : N_("invalid unit");
-	const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s");
+		_("out of range") : _("invalid unit");
+	const char *bad_numeric = _("bad numeric config value '%s' for '%s': %s");
 
 	if (!value)
 		value = "";

gitgitgadget

The approach does not make anything worse than what we currently have, which is good.

I am undecided if we want to apply 2/2, or if we want to apply 1/2 alone without 2/2. If we applied 2/2, those who are reading the code in a year who forgot about this review thread would have to wonder if all values assigned to the variable bad_numeric are enclosed in _() and go up to find all assignments.

Omitting 2/2 would keep _() around the message string fed to die(), so it may be easier to immediately see that the call to die is not missing basic i18n, but there is a risk to forget marking with N_().

If we were to use 2/2 in addition to 1/2, then squashing them into one commit will make the result easier to follow, because we no longer need an untranslated string in bad_numeric after 1/2 is applied. We are losing "the reason why we use N_() is..." comment in 1/2 anyway so doing what 2/2 does in the same commit would be more sensible than splitting these into two patches.

I dunno.

Thanks.

Hi Andrew

This patch improves things for boolean config keys but git_config_bool_or_int() is used by status.submoduleSummary, merge.log and commit.verbose which can be either a number or a boolean (where a boolean generally means use a default number). It would be a more invasive change but I wonder if it would be better for git_config_bool() to have it's own error message rather sharing it with git_config_bool_or_int().

Best Wishes

Phillip

Excellent point. The rephrasing done by the patch as-is will be a regression.

You'd probably need a variant of die_bad_numer(), with an untested patch like below.

 config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 8db9c77098..801d944d2b 100644
@@ -1097,7 +1097,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 		return v;
 	}
 	*is_bool = 0;
-	return git_config_int(name, value);
+
+	if (!git_parse_int(value, &v))
+		die_bad_number_or_bool(name, value);
+	return v;
 }
 
 int git_config_bool(const char *name, const char *value)

From: Andrew Klotz <[email protected]>

Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int.

before with an invalid boolean value of non-boolean, its unclear what numeric is referring to:

```
fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit
```

now the error message mentions non-boolean is a bad boolean value:

```
fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'
```

---
config: improve error message for boolean config

Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value.

The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out from die_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values.

changes since v1

* moved boolean error message change out of git_config_bool_or_int to just in git_config_bool and added die_bad_boolean instead of modifying die_bad_number.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v2
Pull-Request: https://github.com/git/git/pull/841

Range-diff vs v1:

1: 689d84672422 ! 1: 32dd4ee1e373 config: improve error message for boolean config
@@ Commit message
boolean error message when we know the value is neither a bool text or int.

- GIT_TEST_GETTEXT_POISON is a boolean so we no longer fail on
- evaluating it as an int in git_config_int. Because of that we can
- move the special translation case into the boolean config check where
- we are now failing with an updated message
-
before with an invalid boolean value of non-boolean, its unclear what numeric is referring to:
```
@@ Commit message

## config.c ##
@@ config.c: static void die_bad_number(const char *name, const char *value)
- if (!value)
- value = "";
-
-- if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
-- /*
-- * We explicitly *don't* use _() here since it would
-- * cause an infinite loop with _() needing to call
-- * use_gettext_poison(). This is why marked up
-- * translations with N_() above.
-- */
-- die(bad_numeric, value, name, error_type);
--
- if (!(cf && cf->name))
- die(_(bad_numeric), value, name, _(error_type));
-
-@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
- return v;
}
- *is_bool = 0;
-- return git_config_int(name, value);
-+ if (git_parse_int(value, &v))
-+ return v;
-+
+ }
+
++NORETURN
++static void die_bad_bool(const char *name, const char *value)
++{
+ if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+ /*
+ * We explicitly *don't* use _() here since it would
@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *i
+ die("bad boolean config value '%s' for '%s'", value, name);
+ else
+ die(_("bad boolean config value '%s' for '%s'"), value, name);
- }
++}
++
+ int git_config_int(const char *name, const char *value)
+ {
+ int ret;
+@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *is_bool)

int git_config_bool(const char *name, const char *value)
+ {
+- int discard;
+- return !!git_config_bool_or_int(name, value, &discard);
++ int v = git_parse_maybe_bool(value);
++ if (0 <= v)
++ return v;
++ else
++ die_bad_bool(name, value);
+ }
+
+ int git_config_string(const char **dest, const char *var, const char *value)

## t/t0205-gettext-poison.sh ##
@@ t/t0205-gettext-poison.sh: test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
2: 1e9caf1911d3 < -: ------------ formatting for error messages

 config.c                  | 21 +++++++++++++++++++--
 t/t0205-gettext-poison.sh |  2 +-
 2 files changed, 20 insertions(+), 3 deletions(-)

index 4c0cf3a1c15d..5e4fd6b5561b 100644
@@ -1030,6 +1030,20 @@ static void die_bad_number(const char *name, const char *value)
 	}
 }
 
+NORETURN
+static void die_bad_bool(const char *name, const char *value)
+{
+	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+		/*
+		 * We explicitly *don't* use _() here since it would
+		 * cause an infinite loop with _() needing to call
+		 * use_gettext_poison().
+		 */
+		die("bad boolean config value '%s' for '%s'", value, name);
+	else
+		die(_("bad boolean config value '%s' for '%s'"), value, name);
+}
+
 int git_config_int(const char *name, const char *value)
 {
 	int ret;

@@ -1102,8 +1116,11 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 
 int git_config_bool(const char *name, const char *value)
 {
-	int discard;
-	return !!git_config_bool_or_int(name, value, &discard);
+	int v = git_parse_maybe_bool(value);
+	if (0 <= v)
+		return v;
+	else
+		die_bad_bool(name, value);
 }
 
 int git_config_string(const char **dest, const char *var, const char *value)

index f9fa16ad8363..b66d34c6f2bc 100755
@@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
 
 test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
 	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
-	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
+	grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error
 "
 
 test_done

base-commit: 66e871b6647ffea61a77a0f82c7ef3415f1ee79c

gitgitgadget

Thanks for keeping at this. The goal makes sense. The implementation looks OK to me, but I had a few really tiny comments.

Our commit messages aren't generally formatted as markdown. So this looks a little nicer using indentation (which also happens to generate nice markdown output):

numeric is referring to:

fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit

now the error message mentions non-boolean is a bad boolean value:

fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'

Not worth a re-roll on its own, though.

  int v = git_parse_maybe_bool(value);
  if (v < 0)
          die_bad_bool(name, value);
  return v;

Combined together with the log message formatting, they already make it worth an updated patch.

Yeah, I do not think it is a good idea to add more test that depends on GETTEXT_POISON and assumes how it works.

We certainly wish that we want to ensure more quality translation and more complete i18n coverage, but the "pretend as if the test passed if it tries to judge the success/failure by looking at the output string from the command that can be translated, to ferret out mistakenly marked machine readable string for translation", which is what the current GETTEXT_POISON thing is, does not contribute much to improving translation.

From: Andrew Klotz <[email protected]>

Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int.

before with an invalid boolean value of non-boolean, its unclear what numeric is referring to:
fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit

now the error message mentions non-boolean is a bad boolean value:
fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'

---
config: improve error message for boolean config

Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value.

The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out from die_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values.

changes since v1

* moved boolean error message change out of git_config_bool_or_int to just in git_config_bool and added die_bad_boolean instead of modifying die_bad_number.

changes since v2

* added a test for boolean config values
* changed the condition to hit die_bad_bool from if (0 <= v) to if (v <
0)
* removed change in get-text-poison.sh test

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v3
Pull-Request: https://github.com/git/git/pull/841

Range-diff vs v2:

1: 32dd4ee1e373 ! 1: 45a51e18c1d3 config: improve error message for boolean config
@@ Commit message

before with an invalid boolean value of non-boolean, its unclear what numeric is referring to:
- ```
- fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit
- ```
+ fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit

now the error message mentions non-boolean is a bad boolean value:
- ```
- fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'
- ```
+ fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign'

@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *i
- int discard;
- return !!git_config_bool_or_int(name, value, &discard);
+ int v = git_parse_maybe_bool(value);
-+ if (0 <= v)
-+ return v;
-+ else
++ if (v < 0)
+ die_bad_bool(name, value);
++ return v;
}

int git_config_string(const char **dest, const char *var, const char *value)

- ## t/t0205-gettext-poison.sh ##
-@@ t/t0205-gettext-poison.sh: test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant -
- test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
- test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
-- grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
-+ grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error
- "
+ ## t/t1300-config.sh ##
+@@ t/t1300-config.sh: test_expect_success 'invalid unit' '
+ test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
+ '

- test_done
++test_expect_success 'invalid unit boolean' '
++ git config commit.gpgsign "1true" &&
++ test_cmp_config 1true commit.gpgsign &&
++ test_must_fail git config --bool --get commit.gpgsign 2>actual &&
++ test_i18ngrep "bad boolean config value .1true. for .commit.gpgsign." actual
++'
++
+ test_expect_success 'line number is reported correctly' '
+ printf "[bool]\n\tvar\n" >invalid &&
+ test_must_fail git config -f invalid --path bool.var 2>actual &&

 config.c          | 20 ++++++++++++++++++--
 t/t1300-config.sh |  7 +++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

index b922b4f28572..f90b633dba21 100644
@@ -1180,6 +1180,20 @@ static void die_bad_number(const char *name, const char *value)
 	}
 }
 
+NORETURN
+static void die_bad_bool(const char *name, const char *value)
+{
+	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+		/*
+		 * We explicitly *don't* use _() here since it would
+		 * cause an infinite loop with _() needing to call
+		 * use_gettext_poison().
+		 */
+		die("bad boolean config value '%s' for '%s'", value, name);
+	else
+		die(_("bad boolean config value '%s' for '%s'"), value, name);
+}
+
 int git_config_int(const char *name, const char *value)
 {
 	int ret;

@@ -1252,8 +1266,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 
 int git_config_bool(const char *name, const char *value)
 {
-	int discard;
-	return !!git_config_bool_or_int(name, value, &discard);
+	int v = git_parse_maybe_bool(value);
+	if (v < 0)
+		die_bad_bool(name, value);
+	return v;
 }
 
 int git_config_string(const char **dest, const char *var, const char *value)

index 936d72041bab..e0dd5d65ceda 100755
@@ -675,6 +675,13 @@ test_expect_success 'invalid unit' '
 	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
 
+test_expect_success 'invalid unit boolean' '
+	git config commit.gpgsign "1true" &&
+	test_cmp_config 1true commit.gpgsign &&
+	test_must_fail git config --bool --get commit.gpgsign 2>actual &&
+	test_i18ngrep "bad boolean config value .1true. for .commit.gpgsign." actual
+'
+
 test_expect_success 'line number is reported correctly' '
 	printf "[bool]\n\tvar\n" >invalid &&
 	test_must_fail git config -f invalid --path bool.var 2>actual &&

base-commit: c6102b758572c7515f606b2423dfe38934fe6764

gitgitgadget

Thanks, all the changes here look good to me except for one curiosity:

Thanks for being extra careful. I sort-of considered that this was a bugfix and that was why it is made mergeable down to the maint track, but I do not mind making this one of the "incentives" to upgrade for our users ;-)

I actually suspect that without any of us worrying about it, Ævar will take care of the cleaning up when the dust settles.

Thanks.