Hi Andrew
On 18/09/2020 03:17, Andrew Klotz via GitGitGadget wrote:
From: Andrew Klotz <agc.klotz@xxxxxxxxx>
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.
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
`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'
```
Signed-off-by: Andrew Klotz <agc.klotz@xxxxxxxxx>
---
config.c | 22 ++++++++++++----------
t/t0205-gettext-poison.sh | 2 +-
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c
index 2bdff4457b..198d0d3216 100644
--- a/config.c
+++ b/config.c
@@ -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)
diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index f9fa16ad83..b66d34c6f2 100755
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -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
