From 3d41db1659ee288d6cab28e5b05e5c40f553d8e5 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 10 Jun 2022 11:42:15 +0900
Subject: [PATCH] config: move NO_COLOR handling from ui, don't override user
 setting

According to the NO_COLOR FAQ, "user-level configuration files [...] should
override $NO_COLOR." https://no-color.org/

Unfortunately this makes it harder to test the $NO_COLOR behavior since the
test environment isn't attached to a tty. We could allocate a pty or
LD_PRELOAD shim to intercept isatty(), but I feel it would be too much to do.

https://github.com/assert-rs/assert_cmd/issues/138
---
 CHANGELOG.md              | 3 +++
 src/config.rs             | 5 +++++
 src/ui.rs                 | 3 ---
 tests/test_global_opts.rs | 6 +++---
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 17fc008c7..d39c8e12a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * (#330) `jj branch` now uses subcommands like `jj branch create` and
   `jj branch forget` instead of options like `jj branch --forget`.
 
+* The [`$NO_COLOR` environment variable](https://no-color.org/) no longer
+  overrides the `ui.color` configuration if explicitly set.
+
 ### New features
 
 * `jj rebase` now accepts a `--branch/-b <revision>` argument, which can be used
diff --git a/src/config.rs b/src/config.rs
index c6baa967e..2ce5d0087 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -53,6 +53,11 @@ fn config_path() -> Result<Option<PathBuf>, ConfigError> {
 /// Environment variables that should be overridden by config values
 fn env_base() -> config::Config {
     let mut builder = config::Config::builder();
+    if env::var("NO_COLOR").is_ok() {
+        // "User-level configuration files and per-instance command-line arguments
+        // should override $NO_COLOR." https://no-color.org/
+        builder = builder.set_override("ui.color", "never").unwrap();
+    }
     if let Ok(value) = env::var("VISUAL") {
         builder = builder.set_override("ui.editor", value).unwrap();
     } else if let Ok(value) = env::var("EDITOR") {
diff --git a/src/ui.rs b/src/ui.rs
index bdd0d9730..21073e362 100644
--- a/src/ui.rs
+++ b/src/ui.rs
@@ -48,9 +48,6 @@ fn new_formatter<'output>(
 }
 
 fn use_color(settings: &UserSettings) -> bool {
-    if std::env::var("NO_COLOR").is_ok() {
-        return false;
-    }
     let color_setting = settings
         .config()
         .get_string("ui.color")
diff --git a/tests/test_global_opts.rs b/tests/test_global_opts.rs
index 5733440b8..09d66461b 100644
--- a/tests/test_global_opts.rs
+++ b/tests/test_global_opts.rs
@@ -109,12 +109,12 @@ color="always""#,
     o 0000000000000000000000000000000000000000
     "###);
 
-    // Test that NO_COLOR overrides the request for color in the config file
+    // Test that NO_COLOR does NOT override the request for color in the config file
     test_env.add_env_var("NO_COLOR", "");
     let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", "commit_id"]);
     insta::assert_snapshot!(stdout, @r###"
-    @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22
-    o 0000000000000000000000000000000000000000
+    @ 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    o 0000000000000000000000000000000000000000
     "###);
 }