[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

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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

>> Perhaps the updated "git_config_parse_key()" in this patch should be
>> renamed to be a file-scape static internal helper, and the existing
>> "git_config_parse_key()" should become a thin wrapper around that
>> new helper function, retaining the current external interface,
>> requiring no changes to existing callers.
>
> I want to remember a discussion on one of my earlier topics, a few
> months back, where someone else suggested instead of introducing two
> thin wrappers over a helper, we should update the callers instead.
>
> But for me either way is fine, maybe here it makes more sense, because
> of the repeated NULL/0/1 parameters.

If the "quiet" and "store_key" setting were independent, then I
wouldn't have made such a suggestion.  But I got an impression that
with the updated code, there wasn't a valid use case to ask to
quietly store the discovered key.

An ideal refactoring would have been a low level helper function
that only yields error code, and git_config_parse_key() would call
it and react to the returned error code, stores the discovered key,
and produces error message on its own.  Then such an "always quiet"
helper can be used for the purpose of the new caller, without having
to have "if (!quiet)" sprinkled all over.  But that is certainly
cumbersome to arrange.

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

[Junio C Hamano]

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

>  * The quiet parameter now lives on a static do_parse_config_key() instead
>    of git_config_parse_key() itself. git_config_parse_key() is back to its
>    three-argument signature; existing callers don't change.
>  * New public git_config_key_is_valid() for callers that only need a yes/no
>    check.
>
> Harald Nordgren (2):
>   config: add git_config_key_is_valid() for quiet validation
>   config: improve diagnostic for "set" with missing value
>
>  builtin/config.c  | 32 ++++++++++++++++++++++++++-
>  config.c          | 38 ++++++++++++++++++++++++--------
>  config.h          |  2 ++
>  t/t1300-config.sh | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 118 insertions(+), 10 deletions(-)

Looking good.  Thanks.  Will queue.