invalid config makes it hard to fix the config

[steveklabnik]

Description

I managed to mis-configure jj:

$ jj config set --user ui.paginate :builtin

An ominous silence (not really, it's just funnier to put it that way). This was an error, but I didn't get any error message.

This gives an error:

> jj st --no-pager
Config error: Invalid `ui.paginate`: enum PaginationChoice does not have variant constructor :builtin
For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.

That's fine, I'll just set it to auto, what I meant to set it to in the first place:

> jj config set --user ui.paginate auto
Config error: Invalid `ui.paginate`: enum PaginationChoice does not have variant constructor :builtin
For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.

Uh oh. Where does the config live so I can edit it? Well:

> jj config path --user
Config error: Invalid `ui.paginate`: enum PaginationChoice does not have variant constructor :builtin
For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.

Oh no.

Honestly this is a very funny bug.

Specifications

• Platform: Windows
• Version: 0.15.1

[steveklabnik]

(Oh and of course, manually looking up the config path, editing the file to fix it, fixes the issue)

[Kintaro]

Seems like the with_config method in Ui tries to read the pagination setting before the config command even has a chance to read or set any value.

Maybe a better default here would be to warn about the invalid setting but revert to a default in case of invalid settings?
Happy to take this on.

[Kintaro]

Same goes for the other values that are read by with_config and can fail. These are:

• progress_indicator_setting
• pager_setting
• pagination_setting
• formatter_factory

[yuja]

Seems like the with_config method in Ui tries to read the pagination setting before the config command even has a chance to read or set any value.

The error would be detected by ui.reset(), not by ::with_config().

Perhaps, failed ui.reset() can be translated to a warning? Maybe it's okay to propagate the error if the command isn't jj config, but I'm not sure if that's feasible.

[Kintaro]

Maybe the values in ui.reset() that are fallible could be used with defaults in case of error? A warning could still be displayed of course.

[yuja]

Maybe the values in ui.reset() that are fallible could be used with defaults in case of error? A warning could still be displayed of course.

That's also doable, but I don't think we need to try to parse broken user settings as much as possible. If the config file had syntax error, the error would be detected earlier. So we'll probably need some fallback at the call site (= CliRunner::run_internal()) to cover all edge cases.

[Kintaro]

So, check for any errors from reset, output the message but just continue? Or call reset again with some sane defaults, output the warning and continue?

[yuja]

So, check for any errors from reset, output the message but just continue?

Yes. ui should have the default values + some user values successfully parsed by reset().

Btw, git appears to print config file path and exit if the file can't be parsed. That's probably simpler and might be actually better because jj config command could fail in that situation.

If jj config set gained support for config schema, jj config set ui.paginate :builtin will fail. So the remaining problems will be when the user edited the config file manually. In that case, the file can be broken in various ways.

[Kintaro]

So, consensus is to print the path and just fail?

[yuja]

So, consensus is to print the path and just fail?

I personally think that's good enough. And the path thing will hopefully be resolved if we can upgrade config-rs. See #3023 (comment) for the blocker. Until then, maybe we can print all searched paths?

[Kintaro]

Oh, wasn't aware of #3023. I'll whip up a PR then and just print all searched paths.

[Kintaro]

With 3cd1fe4 merged, should this issue be closed?

[yuja]

With 3cd1fe4 merged, should this issue be closed?

Agreed. Closing this, thanks!

[steveklabnik]

Excellent! Thanks so much for implementing this!

Here's what it looks like:

$ jj config set --user ui.paginate :builtin
$ jj st --no-pager
Config error: Invalid `ui.paginate`
Caused by: enum PaginationChoice does not have variant constructor :builtin
Hint: Check the following config files:
- /home/steveklabnik/src/terra/.jj/repo/config.toml
- /home/steveklabnik/.config/jj/config.toml
For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.