[PATCH 1/2] config: improve error message for boolean config

From: Andrew Klotz via GitGitGadget <gitgitgadget@gmail.com>
Date: Sep 18, 2020, 02:17 UTC
Message-ID: <689d84672422dd96d1eb89ea6137e79ce4030248.1600395427.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.841.git.git.1600395427.gitgitgadget@gmail.com>

From: Andrew Klotz <agc.klotz@gmail.com>

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' ```

Signed-off-by: Andrew Klotz <agc.klotz@gmail.com>
---
 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

-- 
gitgitgadget