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.

assert-rs/assert_cmd#138

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

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") {

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")

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
+    @ [1;34m230dd059e1b059aefc0da06a2e5a7dbf22362f22[0m
+    o [34m0000000000000000000000000000000000000000[0m
     "###);
 }