Show 404 quoted lines
>
> 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