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

["Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com>]

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