[PATCH] config: suggest the correct form when key contains "="

[PATCH] config: suggest the correct form when key contains "="

[Harald Nordgren via GitGitGadget]

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

The hint is emitted only when the offending character is "="; other
invalid characters (newlines, "@", etc.) keep their existing error
unchanged.

Signed-off-by: Harald Nordgren <harald.nordgren@kostdoktorn.se>
---
    config: suggest the correct form when key contains "="

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2302%2FHaraldNordgren%2Fconfig-hint-equals-key-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v1
Pull-Request: https://github.com/git/git/pull/2302

 config.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/config.c b/config.c
index a1b92fe083..6e658d71d1 100644
--- a/config.c
+++ b/config.c
@@ -580,6 +580,10 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 			if (!iskeychar(c) ||
 			    (i == baselen + 1 && !isalpha(c))) {
 				error(_("invalid key: %s"), key);
+				if (c == '=')
+					fprintf_ln(stderr,
+						   _("  (did you mean \"git config set %.*s %s\"?)"),
+						   (int)i, key, key + i + 1);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
-- 
gitgitgadget

Re: [PATCH] config: suggest the correct form when key contains "="

[Junio C Hamano]

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

[PATCH] fetch: add fetch.pruneLocalBranches config

[Harald Nordgren]

> 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

Re: [PATCH] fetch: add fetch.pruneLocalBranches config

[Junio C Hamano]

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?

Email issues

[Harald Nordgren]

> 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

Re: [PATCH] fetch: add fetch.pruneLocalBranches config

[Harald Nordgren]

> 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

Re: Email issues

[Kristoffer Haugsbakk]

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

Re: [PATCH] config: suggest the correct form when key contains "="

[Harald Nordgren]

> 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

[PATCH v2] config: suggest the correct form when key contains "=" in set context

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A user who types "git config pull.rebase=false" gets only "error:
invalid key: pull.rebase=false" with no clue what went wrong.

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.

"=" 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 true
    error: invalid key: pull.rebase=false
    hint: did you mean "git config set pull.rebase true"?

Signed-off-by: Harald Nordgren <harald.nordgren@kostdoktorn.se>
---
    config: suggest the correct form when key contains "="
    
     * Hint moved from git_config_parse_key() to a new
       advise_setting_with_equals() in builtin/config.c; wired only into set
       and bare paths.
     * Only fires when = is after the last .; 2-arg forms use the user's
       value.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2302%2FHaraldNordgren%2Fconfig-hint-equals-key-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v2
Pull-Request: https://github.com/git/git/pull/2302

Range-diff vs v1:

 1:  56eb3ce6fd < -:  ---------- config: suggest the correct form when key contains "="
 -:  ---------- > 1:  40d9eb3e5c config: suggest the correct form when key contains "=" in set context


 builtin/config.c  | 30 ++++++++++++++++++++++++++++++
 t/t1300-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/builtin/config.c b/builtin/config.c
index cf4ba0f7cc..f14a30e720 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,22 @@ static void check_argc(int argc, int min, int max)
 	exit(129);
 }
 
+static void advise_setting_with_equals(const char *key, const char *value)
+{
+	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;
+	advise(_("did you mean \"git config set %.*s %s\"?"),
+	       (int)(eq - key), key, value);
+}
+
 static void show_config_origin(const struct config_display_options *opts,
 			       const struct key_value_info *kvi,
 			       struct strbuf *buf)
@@ -1133,6 +1150,11 @@ 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 && strchr(argv[0], '=')) {
+		error(_("wrong number of arguments, should be 2"));
+		advise_setting_with_equals(argv[0], NULL);
+		exit(129);
+	}
 	check_argc(argc, 2, 2);
 
 	if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
@@ -1160,6 +1182,8 @@ 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);
@@ -1371,6 +1395,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,6 +1410,7 @@ static int cmd_config_actions(int argc, const char **argv, const char *prefix)
 		exit(129);
 	}
 
