diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 0dfdc7bcf..65ace23d8 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -691,7 +691,10 @@ fn test_config_unset_non_existent_key() { #[test] fn test_config_unset_inline_table_key() { - let test_env = TestEnvironment::default(); + let mut test_env = TestEnvironment::default(); + // Test with fresh new config file + let user_config_path = test_env.config_path().join("config.toml"); + test_env.set_config_path(&user_config_path); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); @@ -699,12 +702,12 @@ fn test_config_unset_inline_table_key() { &repo_path, &["config", "set", "--user", "inline-table", "{ foo = true }"], ); - let stderr = test_env.jj_cmd_failure( + test_env.jj_cmd_ok( &repo_path, &["config", "unset", "--user", "inline-table.foo"], ); - - insta::assert_snapshot!(stderr, @r#"Error: "inline-table.foo" doesn't exist"#); + let user_config_toml = std::fs::read_to_string(&user_config_path).unwrap(); + insta::assert_snapshot!(user_config_toml, @"inline-table = {}"); } #[test] @@ -724,7 +727,7 @@ fn test_config_unset_table_like() { ) .unwrap(); - // Inline table is a "value", so it can be deleted. + // Inline table is syntactically a "value", so it can be deleted. test_env.jj_cmd_success( test_env.env_root(), &["config", "unset", "--user", "inline-table"], diff --git a/lib/src/config.rs b/lib/src/config.rs index 1432cd61b..bf0cd3ce6 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -103,13 +103,14 @@ pub enum ConfigUpdateError { /// Dotted config name path. name: String, }, - /// Table exists at the path, which shouldn't be overwritten by a value. + /// Non-inline table exists at the path, which shouldn't be overwritten by a + /// value. #[error("Would overwrite entire table {name}")] WouldOverwriteTable { /// Dotted config name path. name: String, }, - /// Table exists at the path, which shouldn't be deleted. + /// Non-inline table exists at the path, which shouldn't be deleted. #[error("Would delete entire table {name}")] WouldDeleteTable { /// Dotted config name path. @@ -397,7 +398,8 @@ impl ConfigLayer { /// Sets `new_value` to the `name` path. Returns old value if any. /// /// This function errors out if attempted to overwrite a non-table middle - /// node or a leaf table (in the same way as file/directory operation.) + /// node or a leaf non-inline table. An inline table can be overwritten + /// because it's syntactically a value. pub fn set_value( &mut self, name: impl ToConfigNamePath, @@ -411,7 +413,6 @@ impl ConfigLayer { })?; match parent_table.entry_format(leaf_key) { toml_edit::Entry::Occupied(mut entry) => { - // TODO: Don't overwrite inline table because it is a merge-able item if !entry.get().is_value() { return Err(ConfigUpdateError::WouldOverwriteTable { name: name.to_string(), @@ -433,7 +434,8 @@ impl ConfigLayer { /// Deletes value specified by the `name` path. Returns old value if any. /// /// Returns `Ok(None)` if middle node wasn't a table or a value wasn't - /// found. Returns `Err` if attempted to delete a table. + /// found. Returns `Err` if attempted to delete a non-inline table. An + /// inline table can be deleted because it's syntactically a value. pub fn delete_value( &mut self, name: impl ToConfigNamePath, @@ -445,16 +447,14 @@ impl ConfigLayer { let leaf_key = keys .next_back() .ok_or_else(|| would_delete_table(name.to_string()))?; - let root_table = self.data.as_table_mut(); - let Some(parent_table) = - // TODO: as_table_like_mut() - keys.try_fold(root_table, |table, key| table.get_mut(key)?.as_table_mut()) - else { + let Some(parent_table) = keys.try_fold( + self.data.as_table_mut() as &mut ConfigTableLike, + |table, key| table.get_mut(key)?.as_table_like_mut(), + ) else { return Ok(None); }; match parent_table.entry(leaf_key) { toml_edit::Entry::Occupied(entry) => { - // TODO: Don't delete inline table because it is a merge-able item if !entry.get().is_value() { return Err(would_delete_table(name.to_string())); } @@ -488,15 +488,14 @@ fn look_up_item<'a>( /// Inserts tables down to the parent of the `name` path. Returns `Err(keys)` if /// middle node exists at the prefix name `keys` and wasn't a table. fn ensure_parent_table<'a, 'b>( - root_table: &'a mut ConfigTable, + root_table: &'a mut ConfigTableLike<'a>, name: &'b ConfigNamePathBuf, -) -> Result<(&'a mut ConfigTable, &'b toml_edit::Key), &'b [toml_edit::Key]> { +) -> Result<(&'a mut ConfigTableLike<'a>, &'b toml_edit::Key), &'b [toml_edit::Key]> { let mut keys = name.components(); let leaf_key = keys.next_back().ok_or(&name.0[..])?; let parent_table = keys.enumerate().try_fold(root_table, |table, (i, key)| { let sub_item = table.entry_format(key).or_insert_with(new_implicit_table); - // TODO: as_table_like_mut() - sub_item.as_table_mut().ok_or(&name.0[..=i]) + sub_item.as_table_like_mut().ok_or(&name.0[..=i]) })?; Ok((parent_table, leaf_key)) } @@ -884,11 +883,15 @@ mod tests { layer .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")])) .unwrap(); + layer + .set_value("bar.to-update", ConfigValue::from_iter([("some", true)])) + .unwrap(); insta::assert_snapshot!(layer.data, @r#" foo = 1 [bar] qux = { inline = "table" } + to-update = { some = true } [bar.baz] blah = "2" @@ -900,6 +903,13 @@ mod tests { .unwrap(); // Can overwrite inline table layer.set_value("bar.qux", "new bar.qux").unwrap(); + // Can add value to inline table + layer + .set_value( + "bar.to-update.new", + ConfigValue::from_iter([("table", "value")]), + ) + .unwrap(); // Cannot overwrite table assert_matches!( layer.set_value("bar", 0), @@ -915,6 +925,7 @@ mod tests { [bar] qux = "new bar.qux" + to-update = { some = true, new = { table = "value" } } [bar.baz] blah = "2" @@ -961,11 +972,15 @@ mod tests { layer .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")])) .unwrap(); + layer + .set_value("bar.to-update", ConfigValue::from_iter([("some", true)])) + .unwrap(); insta::assert_snapshot!(layer.data, @r#" foo = 1 [bar] qux = { inline = "table" } + to-update = { some = true } [bar.baz] blah = "2" @@ -977,6 +992,9 @@ mod tests { // Can delete inline table let old_value = layer.delete_value("bar.qux").unwrap(); assert!(old_value.is_some_and(|v| v.is_inline_table())); + // Can delete inner value from inline table + let old_value = layer.delete_value("bar.to-update.some").unwrap(); + assert_eq!(old_value.and_then(|v| v.as_bool()), Some(true)); // Cannot delete table assert_matches!( layer.delete_value("bar"), @@ -986,6 +1004,9 @@ mod tests { // exist assert_matches!(layer.delete_value("bar.baz.blah.blah"), Ok(None)); insta::assert_snapshot!(layer.data, @r#" + [bar] + to-update = {} + [bar.baz] blah = "2" "#); diff --git a/lib/src/config_resolver.rs b/lib/src/config_resolver.rs index ea8c8cec3..70f99e928 100644 --- a/lib/src/config_resolver.rs +++ b/lib/src/config_resolver.rs @@ -24,7 +24,6 @@ use serde::Deserialize as _; use toml_edit::DocumentMut; use crate::config::ConfigGetError; -use crate::config::ConfigItem; use crate::config::ConfigLayer; use crate::config::ConfigNamePathBuf; use crate::config::ConfigUpdateError; @@ -170,15 +169,12 @@ fn pop_scope_tables(layer: &mut ConfigLayer) -> Result Ok(tables), - _ => Err(ConfigGetError::Type { + item.into_array_of_tables() + .map_err(|item| ConfigGetError::Type { name: SCOPE_TABLE_KEY.to_owned(), error: format!("Expected an array of tables, but is {}", item.type_name()).into(), source_path: layer.path.clone(), - }), - } + }) } /// Rule to migrate deprecated config variables.