From 896ab5f1793332a86be15e570f91d8041527c934 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 17 Nov 2024 21:50:56 +0900
Subject: [PATCH] cli: fix "config unset" to not delete a whole table

It can be a footgun, and is inconsistent because other mutation commands can't
overwrite a table.
---
 CHANGELOG.md                     |  2 ++
 cli/src/config.rs                | 14 +++++++++---
 cli/tests/test_config_command.rs | 37 ++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e6809820c7..3ec90d38bd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -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
diff --git a/cli/src/config.rs b/cli/src/config.rs
index 5bdda27c5d..9f424567be 100644
--- a/cli/src/config.rs
+++ b/cli/src/config.rs
@@ -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!("Won't remove table {key}")));
+            }
+            entry.remove();
+        }
+        toml_edit::Entry::Vacant(_) => {
+            return Err(config::ConfigError::NotFound(key.to_string()).into());
+        }
+    }
 
     write_config(path, &doc)
 }
diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs
index 2a85a7f645..9e1de58c3f 100644
--- a/cli/tests/test_config_command.rs
+++ b/cli/tests/test_config_command.rs
@@ -14,6 +14,7 @@
 
 use std::path::PathBuf;
 
+use indoc::indoc;
 use insta::assert_snapshot;
 use itertools::Itertools;
 use regex::Regex;
@@ -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: Won't remove table non-inline-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();