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
This commit is contained in:
Yuya Nishihara 2022-06-10 11:42:15 +09:00
parent d90fd8bcb4
commit 3d41db1659
4 changed files with 11 additions and 6 deletions

View file

@ -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

View file

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

View file

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

View file

@ -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
"###);
}