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

[Junio C Hamano <gitster@pobox.com>]

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