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

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

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