From 564ce044fbf61866eb37878b0c30b9fe06ccf543 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 1 Dec 2024 14:28:39 +0900
Subject: [PATCH] ui: define default ui.color in config/misc.toml, use serde to
 parse value

It looks a bit odd that FromStr is forwarded to Deserialize, not the other way
around, but that's convenient because Deserialize can be derived.
---
 CHANGELOG.md                  |  2 +-
 cli/src/config/misc.toml      |  1 +
 cli/src/ui.rs                 | 28 ++++++++++------------------
 cli/tests/test_global_opts.rs | 16 ++++++++++++++++
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c2486a9351..295ecda3cb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,7 +16,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   not converted to a string `"true"`, and vice versa.
 
 * The following configuration variables are now parsed strictly:
-  `ui.progress-indicator`, `ui.quiet`
+  `ui.color`, `ui.progress-indicator`, `ui.quiet`
 
 * The deprecated `[alias]` config section is no longer respected. Move command
   aliases to the `[aliases]` section.
diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml
index dc370e919d..a315ebfb2d 100644
--- a/cli/src/config/misc.toml
+++ b/cli/src/config/misc.toml
@@ -17,6 +17,7 @@ context = 3
 # TODO: delete ui.allow-filesets in jj 0.26+
 allow-filesets = true
 always-allow-large-revsets = false
+color = "auto"
 diff-instructions = true
 graph.style = "curved"
 paginate = "auto"
diff --git a/cli/src/ui.rs b/cli/src/ui.rs
index f66c480829..d32e19c1de 100644
--- a/cli/src/ui.rs
+++ b/cli/src/ui.rs
@@ -36,6 +36,8 @@ use jj_lib::config::ConfigGetError;
 use jj_lib::config::StackedConfig;
 use minus::MinusError;
 use minus::Pager as MinusPager;
+use serde::de::Deserialize as _;
+use serde::de::IntoDeserializer as _;
 use tracing::instrument;
 
 use crate::command_error::CommandError;
@@ -262,7 +264,8 @@ pub struct Ui {
     output: UiOutput,
 }
 
-#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
+#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
 pub enum ColorChoice {
     Always,
     Never,
@@ -272,16 +275,13 @@ pub enum ColorChoice {
 }
 
 impl FromStr for ColorChoice {
-    type Err = &'static str;
+    type Err = String;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        match s {
-            "always" => Ok(ColorChoice::Always),
-            "never" => Ok(ColorChoice::Never),
-            "debug" => Ok(ColorChoice::Debug),
-            "auto" => Ok(ColorChoice::Auto),
-            _ => Err("must be one of always, never, or auto"),
-        }
+        // serde::de::value::Error is Box<str> wrapper. Map it to String to hide
+        // the implementation detail.
+        Self::deserialize(s.into_deserializer())
+            .map_err(|err: serde::de::value::Error| err.to_string())
     }
 }
 
@@ -297,20 +297,12 @@ impl fmt::Display for ColorChoice {
     }
 }
 
-fn color_setting(config: &StackedConfig) -> ColorChoice {
-    config
-        .get::<String>("ui.color")
-        .ok()
-        .and_then(|s| s.parse().ok())
-        .unwrap_or_default()
-}
-
 fn prepare_formatter_factory(
     config: &StackedConfig,
     stdout: &Stdout,
 ) -> Result<FormatterFactory, ConfigGetError> {
     let terminal = stdout.is_terminal();
-    let (color, debug) = match color_setting(config) {
+    let (color, debug) = match config.get("ui.color")? {
         ColorChoice::Always => (true, false),
         ColorChoice::Never => (false, false),
         ColorChoice::Debug => (true, true),
diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs
index b3573211df..503bf49541 100644
--- a/cli/tests/test_global_opts.rs
+++ b/cli/tests/test_global_opts.rs
@@ -455,6 +455,22 @@ fn test_color_config() {
     @  230dd059e1b059aefc0da06a2e5a7dbf22362f22
     ◆  0000000000000000000000000000000000000000
     "###);
+
+    // Invalid --color
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["log", "--color=foo"]);
+    insta::assert_snapshot!(stderr, @r"
+    error: invalid value 'foo' for '--color <WHEN>': unknown variant `foo`, expected one of `always`, `never`, `debug`, `auto`
+
+    For more information, try '--help'.
+    ");
+    // Invalid ui.color
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "--config-toml=ui.color=true"]);
+    insta::assert_snapshot!(stderr, @r"
+    Config error: Invalid type or value for ui.color
+    Caused by: wanted string or table
+
+    For help, see https://martinvonz.github.io/jj/latest/config/.
+    ");
 }
 
 #[test]