git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Re: [PATCH 1/2] config: improve error message for boolean config
@ 2021-01-23 23:15 Andrew Klotz
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Klotz @ 2021-01-23 23:15 UTC (permalink / raw)
  To: gitster; +Cc: phillip.wood123, peff, gitgitgadget, git, agc.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?


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] config: improve error message for boolean config
  2020-09-18 18:30   ` Phillip Wood
@ 2020-09-18 19:14     ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2020-09-18 19:14 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Andrew Klotz via GitGitGadget, git, Andrew Klotz, Jeff King

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)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] config: improve error message for boolean config
  2020-09-18  2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget
  2020-09-18 18:10   ` Jeff King
@ 2020-09-18 18:30   ` Phillip Wood
  2020-09-18 19:14     ` Junio C Hamano
  1 sibling, 1 reply; 5+ messages in thread
From: Phillip Wood @ 2020-09-18 18:30 UTC (permalink / raw)
  To: Andrew Klotz via GitGitGadget, git
  Cc: Andrew Klotz, Jeff King, Junio C Hamano

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
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] config: improve error message for boolean config
  2020-09-18  2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget
@ 2020-09-18 18:10   ` Jeff King
  2020-09-18 18:30   ` Phillip Wood
  1 sibling, 0 replies; 5+ messages in thread
From: Jeff King @ 2020-09-18 18:10 UTC (permalink / raw)
  To: Andrew Klotz via GitGitGadget; +Cc: git, Andrew Klotz

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] config: improve error message for boolean config
  2020-09-18  2:17 [PATCH 0/2] " Andrew Klotz via GitGitGadget
@ 2020-09-18  2:17 ` Andrew Klotz via GitGitGadget
  2020-09-18 18:10   ` Jeff King
  2020-09-18 18:30   ` Phillip Wood
  0 siblings, 2 replies; 5+ messages in thread
From: Andrew Klotz via GitGitGadget @ 2020-09-18  2:17 UTC (permalink / raw)
  To: git; +Cc: Andrew Klotz, Andrew Klotz

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-01-23 23:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-23 23:15 [PATCH 1/2] config: improve error message for boolean config Andrew Klotz
  -- strict thread matches above, loose matches on Subject: below --
2020-09-18  2:17 [PATCH 0/2] " Andrew Klotz via GitGitGadget
2020-09-18  2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget
2020-09-18 18:10   ` Jeff King
2020-09-18 18:30   ` Phillip Wood
2020-09-18 19:14     ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).