+	actions_implicit = (actions == 0);
 	if (actions == 0)
 		switch (argc) {
 		case 1: actions = ACTION_GET; break;
@@ -1485,6 +1511,8 @@ 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);
@@ -1515,6 +1543,8 @@ 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);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 128971ee12..f46c081413 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -462,6 +462,53 @@ 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_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_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_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_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_grep ! "did you mean" err
+'
+
+test_expect_success '"=" inside subsection is valid, no hint' '
+	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 &&
+	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: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
-- 
gitgitgadget

[PATCH v3] config: suggest the correct form when key contains "=" in set context

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A user who types "git config pull.rebase=false" gets only "error:
invalid key: pull.rebase=false" with no clue what went wrong.

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.

"=" 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 true
    error: invalid key: pull.rebase=false
    hint: did you mean "git config set pull.rebase true"?

Signed-off-by: Harald Nordgren <harald.nordgren@kostdoktorn.se>
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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v3
Pull-Request: https://github.com/git/git/pull/2302

Range-diff vs v2:

 1:  40d9eb3e5c ! 1:  6b9d66361d config: suggest the correct form when key contains "=" in set context
     @@ Commit message
              hint: did you mean "git config set pull.rebase true"?
      
          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)
      +		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);
      +}
     @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, con
       		exit(129);
       	}
       
     +-	if (actions == 0)
      +	actions_implicit = (actions == 0);
     - 	if (actions == 0)
     ++	if (actions_implicit)
       		switch (argc) {
       		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"
     @@ t/t1300-config.sh: test_expect_success 'invalid key' '
      +	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_grep ! "did you mean" err
     ++'
     ++
      +test_expect_success '"=" inside subsection is valid, no hint' '
      +	test_when_finished "rm -f subsection.cfg" &&
      +	git config set -f subsection.cfg foo.bar=baz.boo qux 2>err &&


 builtin/config.c  | 34 +++++++++++++++++++++++++++++-
 t/t1300-config.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index cf4ba0f7cc..8c7ab36fcb 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,24 @@ static void check_argc(int argc, int min, int max)
 	exit(129);
 }
 
+static void advise_setting_with_equals(const char *key, const char *value)
+{
+	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);
+}
+
 static void show_config_origin(const struct config_display_options *opts,
 			       const struct key_value_info *kvi,
 			       struct strbuf *buf)
@@ -1133,6 +1152,11 @@ 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 && strchr(argv[0], '=')) {
+		error(_("wrong number of arguments, should be 2"));
+		advise_setting_with_equals(argv[0], NULL);
+		exit(129);
+	}
 	check_argc(argc, 2, 2);
 
 	if ((flags & CONFIG_FLAGS_FIXED_VALUE) && !value_pattern)
@@ -1160,6 +1184,8 @@ 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);
@@ -1371,6 +1397,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 +1412,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;
@@ -1485,6 +1513,8 @@ 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);
@@ -1515,6 +1545,8 @@ 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);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 11fc976f3a..4e12b78536 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -469,6 +469,59 @@ 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_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_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_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_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_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_grep ! "did you mean" err
+'
+
+test_expect_success '"=" inside subsection is valid, no hint' '
+	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 &&
+	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: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
-- 
gitgitgadget

Re: [PATCH v3] config: suggest the correct form when key contains "=" in set context

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I understand that it would be a good idea to give this warning
against these two where $A is an arbitrary string with at least one
dot in it (making it a likely variable name), and $B is an arbitrary
string that may contain anything:

    git config set "$A=$B"
    git config "$A=$B"

It is plausible that the user wanted to make the value of the
variable "$A" to "$B", so telling them the right syntax would be
valuable.

If "$A" is a syntactically valid variable name, then I would imagine
that we want to say something like this:

    $ git config set "$A=$B"
    error: missing value to set to the variable "$A=$B"
    hint: did you mean 'git config set "$A" "$B"'?

If "$A" is *not* a syntactically valid variable name, then giving a
hint to try to assing to it is a counter-productive.  Ideally we
probably want something like:

    $ git config set "foo=bar"
    error: missing value to set to a variable with an invalid name 'foo=bar'

