`jj config edit` doesn't reject invalid TOML

[leeavital]

Description

Steps to Reproduce the Problem

1. Edit user config jj config edit --user
2. Add some invalid cargo like:

[ui]
editor = vim

Note the lack of quotes around vim

1. quit your editor

Expected Behavior

jj config edit --user quits with a message like "could not save, invalid toml"

Actual Behavior

Any jj command (including jj config edit --user) fails with:

Config error: newline in string found at line 7 column 14 in ../../../../../Library/Application Support/jj/config.toml

Until you fix the problem.

Specifications

• Version: 0.18.0

[yuja]

jj config edit --user quits with a message like "could not save, invalid toml"

Just to be clear, it probably means jj config edit will create a temporary copy for editing, and copy it back at end.

[martinvonz]

Good point. I don't know if users expect changes to the file to take effect immediately. If we think they do, we could create the temporary copy for backup instead.

[ilyagr]

Most programs that I can think of open the real config file when you use some sort of an "edit configuration" command. Kitty terminal and VS Code come to mind. As Martin pointed out, I do expect being able to save the config file in one tab and immediately test it out in another tab.

VS Code seems to have some logic to use a last known OK config (I think?) if the syntax of the config file is broken. I wonder what they do exactly.

I also wonder whether it's ever dangerous to run jj with an unexpected config. If not, perhaps either storing a last known good config, or a last known minimal good config (e.g. just the user name and email) might be worth it.

Update: If we go into the direction of "minimal good config", it might make sense to simply store those "minimal" options in a separate config file that jj config edit does not touch.

[ilyagr]

OTOH, it would be nice if jj config set was a "safe" interface that never broke the config file syntax and perhaps even warned the user about semantic errors. I think it's a good distance on the way there.

[leeavital]

I took a crack at the 'temporary file solution' in https://github.com/martinvonz/jj/pull/3945

[dbarnett]

Instead of editing a temporary file copy, couldn't it just try validating the config after edit, print the validation error, and prompt for whether to reopen the same editor to fix errors?

[martinvonz]

Yes, I think so. And if they don't want to reopen the editor, we can revert to the old value we stored in a backup. Or maybe your suggestion is to skip the backup? I'm not sure what your "instead" refers to.

[dbarnett]

Yeah, I'd imagined just leaving the invalid config saved as-is (for them to manually fix) if they don't want to reopen the editor but restoring a backup would be fine too.

You can ignore the "instead" part, I think I just misunderstood how the temporary file was getting used but it makes sense now. =)

[drieber]

@leeavital if you are not currently working on this I would like to pick it up.

Reading through the comments above I sense there is still room for arguing in favor of editing a temp file, then validating it, and if valid overwriting the actual config with the temp copy. My gut feeling is that would be safer than editing the live config file. I don't feel strongly about it though.

How do people feel about adding a knob to choose between the two behaviors of jj config edit behaviors? Something like edit-live-config = true|false ?

[martinvonz]

I also don't feel strongly about which way we go. One argument in favor of editing a temporary file is that we do something like that when we run diff editors and merge tools.

I do feel pretty strongly that we should not have a config option for it. That would involve implementing both options, which is probably not worth it unless we know that we have two separate groups of users who really want it one way or the other.

[yuja]

My feeling is that editing config file directly is less surprising. If you're updating color table, you'll probably want to preview the change without closing the editor.

I do feel pretty strongly that we should not have a config option for it.

+1

[drieber]

Point taken. Lets not add a knob. I think it is best to edit a temporary file. The documentation would of course cover this. If users want to edit config, then try it, then edit again, then try again and so on, they can run jj config edit multiple times.

But then again I don't feel very strongly about it.

[BatmanAoD]

As an incremental improvement, could jj edit at least print a warning after the file is closed if the config is invalid and/or has unrecognized options?

(My point of confusion that could have been improved with such a warning: the git version of aliases is called alias, and I didn't notice the difference at first and was confused as to why I couldn't get an alias to work.)

---

Most programs that I can think of open the real config file when you use some sort of an "edit configuration" command.

I have no strong feelings about editing a temp file vs the real file, but I thought I'd mention that there are some classic counterexamples: visudo and vipw, and crontab -e. Of course these are not exactly "configuration" files, and in all three cases the consequences of a malformed file are much worse than they are for jj config. But I think it's fair to say, at least, that there is precedent for the "write to a tempfile" approach.

As Martin pointed out, I do expect being able to save the config file in one tab and immediately test it out in another tab.

I do find this useful, but regardless of the behavior of config edit, testing out a config file while editing it is simpler with <visual-editor> "$(jj config path --user)" anyway, as long as <visual-editor> opens a separate tab or window.