search

martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
7.34k stars 242 forks source link

`jj config edit` doesn't reject invalid TOML #3905

Open leeavital opened 2 weeks ago

leeavital commented 2 weeks ago

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

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

yuja commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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