It is pointless to say the user may have meant "git config set foo bar",
as "foo" is clearly not a valid variable.

I do not understand what you mean by "their 2-arg forms".  Do you
mean

    git config set "$A=$B" "$C"

by that?  If so, I doubt that user meant an assignment to "$A" by
this form with explicit "set".  If "$A=$B" is a variable whose name
is valid (i.e. three-level name whose the second level component
contains a "="), we should just take it as asked.  E.g.,

    git config set "foo.bar=baz.boo" "some-string"

needs no hand holding.  But
if "$A=$B" is not a valid variable name, we should just complain
that the user is trying to assign to a variable with an invalid
name.

    $ git config set "foo.bar=baz" "some-string"
    error: setting to a variable with invalid name 'foo.bar=baz'

I think

    git config "$A=$B" "$C"

that implicitly uses the 'set' verb can be left as an exercise to
readers.  If "$A=$B" is a valid name, we shouldn't do any complaint.
If it is not, 

    $ git config "foo.bar=baz" "some-string"
    error: setting to a variable with invalid name 'foo.bar=baz'

It makes it clear to the user that (1) we interpreted the command
line to be "implicit set", (2) we interpreted the command line to
set variable 'foo.bar=baz', and (3) 'foo.bar=baz' is not a valid
name.  I do not think there is anything more needed for this case.

> "=" 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 true
>     error: invalid key: pull.rebase=false
>     hint: did you mean "git config set pull.rebase true"?

I really do not think '=' needs *any* special casing in this case.
If we used "pull.rebase*false" as the variable instead, the message
would say that "pull.rebase*false" is an invalid key.  Two important
things for this message to convey are (1) the command correctly
parsed the command line to mean that the user wants to assign to a
variable whose name is 'pull.rebase*false' and (2) that variable
name *is* invalid.

If you find the current message suboptimal, I think we should try to
clarify the message, as '=' or '*' or any letter that makes the
variable name invalid would benefit from the same improvement.
Perhaps something like:

    $ git config set pull.rebase*false true
    error: setting to a variable with invalid name: 'pull.rebase*false'

perhaps?


> Signed-off-by: Harald Nordgren <harald.nordgren@kostdoktorn.se>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>

Interesting.  We typically do not do this.

[PATCH v4] config: improve diagnostic for "set" with missing value

[Harald Nordgren via GitGitGadget]

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

Re: [PATCH v4] config: improve diagnostic for "set" with missing value

[Harald Nordgren]

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

Re: [PATCH v4] config: improve diagnostic for "set" with missing value

[Junio C Hamano]

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

Re: [PATCH v4] config: improve diagnostic for "set" with missing value

[Junio C Hamano]

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

[PATCH v5 0/2] config: suggest the correct form when key contains "="

[Harald Nordgren via GitGitGadget]

 * New commit config: let git_config_parse_key() validate quietly adds a
   quiet parameter (and an optional store_key) so callers can validate
   without writing to stderr.
 * Validation in die_missing_set_value() now routes through
   git_config_parse_key(key, NULL, NULL, 1) instead of the previous local
   helper.
 * Added tests for 1foo.bar=baz and foo.some.b_r=baz.

Harald Nordgren (2):
  config: let git_config_parse_key() validate quietly
  config: improve diagnostic for "set" with missing value

 builtin/config.c   | 34 ++++++++++++++++++++++++++--
 config.c           | 34 ++++++++++++++++++----------
 config.h           |  2 +-
 submodule-config.c |  2 +-
 t/t1300-config.sh  | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 111 insertions(+), 16 deletions(-)


base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2302%2FHaraldNordgren%2Fconfig-hint-equals-key-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v5
Pull-Request: https://github.com/git/git/pull/2302

