Skip to content

Commit

Permalink
cli: fix "config unset" to not delete a whole table
Browse files Browse the repository at this point in the history
It can be a footgun, and is inconsistent because other mutation commands can't
overwrite a table.
  • Loading branch information
yuja committed Nov 17, 2024
1 parent a22620d commit 9a065d0
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### Fixed bugs

* `jj config unset <TABLE-NAME>` no longer removes a table (such as `[ui]`.)

## [0.23.0] - 2024-11-06

### Security fixes
Expand Down
14 changes: 11 additions & 3 deletions cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,9 +669,17 @@ pub fn remove_config_value_from_file(
})?;

// Remove config value
target_table
.remove(last_key)
.ok_or_else(|| config::ConfigError::NotFound(key.to_string()))?;
match target_table.entry(last_key) {
toml_edit::Entry::Occupied(entry) => {
if entry.get().is_table() {
return Err(user_error(format!("Cannot remove {key}: is a table")));
}
entry.remove();
}
toml_edit::Entry::Vacant(_) => {
return Err(config::ConfigError::NotFound(key.to_string()).into());
}
}

write_config(path, &doc)
}
Expand Down
37 changes: 37 additions & 0 deletions cli/tests/test_config_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use std::path::PathBuf;

use indoc::indoc;
use insta::assert_snapshot;
use itertools::Itertools;
use regex::Regex;
Expand Down Expand Up @@ -642,6 +643,42 @@ fn test_config_unset_inline_table_key() {
"###);
}

#[test]
fn test_config_unset_table_like() {
let mut test_env = TestEnvironment::default();
// Point to a config file since `config unset` can't handle directories.
let user_config_path = test_env.config_path().join("config.toml");
test_env.set_config_path(user_config_path.clone());

std::fs::write(
&user_config_path,
indoc! {b"
inline-table = { foo = true }
[non-inline-table]
foo = true
"},
)
.unwrap();

// Inline table is a "value", so it can be deleted.
test_env.jj_cmd_success(
test_env.env_root(),
&["config", "unset", "--user", "inline-table"],
);
// Non-inline table cannot be deleted.
let stderr = test_env.jj_cmd_failure(
test_env.env_root(),
&["config", "unset", "--user", "non-inline-table"],
);
insta::assert_snapshot!(stderr, @"Error: Cannot remove non-inline-table: is a table");

let user_config_toml = std::fs::read_to_string(&user_config_path).unwrap();
insta::assert_snapshot!(user_config_toml, @r"
[non-inline-table]
foo = true
");
}

#[test]
fn test_config_unset_for_user() {
let mut test_env = TestEnvironment::default();
Expand Down

0 comments on commit 9a065d0

Please sign in to comment.