config: improve error message for boolean config#841
Closed
KlotzAndrew wants to merge 1 commit intogit:masterfrom
Closed
config: improve error message for boolean config#841KlotzAndrew wants to merge 1 commit intogit:masterfrom
KlotzAndrew wants to merge 1 commit intogit:masterfrom
Conversation
Welcome to GitGitGadgetHi @KlotzAndrew, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
KlotzAndrew
force-pushed
the
better_bool_errors
branch
3 times, most recently
from
dd17e4d to
2f791ac
Compare
KlotzAndrew
changed the title
KlotzAndrew
force-pushed
the
better_bool_errors
branch
2 times, most recently
from
8aac139 to
8a4fecf
Compare
Member
|
/allow |
|
User KlotzAndrew is now allowed to use GitGitGadget. |
dscho
reviewed
Sep 14, 2020
KlotzAndrew
force-pushed
the
better_bool_errors
branch
from
8a4fecf to
1e9caf1
Compare
Contributor
Author
|
/preview |
|
Preview email sent as pull.841.git.git.1600394430.gitgitgadget@gmail.com |
Contributor
Author
|
/submit |
|
Submitted as pull.841.git.git.1600395427.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -996,15 +996,6 @@ static void die_bad_number(const char *name, const char *value) | |||
| if (!value) | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
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
|
User |
| @@ -996,15 +996,6 @@ static void die_bad_number(const char *name, const char *value) | |||
| if (!value) | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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
>
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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)
|
User |
KlotzAndrew
force-pushed
the
better_bool_errors
branch
from
1e9caf1 to
32dd4ee
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.841.v2.git.git.1612833909210.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
KlotzAndrew
force-pushed
the
better_bool_errors
branch
3 times, most recently
from
efc4492 to
33e0e2a
Compare
sosbmw12345
approved these changes
Feb 10, 2021
sosbmw12345
approved these changes
Feb 10, 2021
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>
KlotzAndrew
force-pushed
the
better_bool_errors
branch
from
33e0e2a to
45a51e1
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.841.v3.git.git.1613075454274.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via 72199fd. |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via 1ff8637. |
|
This patch series was integrated into next via 76784be. |
|
This patch series was integrated into seen via 483e09e. |
|
This patch series was integrated into next via 483e09e. |
|
This patch series was integrated into master via 483e09e. |
|
Closed via 483e09e. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_POISONbeing a boolean value, moving its special case out from
die_bad_number()and intogit_config_bool_or_int(). Analternative could be for
die_bad_number()to handle booleanvalues when erroring, although the function name might need to
change if it is handling non-numeric values.
changes since v1
git_config_bool_or_intto just in
git_config_booland addeddie_bad_booleaninstead ofmodifying
die_bad_number.changes since v2
die_bad_boolfromif (0 <= v)toif (v < 0)Signed-off-by: Andrew Klotz agc.klotz@gmail.com
cc: Jeff King peff@peff.net
cc: Phillip Wood phillip.wood123@gmail.com