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

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

[Andrew Klotz via GitGitGadget]

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.

Signed-off-by: Andrew Klotz agc.klotz@gmail.com [agc.klotz@gmail.com]

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

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

[Andrew Klotz via GitGitGadget]

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

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

[Jeff King]

On Fri, Sep 18, 2020 at 02:17:06AM +0000, Andrew Klotz via GitGitGadget wrote:

> 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
>  "

This test is pretty subtle and depends on gettext-poison's inner
workings. I wonder if it's worth adding a separate:

  git config --bool --default=foo does.not.exist

test in t1300 or similar.

-Peff

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

[Phillip Wood]

Hi Andrew

On 18/09/2020 03:17, Andrew Klotz via GitGitGadget wrote:
> 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.

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

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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).

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

> 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().

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(-)

diff --git a/config.c b/config.c
index 8db9c77098..801d944d2b 100644
--- a/config.c
+++ b/config.c
@@ -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)

[PATCH 2/2] formatting for error messages

[Andrew Klotz via GitGitGadget]

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

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 `_`.

Signed-off-by: Andrew Klotz <agc.klotz@gmail.com>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 198d0d3216..624fb30fdd 100644
--- a/config.c
+++ b/config.c
@@ -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

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

[Junio C Hamano]

"Andrew Klotz via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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().

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.

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

[Andrew Klotz via GitGitGadget]

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.

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

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(-)

diff --git a/config.c b/config.c
index 4c0cf3a1c15d..5e4fd6b5561b 100644
--- a/config.c
+++ b/config.c
@@ -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)
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
 "
 
 test_done

base-commit: 66e871b6647ffea61a77a0f82c7ef3415f1ee79c
-- 
gitgitgadget

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

[Jeff King]

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

> 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.

> 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.

> +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 :) ).

> @@ -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.

> 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)?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 09, 2021 at 01:25:08AM +0000, Andrew Klotz via GitGitGadget wrote:
>
>> 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.
>
>> 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.
>
>> +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 :) ).
>
>> @@ -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.

>> 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.

[PATCH v3] config: improve error message for boolean config

[Andrew Klotz via GitGitGadget]

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.

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

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

diff --git a/config.c b/config.c
index b922b4f28572..f90b633dba21 100644
--- a/config.c
+++ b/config.c
@@ -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)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 936d72041bab..e0dd5d65ceda 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -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

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

[Jeff King]

On Thu, Feb 11, 2021 at 08:30:53PM +0000, Andrew Klotz via GitGitGadget wrote:

> 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.
> 
> 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>

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

> diff --git a/config.c b/config.c
> index b922b4f28572..f90b633dba21 100644
> --- a/config.c
> +++ b/config.c
> @@ -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);
> +}

...if this is rebased on the current master that does not have
GIT_TEST_GETTEXT_POISON anymore, then I think this whole function can be
simplified down to the final line.

Since it looks like Junio applied this on top of v2.30.1, which still
has GETTEXT_POISON, we need it here. But we should remember to remove it
with the rest of the GETTEXT_POISON once it's all merged together.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thanks, all the changes here look good to me except for one curiosity:
>
>> diff --git a/config.c b/config.c
>> index b922b4f28572..f90b633dba21 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -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);
>> +}
>
> ...if this is rebased on the current master that does not have
> GIT_TEST_GETTEXT_POISON anymore, then I think this whole function can be
> simplified down to the final line.
>
> Since it looks like Junio applied this on top of v2.30.1, which still
> has GETTEXT_POISON, we need it here. But we should remember to remove it
> with the rest of the GETTEXT_POISON once it's all merged together.

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.

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

[Andrew Klotz]

Thanks for the feedback,

I updated the branch and moved:

  - boolean error message change out of git_config_bool_or_int to just 
in git_config_bool

  - added die_bad_boolean instead of modifying die_bad_number.

The advantage here is we only use a bool error message when we know the 
value
is a bool. Is this more along the lines of the suggestion?