config: suggest the correct form when key contains "="#2302
Open
HaraldNordgren wants to merge 2 commits into
Open
config: suggest the correct form when key contains "="#2302HaraldNordgren wants to merge 2 commits into
HaraldNordgren wants to merge 2 commits into
Conversation
9add914 to
56eb3ce
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2302.git.git.1778680725459.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When a user types "git config foo.bar=baz", git_config_parse_key()
> rejects the key with "error: invalid key: foo.bar=baz" but gives no
> indication of what the user should have written. The mistake is a
> common one for users who reach for INI-file syntax or for the
> "--flag=value" convention used by other command-line tools.
>
> Since "=" is never a valid character in a config key, treat its
> presence as a strong signal of this specific mistake and follow the
> error with a one-line suggestion in the "(did you mean ...)" style
> used elsewhere in git, e.g.:
>
> $ git config pull.rebase=false
> error: invalid key: pull.rebase=false
> (did you mean "git config set pull.rebase false"?)
If the command line were
git config get foo.bar=baz
git config set foo.bar=baz nitfol
we shouldn't give an extra "did you mean?" at all.
The only cases you may want to do the "did you mean?" I think are
git config foo.bar=baz
git config set foo.bar=baz
And I think git_config_parse_key() is at a way too low level to tell
in what context we are seeing this faulty key to guess end-user's
intention to limit our "did you mean?"
I also wonder if, given that "=" in anywhere other than three-level
names, is invalid, we should just start accept
git config foo.bar=baz
git config set foo.bar=baz
and interpret them as
git config set foo.bar baz
We of course need to be careful about non-invalid keys, i.e.
git config foo.bar=baz.boo
is a request to read the value of that named variable, i.e.
[foo "bar=baz"]
boo = its value
so either you start offering unsolicited "did you mean?" or accepting
tokens with '=' in them as new style "set", you need to be extra
careful not to trigger a false positive. |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > I also wonder if, given that "=" in anywhere other than three-level
> names, is invalid, we should just start accept
>
> git config foo.bar=baz
> git config set foo.bar=baz
>
> and interpret them as
>
> git config set foo.bar baz
That sounds good too! Probably even better.
Harald |
56eb3ce to
24a8eee
Compare
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Harald Nordgren <haraldnordgren@gmail.com> writes:
>> I also wonder if, given that "=" in anywhere other than three-level
>> names, is invalid, we should just start accept
>>
>> git config foo.bar=baz
>> git config set foo.bar=baz
>>
>> and interpret them as
>>
>> git config set foo.bar baz
>
> That sounds good too! Probably even better.
>
>
> Harald
Why do I get the above, which apparently is a response to my review
for
[PATCH] config: suggest the correct form when key contains "="
under this thread? Am I dealing with some sort of mechanical slop? |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > Why do I get the above, which apparently is a response to my review
> for
>
> [PATCH] config: suggest the correct form when key contains "="
>
> under this thread? Am I dealing with some sort of mechanical slop?
I think the problem here is my email sending process is not good. I edit
all the emails in Sublime text, where I keep the same file for all
different threads.
I have the subject line as the first line of the file and like you notice I
forget to change it sometimes.
I keep each of the topics bookmarked like this,
https://lore.kernel.org/git/xmqqecjdea13.fsf@gitster.g/, and then utilize
that like to send the email
```
git send-email \
--in-reply-to=xmqqecjdea13.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=haraldnordgren@gmail.com \
/path/to/YOUR_REPLY
```
I tried playing with neomutt and and email client replacement, but that
adds the complexity of downloading a new mbox file for each reply, it
didn't seem easier, but maybe it is.
How do you handle emails?
Harald |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > Why do I get the above, which apparently is a response to my review
> for
>
> [PATCH] config: suggest the correct form when key contains "="
>
> under this thread? Am I dealing with some sort of mechanical slop?
(Testing plain text email sending via Gmail for a less error-prone
workflow, does it still add the CC's correctly?)
Harald |
|
"Kristoffer Haugsbakk" wrote on the Git mailing list (how to reply to this email): On Fri, May 15, 2026, at 09:56, Harald Nordgren wrote:
>> Why do I get the above, which apparently is a response to my review
>> for
>>
>> [PATCH] config: suggest the correct form when key contains "="
>>
>> under this thread? Am I dealing with some sort of mechanical slop?
>
> I think the problem here is my email sending process is not good. I edit
> all the emails in Sublime text, where I keep the same file for all
> different threads.
>
> I have the subject line as the first line of the file and like you notice I
> forget to change it sometimes.
>
> I keep each of the topics bookmarked like this,
> https://lore.kernel.org/git/xmqqecjdea13.fsf@gitster.g/, and then utilize
> that like to send the email
>
> ```
> git send-email \
> --in-reply-to=xmqqecjdea13.fsf@gitster.g \
> --to=gitster@pobox.com \
> --cc=git@vger.kernel.org \
> --cc=gitgitgadget@gmail.com \
> --cc=haraldnordgren@gmail.com \
> /path/to/YOUR_REPLY
> ```
>
> I tried playing with neomutt and and email client replacement, but that
> adds the complexity of downloading a new mbox file for each reply, it
> didn't seem easier, but maybe it is.
>
> How do you handle emails?
I use the Fastmail webmail client for
regular non-patch emails. The only
things it messes up so far is long lines
in replies to patches.
I edit the emails in a text editor. And sometimes
I have left multiple drafts before sending them
and switched them around. Only to see my mistake on the Lore archive later. :)
But by and large it works just fine. I haven't had
the need for a more ergonomic setup.
--
Sent from mobile |
|
User |
c69a467 to
40d9eb3
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2302.v2.git.git.1778935976330.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > And I think git_config_parse_key() is at a way too low level to tell
> in what context we are seeing this faulty key to guess end-user's
> intention to limit our "did you mean?"
>
> I also wonder if, given that "=" in anywhere other than three-level
> names, is invalid, we should just start accept
>
> git config foo.bar=baz
> git config set foo.bar=baz
>
> and interpret them as
>
> git config set foo.bar baz
I tried implementing a version to be more liberal in what to accept, but
the implementation became very complex.
Moving in the other direction: show the warning, but try to make it more
correct.
(Also switching over to replying to emails with Gmail with 'plain text
mode'), hopefully there will be less miss-sends that end up on the wrong
topic from now on.)
Harald |
|
This patch series was integrated into seen via e834c48. |
|
This branch is now known as |
|
This patch series was integrated into seen via f37a7e6. |
|
This patch series was integrated into seen via 8b3cb0f. |
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Comments? source: <pull.2302.v2.git.git.1778935976330.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 121864a. |
|
This patch series was integrated into seen via f91f3e5. |
|
This patch series was integrated into seen via 363d16e. |
|
Submitted as pull.2302.v4.git.git.1779823288005.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): I forgot to update the PR description on GitHub, it should have read:
- Diagnose the 1-arg set form (explicit and implicit) directly: report
the missing value, and suggest the split form only when the prefix
before `=` is a valid key.
- Did not act on Junio's secondary suggestion to reword the 2-arg
`error: invalid key: <key>`, fix seemed to become too big.
Harald
On Tue, May 26, 2026 at 9:21 PM Harald Nordgren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> "git config set pull.rebase=false" currently fails with "wrong
> number of arguments", and the implicit form "git config
> pull.rebase=false" fails with "invalid key". Neither points at
> the real problem: the value is missing.
>
> Report that directly, and when the argument has the shape
> "<valid-key>=<value>", also suggest the split form:
>
> $ git config set pull.rebase=false
> error: missing value to set to the variable 'pull.rebase=false'
> hint: did you mean "git config set pull.rebase false"?
>
> When the prefix before "=" is not a valid key, drop the hint:
>
> $ git config set foo=bar
> error: missing value to set to a variable with an invalid name 'foo=bar'
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> config: suggest the correct form when key contains "="
>
> * Skip the hint when the inferred value contains whitespace, so git
> config set pull.rebase=false "hello world" no longer suggests a
> malformed command.
> * Replace the inline actions == 0 check with a named actions_implicit
> flag, simplfied the code.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2302%2FHaraldNordgren%2Fconfig-hint-equals-key-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v4
> Pull-Request: https://github.com/git/git/pull/2302
>
> Range-diff vs v3:
>
> 1: 6b9d66361d ! 1: 780b99409c config: suggest the correct form when key contains "=" in set context
> @@ Metadata
> Author: Harald Nordgren <haraldnordgren@gmail.com>
>
> ## Commit message ##
> - config: suggest the correct form when key contains "=" in set context
> + config: improve diagnostic for "set" with missing value
>
> - A user who types "git config pull.rebase=false" gets only "error:
> - invalid key: pull.rebase=false" with no clue what went wrong.
> + "git config set pull.rebase=false" currently fails with "wrong
> + number of arguments", and the implicit form "git config
> + pull.rebase=false" fails with "invalid key". Neither points at
> + the real problem: the value is missing.
>
> - Emit a "did you mean ..." hint suggesting the split form. Restrict it
> - to plausible-set contexts ("git config set", bare "git config <key>",
> - and their 2-arg forms); explicit "get"/"unset" keep the existing error.
> + Report that directly, and when the argument has the shape
> + "<valid-key>=<value>", also suggest the split form:
>
> - "=" is legal inside a subsection, so only fire when "=" lands after
> - the last ".". When the user supplied a separate value, use it in the
> - suggestion instead of the suffix after "=":
> + $ git config set pull.rebase=false
> + error: missing value to set to the variable 'pull.rebase=false'
> + hint: did you mean "git config set pull.rebase false"?
>
> - $ git config set pull.rebase=false true
> - error: invalid key: pull.rebase=false
> - hint: did you mean "git config set pull.rebase true"?
> + When the prefix before "=" is not a valid key, drop the hint:
> +
> + $ git config set foo=bar
> + error: missing value to set to a variable with an invalid name 'foo=bar'
>
> - Signed-off-by: Harald Nordgren <harald.nordgren@kostdoktorn.se>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>
> ## builtin/config.c ##
> @@ builtin/config.c: static void check_argc(int argc, int min, int max)
> exit(129);
> }
>
> -+static void advise_setting_with_equals(const char *key, const char *value)
> ++static int is_valid_key(const char *key)
> +{
> + const char *last_dot = strrchr(key, '.');
> -+ const char *eq;
> +
> -+ if (!last_dot)
> -+ return;
> -+ eq = strchr(last_dot + 1, '=');
> -+ if (!eq)
> -+ return;
> -+ if (!value)
> -+ value = eq + 1;
> -+ if (!*value || strpbrk(value, " \t\n"))
> -+ return;
> -+ advise(_("did you mean \"git config set %.*s %s\"?"),
> -+ (int)(eq - key), key, value);
> ++ return last_dot && isalpha(last_dot[1]);
> ++}
> ++
> ++static NORETURN void die_missing_set_value(const char *arg)
> ++{
> ++ const char *last_dot = strrchr(arg, '.');
> ++ const char *eq = last_dot ? strchr(last_dot + 1, '=') : NULL;
> ++ char *prefix = eq ? xstrndup(arg, eq - arg) : NULL;
> ++
> ++ if (prefix && is_valid_key(prefix)) {
> ++ error(_("missing value to set to the variable '%s'"), arg);
> ++ advise(_("did you mean \"git config set %s %s\"?"),
> ++ prefix, eq + 1);
> ++ } else if (is_valid_key(arg)) {
> ++ error(_("missing value to set to the variable '%s'"), arg);
> ++ } else {
> ++ error(_("missing value to set to a variable with an invalid name '%s'"),
> ++ arg);
> ++ }
> ++ free(prefix);
> ++ exit(129);
> +}
> +
> static void show_config_origin(const struct config_display_options *opts,
> @@ builtin/config.c: static int cmd_config_set(int argc, const char **argv, const c
>
> argc = parse_options(argc, argv, prefix, opts, builtin_config_set_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);
> -+ if (argc == 1 && strchr(argv[0], '=')) {
> -+ error(_("wrong number of arguments, should be 2"));
> -+ advise_setting_with_equals(argv[0], NULL);
> -+ exit(129);
> -+ }
> ++ if (argc == 1)
> ++ die_missing_set_value(argv[0]);
> check_argc(argc, 2, 2);
>
> if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
> -@@ builtin/config.c: static int cmd_config_set(int argc, const char **argv, const char *prefix,
> - error(_("cannot overwrite multiple values with a single value\n"
> - " Use --value=<pattern>, --append or --all to change %s."), argv[0]);
> - }
> -+ if (ret == CONFIG_INVALID_KEY)
> -+ advise_setting_with_equals(argv[0], argv[1]);
> -
> - location_options_release(&location_opts);
> - free(comment);
> @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> };
> char *value = NULL, *comment = NULL;
> @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, con
> case 1: actions = ACTION_GET; break;
> case 2: actions = ACTION_SET; break;
> @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> - if (ret == CONFIG_NOTHING_SET)
> - error(_("cannot overwrite multiple values with a single value\n"
> - " Use a regexp, --add or --replace-all to change %s."), argv[0]);
> -+ else if (ret == CONFIG_INVALID_KEY)
> -+ advise_setting_with_equals(argv[0], argv[1]);
> - }
> - else if (actions == ACTION_SET_ALL) {
> - check_write(&location_opts.source);
> -@@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> - check_argc(argc, 1, 2);
> - ret = get_value(&location_opts, &display_opts, argv[0], argv[1],
> - 0, flags);
> -+ if (ret == CONFIG_INVALID_KEY && actions_implicit)
> -+ advise_setting_with_equals(argv[0], NULL);
> - }
> - else if (actions == ACTION_GET_ALL) {
> - check_argc(argc, 1, 2);
> + error(_("no action specified"));
> + exit(129);
> + }
> ++ if (actions_implicit && argc == 1) {
> ++ const char *last_dot = strrchr(argv[0], '.');
> ++ if (last_dot && strchr(last_dot + 1, '='))
> ++ die_missing_set_value(argv[0]);
> ++ }
> + if (display_opts.omit_values &&
> + !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {
> + error(_("--name-only is only applicable to --list or --get-regexp"));
>
> ## t/t1300-config.sh ##
> @@ t/t1300-config.sh: test_expect_success 'invalid key' '
> test_must_fail git config inval.2key blabla
> '
>
> -+test_expect_success 'misplaced "=" in key: bare 1-arg form hints' '
> -+ test_must_fail git config pull.rebase=false 2>err &&
> -+ test_grep "invalid key: pull\\.rebase=false" err &&
> ++test_expect_success 'set with 1 arg of "key=value": valid key suggests split form' '
> ++ test_must_fail git config set pull.rebase=false 2>err &&
> ++ test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
> +
> -+test_expect_success 'misplaced "=" in key: bare 2-arg form uses given value' '
> -+ test_must_fail git config pull.rebase=false true 2>err &&
> -+ test_grep "did you mean .git config set pull\\.rebase true." err
> -+'
> -+
> -+test_expect_success 'misplaced "=" in key: set subcommand uses given value' '
> -+ test_must_fail git config set pull.rebase=false true 2>err &&
> -+ test_grep "did you mean .git config set pull\\.rebase true." err
> -+'
> -+
> -+test_expect_success 'misplaced "=" in key: set with single arg hints' '
> -+ test_must_fail git config set pull.rebase=false 2>err &&
> -+ test_grep "wrong number of arguments" err &&
> ++test_expect_success 'set with 1 arg of "key=value": implicit form suggests split form' '
> ++ test_must_fail git config pull.rebase=false 2>err &&
> ++ test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
> +
> -+test_expect_success 'misplaced "=" in key: explicit --get does not hint' '
> -+ test_must_fail git config --get pull.rebase=false 2>err &&
> -+ test_grep "invalid key: pull\\.rebase=false" err &&
> ++test_expect_success 'set with 1 arg of "key=value": invalid key does not suggest split form' '
> ++ test_must_fail git config set foo=bar 2>err &&
> ++ test_grep "missing value to set to a variable with an invalid name .foo=bar." err &&
> + test_grep ! "did you mean" err
> +'
> +
> -+test_expect_success 'misplaced "=" in key: get subcommand does not hint' '
> -+ test_must_fail git config get pull.rebase=false 2>err &&
> ++test_expect_success 'set with 1 arg: variable name starting with digit is invalid' '
> ++ test_must_fail git config set foo.1bar=baz 2>err &&
> ++ test_grep "missing value to set to a variable with an invalid name .foo\\.1bar=baz." err &&
> + test_grep ! "did you mean" err
> +'
> +
> -+test_expect_success 'misplaced "=" in key: unset subcommand does not hint' '
> -+ test_must_fail git config unset pull.rebase=false 2>err &&
> ++test_expect_success 'set with 1 arg of valid key reports missing value' '
> ++ test_must_fail git config set pull.rebase 2>err &&
> ++ test_grep "missing value to set to the variable .pull\\.rebase." err &&
> + test_grep ! "did you mean" err
> +'
> +
> -+test_expect_success 'misplaced "=" in key: value with whitespace skips hint' '
> -+ test_must_fail git config set pull.rebase=false "hello world" 2>err &&
> -+ test_grep "invalid key: pull\\.rebase=false" err &&
> ++test_expect_success 'set with 2 args including "=" in invalid key does not suggest' '
> ++ test_must_fail git config set pull.rebase=false true 2>err &&
> + test_grep ! "did you mean" err
> +'
> +
> -+test_expect_success '"=" inside subsection is valid, no hint' '
> ++test_expect_success '"=" inside subsection is valid' '
> + test_when_finished "rm -f subsection.cfg" &&
> -+ git config set -f subsection.cfg foo.bar=baz.boo qux 2>err &&
> -+ test_grep ! "did you mean" err &&
> ++ git config set -f subsection.cfg foo.bar=baz.boo qux &&
> + echo qux >expect &&
> + git config get -f subsection.cfg foo.bar=baz.boo >actual &&
> + test_cmp expect actual
>
>
> builtin/config.c | 39 ++++++++++++++++++++++++++++++++++++++-
> t/t1300-config.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index cf4ba0f7cc..6fe2d85814 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -1,6 +1,7 @@
> #define USE_THE_REPOSITORY_VARIABLE
> #include "builtin.h"
> #include "abspath.h"
> +#include "advice.h"
> #include "config.h"
> #include "color.h"
> #include "date.h"
> @@ -210,6 +211,33 @@ static void check_argc(int argc, int min, int max)
> exit(129);
> }
>
> +static int is_valid_key(const char *key)
> +{
> + const char *last_dot = strrchr(key, '.');
> +
> + return last_dot && isalpha(last_dot[1]);
> +}
> +
> +static NORETURN void die_missing_set_value(const char *arg)
> +{
> + const char *last_dot = strrchr(arg, '.');
> + const char *eq = last_dot ? strchr(last_dot + 1, '=') : NULL;
> + char *prefix = eq ? xstrndup(arg, eq - arg) : NULL;
> +
> + if (prefix && is_valid_key(prefix)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + advise(_("did you mean \"git config set %s %s\"?"),
> + prefix, eq + 1);
> + } else if (is_valid_key(arg)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + } else {
> + error(_("missing value to set to a variable with an invalid name '%s'"),
> + arg);
> + }
> + free(prefix);
> + exit(129);
> +}
> +
> static void show_config_origin(const struct config_display_options *opts,
> const struct key_value_info *kvi,
> struct strbuf *buf)
> @@ -1133,6 +1161,8 @@ static int cmd_config_set(int argc, const char **argv, const char *prefix,
>
> argc = parse_options(argc, argv, prefix, opts, builtin_config_set_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);
> + if (argc == 1)
> + die_missing_set_value(argv[0]);
> check_argc(argc, 2, 2);
>
> if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
> @@ -1371,6 +1401,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> };
> char *value = NULL, *comment = NULL;
> int ret = 0;
> + int actions_implicit;
> struct key_value_info default_kvi = KVI_INIT;
>
> argc = parse_options(argc, argv, prefix, opts,
> @@ -1385,7 +1416,8 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> exit(129);
> }
>
> - if (actions == 0)
> + actions_implicit = (actions == 0);
> + if (actions_implicit)
> switch (argc) {
> case 1: actions = ACTION_GET; break;
> case 2: actions = ACTION_SET; break;
> @@ -1394,6 +1426,11 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
> error(_("no action specified"));
> exit(129);
> }
> + if (actions_implicit && argc == 1) {
> + const char *last_dot = strrchr(argv[0], '.');
> + if (last_dot && strchr(last_dot + 1, '='))
> + die_missing_set_value(argv[0]);
> + }
> if (display_opts.omit_values &&
> !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {
> error(_("--name-only is only applicable to --list or --get-regexp"));
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 11fc976f3a..4a8a381bd8 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -469,6 +469,49 @@ test_expect_success 'invalid key' '
> test_must_fail git config inval.2key blabla
> '
>
> +test_expect_success 'set with 1 arg of "key=value": valid key suggests split form' '
> + test_must_fail git config set pull.rebase=false 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
> +
> +test_expect_success 'set with 1 arg of "key=value": implicit form suggests split form' '
> + test_must_fail git config pull.rebase=false 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
> +
> +test_expect_success 'set with 1 arg of "key=value": invalid key does not suggest split form' '
> + test_must_fail git config set foo=bar 2>err &&
> + test_grep "missing value to set to a variable with an invalid name .foo=bar." err &&
> + test_grep ! "did you mean" err
> +'
> +
> +test_expect_success 'set with 1 arg: variable name starting with digit is invalid' '
> + test_must_fail git config set foo.1bar=baz 2>err &&
> + test_grep "missing value to set to a variable with an invalid name .foo\\.1bar=baz." err &&
> + test_grep ! "did you mean" err
> +'
> +
> +test_expect_success 'set with 1 arg of valid key reports missing value' '
> + test_must_fail git config set pull.rebase 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase." err &&
> + test_grep ! "did you mean" err
> +'
> +
> +test_expect_success 'set with 2 args including "=" in invalid key does not suggest' '
> + test_must_fail git config set pull.rebase=false true 2>err &&
> + test_grep ! "did you mean" err
> +'
> +
> +test_expect_success '"=" inside subsection is valid' '
> + test_when_finished "rm -f subsection.cfg" &&
> + git config set -f subsection.cfg foo.bar=baz.boo qux &&
> + echo qux >expect &&
> + git config get -f subsection.cfg foo.bar=baz.boo >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'correct key' '
> git config 123456.a123 987
> '
>
> base-commit: 56a4f3c3a221adf1df9b39da69b8a6890f803157
> --
> gitgitgadget |
780b994 to
85e266a
Compare
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Comments? source: <pull.2302.v3.git.git.1779697995418.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 581042d. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int is_valid_key(const char *key)
> +{
> + const char *last_dot = strrchr(key, '.');
> +
> + return last_dot && isalpha(last_dot[1]);
> +}
None of these are valid configuration variable names, but this
function would allow any of them, no?
1foo.bar
1foo.some.bar
foo.b_r
foo.some.b_r
or does the caller reject such "key" before calling us?
> +static NORETURN void die_missing_set_value(const char *arg)
> +{
> + const char *last_dot = strrchr(arg, '.');
> + const char *eq = last_dot ? strchr(last_dot + 1, '=') : NULL;
OK, the intention is to see "foo.bar=baz" and guess that assinging
to "foo.bar" might be what the user wanted. eq here would point at
that '='. And ...
> + char *prefix = eq ? xstrndup(arg, eq - arg) : NULL;
... prefix is our own copy of "foo.bar".
> + if (prefix && is_valid_key(prefix)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + advise(_("did you mean \"git config set %s %s\"?"),
> + prefix, eq + 1);
OK. If is_valid_key() rejected invalid variable names correctly,
this would catch $A=$B where $A is a plausible-looking name.
> + } else if (is_valid_key(arg)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + } else {
> + error(_("missing value to set to a variable with an invalid name '%s'"),
> + arg);
> + }
The distinction among these three messages does look reasonable,
provided if is_valid_key() gives the correct result.
I wonder if it is too hard to refactor existing logic (perhaps it is
used in git_config_parse_key(), no?) to give us a less noisy version
of it that we can use as is_valid_key() here?
Other than that, the remainder of the code changes looked reasonable
to me. Thanks. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int is_valid_key(const char *key)
> +{
> + const char *last_dot = strrchr(key, '.');
> +
> + return last_dot && isalpha(last_dot[1]);
> +}
None of these are valid configuration variable names, but this
function would allow any of them, no?
1foo.bar
1foo.some.bar
foo.b_r
foo.some.b_r
or does the caller reject such "key" before calling us?
> +static NORETURN void die_missing_set_value(const char *arg)
> +{
> + const char *last_dot = strrchr(arg, '.');
> + const char *eq = last_dot ? strchr(last_dot + 1, '=') : NULL;
OK, the intention is to see "foo.bar=baz" and guess that assinging
to "foo.bar" might be what the user wanted. eq here would point at
that '='. And ...
> + char *prefix = eq ? xstrndup(arg, eq - arg) : NULL;
... prefix is our own copy of "foo.bar".
> + if (prefix && is_valid_key(prefix)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + advise(_("did you mean \"git config set %s %s\"?"),
> + prefix, eq + 1);
OK. If is_valid_key() rejected invalid variable names correctly,
this would catch $A=$B where $A is a plausible-looking name.
> + } else if (is_valid_key(arg)) {
> + error(_("missing value to set to the variable '%s'"), arg);
> + } else {
> + error(_("missing value to set to a variable with an invalid name '%s'"),
> + arg);
> + }
The distinction among these three messages does look reasonable,
provided if is_valid_key() gives the correct result.
I wonder if it is too hard to refactor existing logic (perhaps it is
used in git_config_parse_key(), no?) to give us a less noisy version
of it that we can use as is_valid_key() here?
Other than that, the remainder of the code changes looked reasonable
to me. Thanks. |
|
This patch series was integrated into seen via 3c85eab. |
|
This patch series was integrated into seen via 645f572. |
dc8a011 to
e5a2070
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2302.v5.git.git.1780407557.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Waiting for response(s) to review comment(s). cf. <xmqq33z524yh.fsf@gitster.g> source: <pull.2302.v4.git.git.1779823288005.gitgitgadget@gmail.com> |
| error(_("invalid key pattern: %s"), key_); | ||
| FREE_AND_NULL(data.key_regexp); | ||
| ret = CONFIG_INVALID_PATTERN; | ||
| goto free_strings; |
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Add a "quiet" parameter that suppresses the error() calls, and let
> store_key be NULL to skip the canonical-copy allocation. Existing
> callers pass 0 for quiet.
Hmph.
The way this patch did this may have been easier to implement, but
is a bit different from what I had in mind when I suggested to
"refactor" the existing logic.
Perhaps the updated "git_config_parse_key()" in this patch should be
renamed to be a file-scape static internal helper, and the existing
"git_config_parse_key()" should become a thin wrapper around that
new helper function, retaining the current external interface,
requiring no changes to existing callers.
Then in the next step, config.[ch] can add a new entry point that
serves the purpose of is_valid_key() in the previous iteration,
perhaps call it is_valid_git_config_key() or something like that
(Patrick or others may want to suggest a better word order in its
name). That way, we do not have to sprinkle many calls into
this (rather ugly) version of git_config_parse_key() with overly
wide interface that repeats meaningless NULL/0/1 parameters that no
callers want to use (other than for the purpose of differenciating
the real git_config_parse_key() calls from the new calls made to the
same function to ask "is this a valid key or not, yes/no?".
Thanks.There was a problem hiding this comment.
Harald Nordgren wrote on the Git mailing list (how to reply to this email):
> Perhaps the updated "git_config_parse_key()" in this patch should be
> renamed to be a file-scape static internal helper, and the existing
> "git_config_parse_key()" should become a thin wrapper around that
> new helper function, retaining the current external interface,
> requiring no changes to existing callers.
I want to remember a discussion on one of my earlier topics, a few
months back, where someone else suggested instead of introducing two
thin wrappers over a helper, we should update the callers instead.
But for me either way is fine, maybe here it makes more sense, because
of the repeated NULL/0/1 parameters.
HaraldThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Harald Nordgren <haraldnordgren@gmail.com> writes:
>> Perhaps the updated "git_config_parse_key()" in this patch should be
>> renamed to be a file-scape static internal helper, and the existing
>> "git_config_parse_key()" should become a thin wrapper around that
>> new helper function, retaining the current external interface,
>> requiring no changes to existing callers.
>
> I want to remember a discussion on one of my earlier topics, a few
> months back, where someone else suggested instead of introducing two
> thin wrappers over a helper, we should update the callers instead.
>
> But for me either way is fine, maybe here it makes more sense, because
> of the repeated NULL/0/1 parameters.
If the "quiet" and "store_key" setting were independent, then I
wouldn't have made such a suggestion. But I got an impression that
with the updated code, there wasn't a valid use case to ask to
quietly store the discovered key.
An ideal refactoring would have been a low level helper function
that only yields error code, and git_config_parse_key() would call
it and react to the returned error code, stores the discovered key,
and produces error message on its own. Then such an "always quiet"
helper can be used for the purpose of the new caller, without having
to have "if (!quiet)" sprinkled all over. But that is certainly
cumbersome to arrange.
| @@ -1,6 +1,7 @@ | |||
| #define USE_THE_REPOSITORY_VARIABLE | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 11fc976f3a..ed122d1100 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -469,6 +469,61 @@ test_expect_success 'invalid key' '
> test_must_fail git config inval.2key blabla
> '
>
> +test_expect_success 'set with 1 arg of "key=value": valid key suggests split form' '
> + test_must_fail git config set pull.rebase=false 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
This is a syntax error of the command line, but the lhs of '=' makes
us suspect that the user may have meant to assign to that variable.
Makes perfect sense.
> +test_expect_success 'set with 1 arg of "key=value": implicit form suggests split form' '
> + test_must_fail git config pull.rebase=false 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase=false." err &&
> + test_grep "did you mean .git config set pull\\.rebase false." err
> +'
Ditto, the syntax may be an implicit "get" with bogus variable name,
or an implicit "set" with variable name and its value concatenated
into one argument with '='. The message seems to be assuming the
latter, which is OK to me.
> +test_expect_success 'set with 1 arg of "key=value": invalid key does not suggest split form' '
> + test_must_fail git config set foo=bar 2>err &&
> + test_grep "missing value to set to a variable with an invalid name .foo=bar." err &&
> + test_grep ! "did you mean" err
> +'
OK.
> +test_expect_success 'set with 1 arg: variable name starting with digit is invalid' '
> + test_must_fail git config set foo.1bar=baz 2>err &&
> + test_grep "missing value to set to a variable with an invalid name .foo\\.1bar=baz." err &&
> + test_grep ! "did you mean" err
> +'
OK. The above two should always give us the same error (except for
the actual bogus names given by the user to the command).
> +test_expect_success 'set with 1 arg: digit-led section name is valid' '
> + test_must_fail git config set 1foo.bar=baz 2>err &&
> + test_grep "missing value to set to the variable .1foo\\.bar=baz." err &&
> + test_grep "did you mean .git config set 1foo\\.bar baz." err
> +'
OK.
> +test_expect_success 'set with 1 arg: subsection plus invalid variable name' '
> + test_must_fail git config set foo.some.b_r=baz 2>err &&
> + test_grep "missing value to set to a variable with an invalid name .foo\\.some\\.b_r=baz." err &&
> + test_grep ! "did you mean" err
> +'
This is the third one that should be identical to earlier two that
gave a bogus variable name.
> +test_expect_success 'set with 1 arg of valid key reports missing value' '
> + test_must_fail git config set pull.rebase 2>err &&
> + test_grep "missing value to set to the variable .pull\\.rebase." err &&
> + test_grep ! "did you mean" err
> +'
Did we see this already? No, this is different from the earlier one
that had "=false". This is a bog standard "you said set but did not
say what value to set to". Good.
> +test_expect_success 'set with 2 args including "=" in invalid key does not suggest' '
> + test_must_fail git config set pull.rebase=false true 2>err &&
> + test_grep ! "did you mean" err
> +'
OK. Do we want to see that the bogus variable name reported?
> +test_expect_success '"=" inside subsection is valid' '
> + test_when_finished "rm -f subsection.cfg" &&
> + git config set -f subsection.cfg foo.bar=baz.boo qux &&
> + echo qux >expect &&
> + git config get -f subsection.cfg foo.bar=baz.boo >actual &&
> + test_cmp expect actual
> +'
Excellent.Move the body of git_config_parse_key() into a static helper do_parse_config_key() that takes a "quiet" flag and treats store_key as optional. git_config_parse_key() becomes a thin wrapper. Add git_config_key_is_valid() for callers that only need to know whether a key is well-formed. Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
e5a2070 to
fa2990b
Compare
"git config set pull.rebase=false" currently fails with "wrong
number of arguments", and the implicit form "git config
pull.rebase=false" fails with "invalid key". Neither points at
the real problem: the value is missing.
Report that directly, and when the argument has the shape
"<valid-key>=<value>", also suggest the split form:
$ git config set pull.rebase=false
error: missing value to set to the variable 'pull.rebase=false'
hint: did you mean "git config set pull.rebase false"?
When the prefix before "=" is not a valid key, drop the hint:
$ git config set foo=bar
error: missing value to set to a variable with an invalid name 'foo=bar'
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
fa2990b to
a7f8a08
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2302.v6.git.git.1780425808.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via 3c6778d. |
|
This patch series was integrated into seen via b4e7f40. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * The quiet parameter now lives on a static do_parse_config_key() instead
> of git_config_parse_key() itself. git_config_parse_key() is back to its
> three-argument signature; existing callers don't change.
> * New public git_config_key_is_valid() for callers that only need a yes/no
> check.
>
> Harald Nordgren (2):
> config: add git_config_key_is_valid() for quiet validation
> config: improve diagnostic for "set" with missing value
>
> builtin/config.c | 32 ++++++++++++++++++++++++++-
> config.c | 38 ++++++++++++++++++++++++--------
> config.h | 2 ++
> t/t1300-config.sh | 56 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 118 insertions(+), 10 deletions(-)
Looking good. Thanks. Will queue. |
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Will merge to 'next'? source: <pull.2302.v6.git.git.1780425808.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Will merge to 'next'. cf. <xmqq1penqfg2.fsf@gitster.g> source: <pull.2302.v6.git.git.1780425808.gitgitgadget@gmail.com> |
|
This patch series was integrated into next via 5149e69. |
|
There was a status update in the "Cooking" section about the branch "git config foo.bar=baz" is not likely to be a request to read the value of such a variable with '=' in its name; rather it is plausible that the user meant "git config set foo.bar baz". Give advice when giving an error message. Will merge to 'master'. cf. <xmqq1penqfg2.fsf@gitster.g> source: <pull.2302.v6.git.git.1780425808.gitgitgadget@gmail.com> |
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
1 participant
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.
quietparameter now lives on a staticdo_parse_config_key()instead ofgit_config_parse_key()itself.git_config_parse_key()is back to its three-argument signature; existing callers don't change.git_config_key_is_valid()for callers that only need a yes/no check.cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com