RFC: cli --config global option: allow spaces around =

[ilyagr]

I know @yuja originally forbid this intentionally, but I find it confusing that something like

--config 'revsets."immutable_heads()" = "none()"'
--config 'ui.color = always'

was forbidden Update: while --config 'ui . color =always' works.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• [ ] I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[yuja]

Maybe you know, but the reason why whitespace is not stripped is that we allow bare strings. In POSIX shell, 'key= ' can also be expressed as key=' '. It seems a bit weird if --config=foo=' ' didn't set foo to (single space).

[ilyagr]

I think it's OK to require quotes, as in --config="foo=' '", for that. How often do people need to set a config to something with a leading space?

[ilyagr]

I was originally going to say:

We do already require quotes if the value is none(), as well as for a key like revsets."immutable_heads()", right?

Upon experimenting,

jj log --config 'revsets.immutable_heads()=none()'

fails but

jj log --config 'revsets."immutable_heads()"=none()'

works.

To me, this means that stripping the space before the = is definitely OK, but maybe there's something important I'm still missing about the space after the =?

The tests do seem to indicate that --config a=" " would work, though.

I added a test for this, seems to work.

[ilyagr]

Another confusing thing is that, before this PR, we have:

$ jj --config "ui . color =never" status  # Works fine
The working copy has no changes.
Working copy : olqzpw d8e5196 (empty) (no description set)
Parent commit: qqruox 386bb6d parse_bookmark_name plain
[22:14] macaw ~/dev/jj <qqru>→<olqz|∅>
$ jj --config "ui . color = never" status  # Boom!
Config error: Invalid type or value for ui.color
Caused by: unknown variant ` never`, expected one of `always`, `never`, `debug`, `auto`

For help, see https://jj-vcs.github.io/jj/latest/config/ or use `jj help -k config`.

After this PR, both would work.

[yuja]

To me, this means that stripping the space before the = is definitely OK,

Correct. The key part is parsed strictly, whereas the value parsing is lax.

For consistency reason, I wouldn't want to add extra whitespace handling to key parsing, though.

[ilyagr]

To me, this means that stripping the space before the = is definitely OK,

Correct. The key part is parsed strictly, whereas the value parsing is lax.

I think you're referring to the example in #6169 (comment) right before the sentence you quoted.

IIUC, you're implying that example is a victim of issues like #5748, which makes perfect sense then. It's a small price to pay.

For consistency reason, I wouldn't want to add extra whitespace handling to key parsing, though.

The key already seems agnostic to whitespace, I guess you're referring to value parsing? The inconsistency with keys vs values still bothers me, as well as the inconsistency with TOML syntax (which allows spaces around =s).

Update: OTOH, it's possible that you or your unconscious mind is worried about some consequence of allowing spaces before values that I'm missing, so if it feels really uncomfortable, I don't want to force the issue.

[yuja]

The key already seems agnostic to whitespace, I guess you're referring to value parsing? The inconsistency with keys vs values still bothers me, as well as the inconsistency with TOML syntax (which allows spaces around =s).

Yeah, it would be nice if the key parser were strict about whitespace (for consistency with the unquoted value handling), but I don't think it would worth the effort to roll our own key parsing function.