Re: [PATCH v2] config: improve error message for boolean config

Jeff King [off-list ref] writes:

On Tue, Feb 09, 2021 at 01:25:08AM +0000, Andrew Klotz via GitGitGadget wrote:

quoted

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.

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

quoted

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'

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.

quoted

+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);
+}

The duplication is ugly, but I think it's the least-bad solution (and I
still dream that GETTEXT_POISON may one day go away :) ).

quoted

@@ -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);

I had to look at this a minute to be sure we always returned a value.
But the compiler knows that die_bad_bool() doesn't return, and that we
always take one of the two conditionals.

I do think it might be easier to read as:

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

but I admit that's nit-picking.

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

quoted

diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index f9fa16ad8363..b66d34c6f2bc 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
 "

Do we want a separate test in t1300 that doesn't rely on GETTEXT_POISON
continuing to hang around (the idea has been thrown around elsewhere of
it maybe going away entirely)?

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.