Range-diff vs v4:

 -:  ---------- > 1:  d938ebf95a config: let git_config_parse_key() validate quietly
 1:  780b99409c ! 2:  e5a2070ee1 config: improve diagnostic for "set" with missing value
     @@ builtin/config.c: 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)) {
     ++	if (prefix && !git_config_parse_key(prefix, NULL, NULL, 1)) {
      +		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)) {
     ++	} else if (!git_config_parse_key(arg, NULL, NULL, 1)) {
      +		error(_("missing value to set to the variable '%s'"), arg);
      +	} else {
      +		error(_("missing value to set to a variable with an invalid name '%s'"),
     @@ t/t1300-config.sh: test_expect_success 'invalid key' '
      +	test_grep ! "did you mean" err
      +'
      +
     ++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
     ++'
     ++
     ++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
     ++'
     ++
      +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 &&

-- 
gitgitgadget

[PATCH v5 1/2] config: let git_config_parse_key() validate quietly

[Harald Nordgren via GitGitGadget]

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.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/config.c   |  2 +-
 config.c           | 34 ++++++++++++++++++++++------------
 config.h           |  2 +-
 submodule-config.c |  2 +-
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index cf4ba0f7cc..b3188cd8d4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -555,7 +555,7 @@ static int get_value(const struct config_location_options *opts,
 			goto free_strings;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL)) {
+		if (git_config_parse_key(key_, &key, NULL, 0)) {
 			ret = CONFIG_INVALID_KEY;
 			goto free_strings;
 		}
diff --git a/config.c b/config.c
index a1b92fe083..81b31c5155 100644
--- a/config.c
+++ b/config.c
@@ -536,11 +536,14 @@ static inline int iskeychar(int c)
  * -2 if there is no section name in the key.
  *
  * store_key - pointer to char* which will hold a copy of the key with
- *             lowercase section and variable name
+ *             lowercase section and variable name, can be NULL to skip
+ *             allocation when only validation is needed
  * baselen - pointer to size_t which will hold the length of the
  *           section + subsection part, can be NULL
+ * quiet - when non-zero, suppress error() reports on rejection
  */
-int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
+int git_config_parse_key(const char *key, char **store_key, size_t *baselen_,
+			 int quiet)
 {
 	size_t i, baselen;
 	int dot;
@@ -552,12 +555,14 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 	 */
 
 	if (last_dot == NULL || last_dot == key) {
-		error(_("key does not contain a section: %s"), key);
+		if (!quiet)
+			error(_("key does not contain a section: %s"), key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
-		error(_("key does not contain variable name: %s"), key);
+		if (!quiet)
+			error(_("key does not contain variable name: %s"), key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
@@ -568,7 +573,8 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmallocz(strlen(key));
+	if (store_key)
+		*store_key = xmallocz(strlen(key));
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
@@ -579,21 +585,25 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 		if (!dot || i > baselen) {
 			if (!iskeychar(c) ||
 			    (i == baselen + 1 && !isalpha(c))) {
-				error(_("invalid key: %s"), key);
+				if (!quiet)
+					error(_("invalid key: %s"), key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error(_("invalid key (newline): %s"), key);
+			if (!quiet)
+				error(_("invalid key (newline): %s"), key);
 			goto out_free_ret_1;
 		}
-		(*store_key)[i] = c;
+		if (store_key)
+			(*store_key)[i] = c;
 	}
 
 	return 0;
 
 out_free_ret_1:
-	FREE_AND_NULL(*store_key);
+	if (store_key)
+		FREE_AND_NULL(*store_key);
 	return -CONFIG_INVALID_KEY;
 }
 
@@ -609,7 +619,7 @@ static int config_parse_pair(const char *key, const char *value,
 
 	if (!strlen(key))
 		return error(_("empty config key"));
-	if (git_config_parse_key(key, &canonical_name, NULL))
+	if (git_config_parse_key(key, &canonical_name, NULL, 0))
 		return -1;
 
 	ret = (fn(canonical_name, value, &ctx, data) < 0) ? -1 : 0;
@@ -1708,7 +1718,7 @@ static int configset_find_element(struct config_set *set, const char *key,
 	 * `key` may come from the user, so normalize it before using it
 	 * for querying entries from the hashmap.
 	 */
-	ret = git_config_parse_key(key, &normalized_key, NULL);
+	ret = git_config_parse_key(key, &normalized_key, NULL, 0);
 	if (ret)
 		return ret;
 
@@ -3001,7 +3011,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	validate_comment_string(comment);
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
-	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
+	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 0);
 	if (ret)
 		goto out_free;
 
diff --git a/config.h b/config.h
index bf47fb3afc..2c66d334c1 100644
--- a/config.h
+++ b/config.h
@@ -341,7 +341,7 @@ int repo_config_set_worktree_gently(struct repository *, const char *, const cha
  */
 void repo_config_set(struct repository *, const char *, const char *);
 
-int git_config_parse_key(const char *, char **, size_t *);
+int git_config_parse_key(const char *, char **, size_t *, int quiet);
 
 /*
  * The following macros specify flag bits that alter the behavior
diff --git a/submodule-config.c b/submodule-config.c
index a81897b4e0..a319956f7a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -970,7 +970,7 @@ int print_config_from_gitmodules(struct repository *repo, const char *key)
 	int ret;
 	char *store_key;
 
-	ret = git_config_parse_key(key, &store_key, NULL);
+	ret = git_config_parse_key(key, &store_key, NULL, 0);
 	if (ret < 0)
 		return CONFIG_INVALID_KEY;
 
-- 
gitgitgadget

[PATCH v5 2/2] config: improve diagnostic for "set" with missing value

[Harald Nordgren via GitGitGadget]

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>
---
 builtin/config.c  | 32 ++++++++++++++++++++++++++-
 t/t1300-config.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index b3188cd8d4..a2d46d0ce1 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,26 @@ static void check_argc(int argc, int min, int max)
 	exit(129);
 }
 
+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 && !git_config_parse_key(prefix, NULL, NULL, 1)) {
+		error(_("missing value to set to the variable '%s'"), arg);
+		advise(_("did you mean \"git config set %s %s\"?"),
+		       prefix, eq + 1);
+	} else if (!git_config_parse_key(arg, NULL, NULL, 1)) {
+		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 +1154,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 +1394,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 +1409,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 +1419,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..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
+'
+
+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: 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
+'
+
+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
+'
+
+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
 '
-- 
gitgitgadget

Re: [PATCH v5 1/2] config: let git_config_parse_key() validate quietly

[Junio C Hamano]

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

Re: [PATCH v5 2/2] config: improve diagnostic for "set" with missing value

[Junio C Hamano]

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

Re: [PATCH v5 1/2] config: let git_config_parse_key() validate quietly

[Harald Nordgren]

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


Harald

[PATCH v6 0/2] config: suggest the correct form when key contains "="

[Harald Nordgren via GitGitGadget]

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


base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2302%2FHaraldNordgren%2Fconfig-hint-equals-key-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2302/HaraldNordgren/config-hint-equals-key-v6
Pull-Request: https://github.com/git/git/pull/2302

Range-diff vs v5:

 1:  d938ebf95a ! 1:  7400ca41bb config: let git_config_parse_key() validate quietly
     @@ Metadata
      Author: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## Commit message ##
     -    config: let git_config_parse_key() validate quietly
     +    config: add git_config_key_is_valid() for quiet validation
      
     -    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.
     +    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.
      
     -    Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
     +    Add git_config_key_is_valid() for callers that only need to
     +    know whether a key is well-formed.
      
     - ## builtin/config.c ##
     -@@ builtin/config.c: static int get_value(const struct config_location_options *opts,
     - 			goto free_strings;
     - 		}
     - 	} else {
     --		if (git_config_parse_key(key_, &key, NULL)) {
     -+		if (git_config_parse_key(key_, &key, NULL, 0)) {
     - 			ret = CONFIG_INVALID_KEY;
     - 			goto free_strings;
     - 		}
     +    Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## config.c ##
      @@ config.c: static inline int iskeychar(int c)
     @@ config.c: static inline int iskeychar(int c)
      + * quiet - when non-zero, suppress error() reports on rejection
        */
      -int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
     -+int git_config_parse_key(const char *key, char **store_key, size_t *baselen_,
     -+			 int quiet)
     ++static int do_parse_config_key(const char *key, char **store_key,
     ++			       size_t *baselen_, int quiet)
       {
       	size_t i, baselen;
       	int dot;
     @@ config.c: int git_config_parse_key(const char *key, char **store_key, size_t *ba
       	return -CONFIG_INVALID_KEY;
       }
       
     -@@ config.c: static int config_parse_pair(const char *key, const char *value,
     - 
     - 	if (!strlen(key))
     - 		return error(_("empty config key"));
     --	if (git_config_parse_key(key, &canonical_name, NULL))
     -+	if (git_config_parse_key(key, &canonical_name, NULL, 0))
     - 		return -1;
     - 
     - 	ret = (fn(canonical_name, value, &ctx, data) < 0) ? -1 : 0;
     -@@ config.c: static int configset_find_element(struct config_set *set, const char *key,
     - 	 * `key` may come from the user, so normalize it before using it
     - 	 * for querying entries from the hashmap.
     - 	 */
     --	ret = git_config_parse_key(key, &normalized_key, NULL);
     -+	ret = git_config_parse_key(key, &normalized_key, NULL, 0);
     - 	if (ret)
     - 		return ret;
     - 
     -@@ config.c: int repo_config_set_multivar_in_file_gently(struct repository *r,
     - 	validate_comment_string(comment);
     - 
     - 	/* parse-key returns negative; flip the sign to feed exit(3) */
     --	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
     -+	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 0);
     - 	if (ret)
     - 		goto out_free;
     - 
     ++int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
     ++{
     ++	return do_parse_config_key(key, store_key, baselen_, 0);
     ++}
     ++
     ++int git_config_key_is_valid(const char *key)
     ++{
     ++	return !do_parse_config_key(key, NULL, NULL, 1);
     ++}
     ++
     + static int config_parse_pair(const char *key, const char *value,
     + 			     struct key_value_info *kvi,
     + 			     config_fn_t fn, void *data)
      
       ## config.h ##
     -@@ config.h: int repo_config_set_worktree_gently(struct repository *, const char *, const cha
     -  */
     - void repo_config_set(struct repository *, const char *, const char *);
     +@@ config.h: void repo_config_set(struct repository *, const char *, const char *);
       
     --int git_config_parse_key(const char *, char **, size_t *);
     -+int git_config_parse_key(const char *, char **, size_t *, int quiet);
     + int git_config_parse_key(const char *, char **, size_t *);
       
     ++int git_config_key_is_valid(const char *);
     ++
       /*
        * The following macros specify flag bits that alter the behavior
     -
     - ## submodule-config.c ##
     -@@ submodule-config.c: int print_config_from_gitmodules(struct repository *repo, const char *key)
     - 	int ret;
     - 	char *store_key;
     - 
     --	ret = git_config_parse_key(key, &store_key, NULL);
     -+	ret = git_config_parse_key(key, &store_key, NULL, 0);
     - 	if (ret < 0)
     - 		return CONFIG_INVALID_KEY;
     - 
     +  * of the repo_config_set_multivar*() methods.
 2:  e5a2070ee1 ! 2:  a7f8a084c7 config: improve diagnostic for "set" with missing value
     @@ builtin/config.c: static void check_argc(int argc, int min, int max)
      +	const char *eq = last_dot ? strchr(last_dot + 1, '=') : NULL;
      +	char *prefix = eq ? xstrndup(arg, eq - arg) : NULL;
      +
     -+	if (prefix && !git_config_parse_key(prefix, NULL, NULL, 1)) {
     ++	if (prefix && git_config_key_is_valid(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 (!git_config_parse_key(arg, NULL, NULL, 1)) {
     ++	} else if (git_config_key_is_valid(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'"),
     @@ t/t1300-config.sh: test_expect_success 'invalid key' '
      +
      +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 "invalid key: pull\\.rebase=false" err &&
      +	test_grep ! "did you mean" err
      +'
      +

-- 
gitgitgadget

[PATCH v6 1/2] config: add git_config_key_is_valid() for quiet validation

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

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>
---
 config.c | 38 +++++++++++++++++++++++++++++---------
 config.h |  2 ++
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/config.c b/config.c
index a1b92fe083..45144f73c5 100644
--- a/config.c
+++ b/config.c
@@ -536,11 +536,14 @@ static inline int iskeychar(int c)
  * -2 if there is no section name in the key.
  *
  * store_key - pointer to char* which will hold a copy of the key with
- *             lowercase section and variable name
+ *             lowercase section and variable name, can be NULL to skip
+ *             allocation when only validation is needed
  * baselen - pointer to size_t which will hold the length of the
  *           section + subsection part, can be NULL
+ * quiet - when non-zero, suppress error() reports on rejection
  */
-int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
+static int do_parse_config_key(const char *key, char **store_key,
+			       size_t *baselen_, int quiet)
 {
 	size_t i, baselen;
 	int dot;
@@ -552,12 +555,14 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 	 */
 
 	if (last_dot == NULL || last_dot == key) {
-		error(_("key does not contain a section: %s"), key);
+		if (!quiet)
+			error(_("key does not contain a section: %s"), key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
-		error(_("key does not contain variable name: %s"), key);
+		if (!quiet)
+			error(_("key does not contain variable name: %s"), key);
 		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
@@ -568,7 +573,8 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmallocz(strlen(key));
+	if (store_key)
+		*store_key = xmallocz(strlen(key));
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
@@ -579,24 +585,38 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 		if (!dot || i > baselen) {
 			if (!iskeychar(c) ||
 			    (i == baselen + 1 && !isalpha(c))) {
-				error(_("invalid key: %s"), key);
+				if (!quiet)
+					error(_("invalid key: %s"), key);
 				goto out_free_ret_1;
 			}
 			c = tolower(c);
 		} else if (c == '\n') {
-			error(_("invalid key (newline): %s"), key);
+			if (!quiet)
+				error(_("invalid key (newline): %s"), key);
 			goto out_free_ret_1;
 		}
-		(*store_key)[i] = c;
+		if (store_key)
+			(*store_key)[i] = c;
 	}
 
 	return 0;
 
 out_free_ret_1:
-	FREE_AND_NULL(*store_key);
+	if (store_key)
+		FREE_AND_NULL(*store_key);
 	return -CONFIG_INVALID_KEY;
 }
 
+int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
+{
+	return do_parse_config_key(key, store_key, baselen_, 0);
+}
+
+int git_config_key_is_valid(const char *key)
+{
+	return !do_parse_config_key(key, NULL, NULL, 1);
+}
+
 static int config_parse_pair(const char *key, const char *value,
 			     struct key_value_info *kvi,
 			     config_fn_t fn, void *data)
diff --git a/config.h b/config.h
index bf47fb3afc..31fe3e2961 100644
--- a/config.h
+++ b/config.h
@@ -343,6 +343,8 @@ void repo_config_set(struct repository *, const char *, const char *);
 
 int git_config_parse_key(const char *, char **, size_t *);
 
+int git_config_key_is_valid(const char *);
+
 /*
  * The following macros specify flag bits that alter the behavior
  * of the repo_config_set_multivar*() methods.
-- 
gitgitgadget

[PATCH v6 2/2] config: improve diagnostic for "set" with missing value

[Harald Nordgren via GitGitGadget]

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>
---
 builtin/config.c  | 32 ++++++++++++++++++++++++++-
 t/t1300-config.sh | 56 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index cf4ba0f7cc..8d8ec0beea 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,26 @@ static void check_argc(int argc, int min, int max)
 	exit(129);
 }
 
+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 && git_config_key_is_valid(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 (git_config_key_is_valid(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 +1154,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 +1394,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 +1409,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 +1419,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..87ca11a127 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -469,6 +469,62 @@ 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: 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
+'
+
+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
+'
+
+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 "invalid key: pull\\.rebase=false" 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
 '
-- 
gitgitgadget