config: add layer.delete_value(name) method

Since .get("path.to.non-table.children") returns NotFound, I made
.delete_value() not fail in that case. The path doesn't exist, so
.delete_value() should be noop.

remove_config_value_from_file() will be inlined to callers.

cli/src/config.rs

@@ -469,36 +469,14 @@ pub fn remove_config_value_from_file(
     path: &Path,
 ) -> Result<(), CommandError> {
     // TODO: Load config layer by caller. Here we use a dummy source for now.
-    let mut doc = load_config_file_or_empty(ConfigSource::User, path)?.data;
+    let mut layer = load_config_file_or_empty(ConfigSource::User, path)?;
-
+    let old_value = layer
-    // Find target table
+        .delete_value(key)
-    let mut key_iter = key.components();
+        .map_err(|err| user_error_with_message(format!("Failed to unset {key}"), err))?;
-    let last_key = key_iter.next_back().expect("key must not be empty");
+    if old_value.is_none() {
-    let target_table = key_iter.try_fold(doc.as_table_mut(), |table, key| {
+        return Err(user_error(format!(r#""{key}" doesn't exist"#)));
-        table
-            .get_mut(key)
-            .ok_or_else(|| user_error(format!(r#""{key}" doesn't exist"#)))
-            .and_then(|table| {
-                table
-                    .as_table_mut()
-                    .ok_or_else(|| user_error(format!(r#""{key}" is not a table"#)))
-            })
-    })?;
-
-    // Remove config value
-    match target_table.entry(last_key) {
-        toml_edit::Entry::Occupied(entry) => {
-            if entry.get().is_table() {
-                return Err(user_error(format!("Won't remove table {key}")));
-            }
-            entry.remove();
-        }
-        toml_edit::Entry::Vacant(_) => {
-            return Err(user_error(format!(r#""{key}" doesn't exist"#)));
-        }
     }
-
+    write_config(path, &layer.data)
-    write_config(path, &doc)
 }
 
 /// Command name and arguments specified by config.

cli/tests/test_config_command.rs

@@ -655,7 +655,7 @@ fn test_config_unset_inline_table_key() {
         &["config", "unset", "--user", "inline-table.foo"],
     );
 
-    insta::assert_snapshot!(stderr, @r#"Error: "inline-table" is not a table"#);
+    insta::assert_snapshot!(stderr, @r#"Error: "inline-table.foo" doesn't exist"#);
 }
 
 #[test]
@@ -685,7 +685,10 @@ fn test_config_unset_table_like() {
         test_env.env_root(),
         &["config", "unset", "--user", "non-inline-table"],
     );
-    insta::assert_snapshot!(stderr, @"Error: Won't remove table non-inline-table");
+    insta::assert_snapshot!(stderr, @r"
+    Error: Failed to unset non-inline-table
+    Caused by: Would delete entire table non-inline-table
+    ");
 
     let user_config_toml = std::fs::read_to_string(&user_config_path).unwrap();
     insta::assert_snapshot!(user_config_toml, @r"

lib/src/config.rs

@@ -95,6 +95,12 @@ pub enum ConfigUpdateError {
         /// Dotted config name path.
         name: String,
     },
+    /// Table exists at the path, which shouldn't be deleted.
+    #[error("Would delete entire table {name}")]
+    WouldDeleteTable {
+        /// Dotted config name path.
+        name: String,
+    },
 }
 
 /// Extension methods for `Result<T, ConfigGetError>`.
@@ -392,6 +398,39 @@ 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.
+    pub fn delete_value(
+        &mut self,
+        name: impl ToConfigNamePath,
+    ) -> Result<Option<ConfigValue>, ConfigUpdateError> {
+        let would_delete_table = |name| ConfigUpdateError::WouldDeleteTable { name };
+        let name = name.into_name_path();
+        let name = name.borrow();
+        let mut keys = name.components();
+        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) =
+            keys.try_fold(root_table, |table, key| table.get_mut(key)?.as_table_mut())
+        else {
+            return Ok(None);
+        };
+        match parent_table.entry(leaf_key) {
+            toml_edit::Entry::Occupied(entry) => {
+                if !entry.get().is_value() {
+                    return Err(would_delete_table(name.to_string()));
+                }
+                let old_item = entry.remove();
+                Ok(Some(old_item.into_value().unwrap()))
+            }
+            toml_edit::Entry::Vacant(_) => Ok(None),
+        }
+    }
 }
 
 /// Looks up item from the `root_item`. Returns `Some(item)` if an item found at
@@ -759,6 +798,53 @@ mod tests {
         "#);
     }
 
+    #[test]
+    fn test_config_layer_delete_value() {
+        let mut layer = ConfigLayer::empty(ConfigSource::User);
+        // Cannot delete the root table
+        assert_matches!(
+            layer.delete_value(ConfigNamePathBuf::root()),
+            Err(ConfigUpdateError::WouldDeleteTable { name }) if name.is_empty()
+        );
+
+        // Insert some values
+        layer.set_value("foo", 1).unwrap();
+        layer.set_value("bar.baz.blah", "2").unwrap();
+        layer
+            .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")]))
+            .unwrap();
+        insta::assert_snapshot!(layer.data, @r#"
+        foo = 1
+
+        [bar]
+        qux = { inline = "table" }
+
+        [bar.baz]
+        blah = "2"
+        "#);
+
+        // Can delete value
+        let old_value = layer.delete_value("foo").unwrap();
+        assert_eq!(old_value.and_then(|v| v.as_integer()), Some(1));
+        // Can delete inline table
+        let old_value = layer.delete_value("bar.qux").unwrap();
+        assert!(old_value.is_some_and(|v| v.is_inline_table()));
+        // Cannot delete table
+        assert_matches!(
+            layer.delete_value("bar"),
+            Err(ConfigUpdateError::WouldDeleteTable { name }) if name == "bar"
+        );
+        // Deleting a non-table child isn't an error because the value doesn't
+        // exist
+        assert_matches!(layer.delete_value("bar.baz.blah.blah"), Ok(None));
+        insta::assert_snapshot!(layer.data, @r#"
+        [bar]
+
+        [bar.baz]
+        blah = "2"
+        "#);
+    }
+
     #[test]
     fn test_stacked_config_layer_order() {
         let empty_data = || DocumentMut::new();