From ee17109501428319311d18f9bfc339a2a6dec88b Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Thu, 18 Jun 2026 22:26:42 +0200 Subject: [PATCH] Fix NO_COLOR to only suppress colors, not all ANSI styles The NO_COLOR convention (https://no-color.org) only calls for suppressing ANSI *color* codes. It does not demand the removal of other attributes such as bold, italic, underline or reverse. Previously, Paint::is_enabled() bundled anstyle_query::no_color() into the single boolean that decides whether to emit any formatting. Because Paint relies on that boolean alone, NO_COLOR ended up stripping bold, italic, reverse, etc. alongside the color codes. That conflates two concerns: terminal styling support and color suppression. It is also contrary to the intent of the standard. Instead, treat the general style decision separately from the color decision. Remove the no_color() check from is_enabled() and add Paint::is_styling_enabled() for the coarse terminal gate. Style::fmt_prefix and fmt_suffix now query NO_COLOR directly and conditionally drop foreground and background codes while preserving non-colour properties. This way bold, italic, underline and the like continue to work when NO_COLOR is set, while colors alone are suppressed. Closes: rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5/cob/xyz.radicle.issue/f922d90 Assisted-by: Pi:deepinfra/moonshotai/Kimi-K2.6 Signed-off-by: Richard Levitte --- crates/radicle-term/src/ansi/paint.rs | 25 ++++++++--- crates/radicle-term/src/ansi/style.rs | 34 ++++++++++++--- crates/radicle-term/src/ansi/tests.rs | 62 +++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/crates/radicle-term/src/ansi/paint.rs b/crates/radicle-term/src/ansi/paint.rs index d7579879..f0365f4a 100644 --- a/crates/radicle-term/src/ansi/paint.rs +++ b/crates/radicle-term/src/ansi/paint.rs @@ -261,7 +261,11 @@ impl Paint { impl fmt::Display for Paint { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - if Paint::is_enabled() && self.style.wrap { + if !Paint::is_enabled() { + return fmt::Display::fmt(&self.item, f); + } + + if self.style.wrap { let mut prefix = String::new(); prefix.push_str("\x1B[0m"); self.style.fmt_prefix(&mut prefix)?; @@ -270,19 +274,20 @@ impl fmt::Display for Paint { let item = format!("{}", self.item).replace("\x1B[0m", &prefix); fmt::Display::fmt(&item, f)?; self.style.fmt_suffix(f) - } else if Paint::is_enabled() { + } else { self.style.fmt_prefix(f)?; fmt::Display::fmt(&self.item, f)?; self.style.fmt_suffix(f) - } else { - fmt::Display::fmt(&self.item, f) } } } impl Paint<()> { - /// Returns `true` if coloring is enabled and `false` otherwise. - pub fn is_enabled() -> bool { + /// Returns `true` if ANSI styling (bold, italic, underline, reverse, etc.) + /// is enabled. This reflects the general terminal capability and user + /// preferences governed by `CLICOLOR`, `ENABLED` / `FORCED`, and terminal + /// detection. It does **not** take `NO_COLOR` into account. + pub fn is_styling_enabled() -> bool { if FORCED.load(sync::atomic::Ordering::SeqCst) { return true; } @@ -295,12 +300,18 @@ impl Paint<()> { is_terminal && is_enabled - && !anstyle_query::no_color() && !clicolor_disabled && (anstyle_query::term_supports_color() || clicolor_enabled || anstyle_query::is_ci()) || anstyle_query::clicolor_force() } + /// Returns `true` if any painting is enabled. + /// + /// This is equivalent to `is_styling_enabled()`. + pub fn is_enabled() -> bool { + Self::is_styling_enabled() + } + /// Check 24-bit RGB color support. pub fn truecolor() -> bool { static TRUECOLOR: LazyLock = LazyLock::new(anstyle_query::term_supports_color); diff --git a/crates/radicle-term/src/ansi/style.rs b/crates/radicle-term/src/ansi/style.rs index 0324835a..14cc6901 100644 --- a/crates/radicle-term/src/ansi/style.rs +++ b/crates/radicle-term/src/ansi/style.rs @@ -21,6 +21,11 @@ impl Property { Property(0) } + #[inline(always)] + pub const fn is_empty(self) -> bool { + self.0 == 0 + } + #[inline(always)] pub const fn contains(self, other: Property) -> bool { (other.0 & self.0) == other.0 @@ -235,10 +240,16 @@ impl Style { /// is currently enabled or disabled. To write the prefix only if painting /// is enabled, condition a call to this method on [`Paint::is_enabled()`]. pub fn fmt_prefix(&self, f: &mut dyn fmt::Write) -> fmt::Result { + let color_enabled = !anstyle_query::no_color(); + // A user may just want a code-free string when no styles are applied. if self.is_plain() { return Ok(()); } + // When colors are disabled and there are no non-color properties, there's nothing to emit. + if !color_enabled && self.properties.is_empty() { + return Ok(()); + } let mut splice = false; write!(f, "\x1B[")?; @@ -248,14 +259,16 @@ impl Style { write_spliced(&mut splice, f, k)?; } - if self.background != Color::Unset { - write_spliced(&mut splice, f, "4")?; - self.background.ansi_fmt(f)?; - } + if color_enabled { + if self.background != Color::Unset { + write_spliced(&mut splice, f, "4")?; + self.background.ansi_fmt(f)?; + } - if self.foreground != Color::Unset { - write_spliced(&mut splice, f, "3")?; - self.foreground.ansi_fmt(f)?; + if self.foreground != Color::Unset { + write_spliced(&mut splice, f, "3")?; + self.foreground.ansi_fmt(f)?; + } } // All the codes end with an `m`. @@ -272,9 +285,16 @@ impl Style { /// is currently enabled or disabled. To write the suffix only if painting /// is enabled, condition a call to this method on [`Paint::is_enabled()`]. pub fn fmt_suffix(&self, f: &mut dyn fmt::Write) -> fmt::Result { + let color_enabled = !anstyle_query::no_color(); + + // A user may just want a code-free string when no styles are applied. if self.is_plain() { return Ok(()); } + // When colors are disabled and there are no non-color properties, there's nothing to emit. + if !color_enabled && self.properties.is_empty() { + return Ok(()); + } write!(f, "\x1B[0m") } } diff --git a/crates/radicle-term/src/ansi/tests.rs b/crates/radicle-term/src/ansi/tests.rs index fff06161..43d0d6c4 100644 --- a/crates/radicle-term/src/ansi/tests.rs +++ b/crates/radicle-term/src/ansi/tests.rs @@ -278,3 +278,65 @@ fn wrapping() { .to_string() ); } + +#[test] +fn no_color_disables_colors_not_styles() { + let _guard = SERIAL.lock(); + + Paint::force(true); + Paint::enable(); + + // Without NO_COLOR, colors and styles are both emitted. + assert_eq!( + Paint::red("hi").bold().to_string(), + "\x1B[1;31mhi\x1B[0m".to_string() + ); + + unsafe { std::env::set_var("NO_COLOR", "1") } + + // With NO_COLOR, foreground and background colors are suppressed, + // but bold, italic, underline, invert etc. are preserved. + assert_eq!( + Paint::red("hi").bold().to_string(), + "\x1B[1mhi\x1B[0m".to_string() + ); + assert_eq!(Paint::red("hi").to_string(), "hi".to_string()); + assert_eq!( + Paint::new("hi").bold().to_string(), + "\x1B[1mhi\x1B[0m".to_string() + ); + assert_eq!( + Paint::new("hi").italic().to_string(), + "\x1B[3mhi\x1B[0m".to_string() + ); + assert_eq!( + Paint::new("hi").invert().to_string(), + "\x1B[7mhi\x1B[0m".to_string() + ); + + // Wrapping without colors should still preserve inner style resets. + let inner = || { + format!( + "{} b {}", + Paint::red("a").bold(), + Paint::green("c").underline() + ) + }; + assert_eq!( + Paint::new(&inner()).wrap().to_string(), + "\u{1b}[1ma\u{1b}[0m b \u{1b}[4mc\u{1b}[0m".to_string() + ); + + unsafe { std::env::remove_var("NO_COLOR") } + + // When all styling is explicitly disabled, no ANSI codes are emitted at all. + Paint::force(false); + Paint::disable(); + assert_eq!(Paint::red("hi").bold().to_string(), "hi".to_string()); + assert_eq!(Paint::new("hi").bold().to_string(), "hi".to_string()); + assert_eq!(Paint::new("hi").italic().to_string(), "hi".to_string()); + assert_eq!(Paint::new("hi").invert().to_string(), "hi".to_string()); + assert_eq!(Paint::blue("hi").underline().to_string(), "hi".to_string()); + Paint::force(true); + Paint::enable(); +}