From 6dc9721602252d5bb3d9d189f6b97e95b34df7f0 Mon Sep 17 00:00:00 2001 From: indaco Date: Sun, 26 Jul 2026 17:03:20 +0200 Subject: [PATCH 1/2] fix(ui): decide colour per output stream Commands that print their colourised rows to stdout were deciding whether to colour by probing stderr, so redirecting stdout to a file from a terminal wrote raw escape sequences into the file. Each now asks about the stream it actually writes to. Colour already resolved to a policy, and always/never are stream- independent, so only the auto case gained a per-stream probe. --- .../stdout-color-decided-from-stderr-tty.sh | 114 +++++++++++ src/cli/info.zig | 2 +- src/cli/list.zig | 4 +- src/cli/outdated/render.zig | 6 +- src/cli/rollback.zig | 2 +- src/cli/search.zig | 2 +- src/cli/uses.zig | 2 +- src/cli/which.zig | 2 +- src/ui/color.zig | 177 +++++++++++++++--- 9 files changed, 276 insertions(+), 35 deletions(-) create mode 100755 scripts/regressions/stdout-color-decided-from-stderr-tty.sh diff --git a/scripts/regressions/stdout-color-decided-from-stderr-tty.sh b/scripts/regressions/stdout-color-decided-from-stderr-tty.sh new file mode 100755 index 00000000..d8e5ccfb --- /dev/null +++ b/scripts/regressions/stdout-color-decided-from-stderr-tty.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# Regression: commands that write their colourised rows to stdout decided +# whether to colour by probing *stderr*. Run from a terminal with stdout +# redirected - `mt info jq > out.txt` - and raw ANSI landed in the file. +# +# Only a real terminal on stderr can reproduce it, so the run happens under +# script(1): the child gets a pty, and the inner redirect sends stdout to a +# file while stderr stays on the pty. That is the exact user-facing shape. +# +# The second half proves the forced policy still reaches the stdout path: +# CLICOLOR_FORCE=1 must colourise even when stdout is a plain file. +# +# Hermetic: a seeded keg in a throwaway prefix, MALT_OFFLINE=1, no network. +# +# Usage: scripts/regressions/stdout-color-decided-from-stderr-tty.sh +# Requirements: built `malt` at $MALT_BIN or zig-out/bin/malt, sqlite3 on PATH. + +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) +BIN="${MALT_BIN:-$ROOT/zig-out/bin/malt}" +[[ -x "$BIN" ]] || { + echo "build malt first: zig build" >&2 + exit 2 +} +command -v sqlite3 >/dev/null 2>&1 || { + echo "this regression needs sqlite3 on PATH" >&2 + exit 2 +} + +PREFIX="$(mktemp -d)" +OUTDIR="$PREFIX/out" +mkdir -p "$PREFIX/db" "$OUTDIR" +export MALT_PREFIX="$PREFIX" +export MALT_NO_EMOJI=1 +export MALT_OFFLINE=1 +trap 'rm -rf "$PREFIX"' EXIT + +pass() { printf ' \xe2\x9c\x93 %s\n' "$*"; } +fail() { + printf ' \xe2\x9c\x97 %s\n' "$*" >&2 + exit 1 +} + +ESC=$'\033[' + +DB="$PREFIX/db/malt.db" + +# Let malt bootstrap the schema, then seed one keg plus the bin symlink +# `mt which` resolves through. +"$BIN" list --quiet >/dev/null 2>&1 || true +[[ -f "$DB" ]] || fail "DB was not initialised by mt list" +sqlite3 "$DB" <"$PREFIX/Cellar/jq/1.7.1/bin/jq" +ln -s "$PREFIX/Cellar/jq/1.7.1/bin/jq" "$PREFIX/bin/jq" + +# Every subcommand here builds a stdout writer and styles what it writes. +CMDS=( + "info jq" + "which jq" + "list" + "search --installed jq" + "uses jq" +) + +slug() { printf '%s' "$1" | tr -c 'a-z0-9' '_'; } + +# script(1) hands the child a fresh pty and records it; the inner `>` peels +# stdout off that pty, leaving stderr on it. shellcheck cannot see the inner +# expansion. +# shellcheck disable=SC2016 +run_under_pty() { + local cmd="$1" out="$2" tty_log="$3" + MT_BIN="$BIN" MT_CMD="$cmd" MT_OUT="$out" \ + script -q "$tty_log" sh -c '$MT_BIN $MT_CMD >"$MT_OUT"' /dev/null 2>&1 || true +} + +for cmd in "${CMDS[@]}"; do + out="$OUTDIR/$(slug "$cmd").txt" + run_under_pty "$cmd" "$out" /dev/null + [[ -s "$out" ]] || fail "mt $cmd wrote nothing to the redirected stdout" + grep -qF "$ESC" "$out" && + fail "mt $cmd leaked ANSI into a redirected stdout (stderr was the terminal)" + pass "mt $cmd: redirected stdout stayed plain" +done + +# The other half of the divergence, and the reason the checks above are not +# vacuous: in the same run, stderr is still a terminal and must KEEP its +# colour. Without this, silencing colour on both streams would pass. +run_under_pty "which definitely-not-installed" "$OUTDIR/miss.txt" "$OUTDIR/miss.tty" +grep -qF "$ESC" "$OUTDIR/miss.tty" || + fail "stderr lost its colour while stdout was redirected; both streams went plain" +pass "stderr kept its colour in the same run" + +# The forced policy is stream-independent: no terminal anywhere, colour anyway. +for cmd in "${CMDS[@]}"; do + out="$OUTDIR/forced_$(slug "$cmd").txt" + # shellcheck disable=SC2086 # $cmd carries its own arguments; splitting is the point + CLICOLOR_FORCE=1 "$BIN" $cmd >"$out" 2>/dev/null || true + grep -qF "$ESC" "$out" || + fail "CLICOLOR_FORCE=1 did not colourise mt $cmd on a piped stdout" + pass "mt $cmd: CLICOLOR_FORCE=1 colourised the redirected stdout" +done + +printf '\n\xe2\x9c\x94 stdout colour follows the stdout stream\n' diff --git a/src/cli/info.zig b/src/cli/info.zig index 048140b5..5fe92b98 100644 --- a/src/cli/info.zig +++ b/src/cli/info.zig @@ -87,7 +87,7 @@ pub fn execute(ctx: *const AppCtx, allocator: std.mem.Allocator, args: []const [ // Flush on teardown; stdout closed by a broken pipe is normal shell usage. defer stdout.flush() catch {}; - const colorize = !json_mode and color.isColorEnabled(); + const colorize = !json_mode and color.isColorEnabledFor(.stdout); // `--cask`/`--formula` are inclusive selectors, like `search`: each // flag opts its kind in, and "both set" reads the same as "neither set" diff --git a/src/cli/list.zig b/src/cli/list.zig index 07463e14..186e6dc0 100644 --- a/src/cli/list.zig +++ b/src/cli/list.zig @@ -295,7 +295,7 @@ fn pinnedUnionSql(show_formula: bool, show_cask: bool, tap_filter: bool) ?[:0]co /// Emit the leading cyan bullet + space, honouring `NO_COLOR`. fn writeBulletPrefix(stdout: *std.Io.Writer) void { - if (color.isColorEnabled()) { + if (color.isColorEnabledFor(.stdout)) { stdout.writeAll(color.SemanticStyle.info.code()) catch return; stdout.writeAll(" ▸ ") catch return; stdout.writeAll(color.Style.reset.code()) catch return; @@ -313,7 +313,7 @@ fn writeStyledSpan( body: []const u8, close: []const u8, ) void { - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledFor(.stdout); if (use_color) stdout.writeAll(style_code) catch return; stdout.writeAll(open) catch return; stdout.writeAll(body) catch return; diff --git a/src/cli/outdated/render.zig b/src/cli/outdated/render.zig index b05b5163..5a03634b 100644 --- a/src/cli/outdated/render.zig +++ b/src/cli/outdated/render.zig @@ -85,7 +85,7 @@ pub fn writeJsonArray( /// Match the `mt list` / `mt search` row shape: cyan bullet, plain /// name, dimmed `(installed)`, warn-coloured `≠ latest`, and an /// optional dim `[kind]` tag for casks. Honours `NO_COLOR` / pipes -/// automatically via `color.isColorEnabled()`. +/// automatically via `color.isColorEnabledFor(.stdout)`. fn writeEntry(stdout: *std.Io.Writer, e: OutdatedEntry, kind_tag: ?[]const u8) void { if (output.isQuiet()) { stdout.writeAll(e.name) catch return; @@ -112,7 +112,7 @@ fn isDowngrade(installed: []const u8, latest: []const u8) bool { } fn writeBullet(stdout: *std.Io.Writer) void { - if (color.isColorEnabled()) { + if (color.isColorEnabledFor(.stdout)) { stdout.writeAll(color.SemanticStyle.info.code()) catch return; stdout.writeAll(" \xe2\x96\xb8 ") catch return; stdout.writeAll(color.Style.reset.code()) catch return; @@ -128,7 +128,7 @@ fn writeStyledSpan( body: []const u8, close: []const u8, ) void { - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledFor(.stdout); if (use_color) stdout.writeAll(style_code) catch return; stdout.writeAll(open) catch return; stdout.writeAll(body) catch return; diff --git a/src/cli/rollback.zig b/src/cli/rollback.zig index 5b1a5295..fb5436ef 100644 --- a/src/cli/rollback.zig +++ b/src/cli/rollback.zig @@ -557,7 +557,7 @@ fn printListing( if (output.isJson()) { try writeListJson(&aw.writer, name, entries); } else { - try writeListHuman(&aw.writer, name, entries, color.isColorEnabled()); + try writeListHuman(&aw.writer, name, entries, color.isColorEnabledFor(.stdout)); } output.writeStdoutAll(aw.written()); } diff --git a/src/cli/search.zig b/src/cli/search.zig index 25e84f99..2d4ec842 100644 --- a/src/cli/search.zig +++ b/src/cli/search.zig @@ -986,7 +986,7 @@ test "loadInstalledKind returns every installed name for the kind, sorted" { /// Write a single search result with the same ▸ prefix style used by `list`. fn writeResult(stdout: *std.Io.Writer, name: []const u8, kind: []const u8) void { - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledFor(.stdout); if (use_color) stdout.writeAll(color.SemanticStyle.info.code()) catch return; stdout.writeAll(" \xe2\x96\xb8 ") catch return; if (use_color) stdout.writeAll(color.Style.reset.code()) catch return; diff --git a/src/cli/uses.zig b/src/cli/uses.zig index 92056517..18e5866d 100644 --- a/src/cli/uses.zig +++ b/src/cli/uses.zig @@ -153,7 +153,7 @@ pub fn writeHuman(stdout: *std.Io.Writer, target: []const u8, dependents: [][]co stdout.writeAll(line) catch return; return; } - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledFor(.stdout); for (dependents) |d| { if (use_color) stdout.writeAll(color.SemanticStyle.info.code()) catch return; stdout.writeAll(" \xe2\x96\xb8 ") catch return; diff --git a/src/cli/which.zig b/src/cli/which.zig index 235bc67d..870270e2 100644 --- a/src/cli/which.zig +++ b/src/cli/which.zig @@ -129,7 +129,7 @@ pub fn execute(ctx: *const AppCtx, _: std.mem.Allocator, args: []const []const u if (output.isJson()) { try encodeJson(stdout, res); } else { - try encodeHuman(stdout, res, color.isColorEnabled()); + try encodeHuman(stdout, res, color.isColorEnabledFor(.stdout)); } } diff --git a/src/ui/color.zig b/src/ui/color.zig index e2fe1af6..feffc83e 100644 --- a/src/ui/color.zig +++ b/src/ui/color.zig @@ -118,7 +118,10 @@ fn bgToCache(bg: Background) BgCache { }; } -var color_enabled: Tri = .unresolved; +// One cache per stream, because `.auto` folds the policy against a per-fd TTY +// probe and `isTty` is a syscall the per-line writers must not repeat. +var stdout_enabled: Tri = .unresolved; +var stderr_enabled: Tri = .unresolved; var color_policy_cached: PolicyCache = .unresolved; var emoji_enabled: Tri = .unresolved; var background_cached: BgCache = .unresolved; @@ -155,7 +158,8 @@ var active_custom: usize = AC_UNRESOLVED; pub fn setRuntime(io: std.Io, environ: std.process.Environ) void { pkg_io = io; pkg_environ = environ; - @atomicStore(Tri, &color_enabled, .unresolved, .release); + @atomicStore(Tri, &stdout_enabled, .unresolved, .release); + @atomicStore(Tri, &stderr_enabled, .unresolved, .release); @atomicStore(PolicyCache, &color_policy_cached, .unresolved, .release); @atomicStore(Tri, &emoji_enabled, .unresolved, .release); @atomicStore(BgCache, &background_cached, .unresolved, .release); @@ -191,17 +195,30 @@ fn lookupEnv(name: []const u8) ?[:0]const u8 { } /// Test-only override for the TTY probe so the policy → colour fold can be -/// exercised in both terminal states without a pty. Pass `null` to release. -var tty_override: ?bool = null; +/// exercised in both terminal states without a pty. Indexed by fd so stdout and +/// stderr can disagree, which is the whole point of a per-stream decision. +/// Pass `null` to release. +var tty_override: [3]?bool = @splat(null); +/// Overrides every standard fd at once — the common case, and what the probes +/// that read stdin (OSC 11) need. pub fn setTtyForTest(v: ?bool) void { if (!builtin.is_test) return; - tty_override = v; + tty_override = @splat(v); +} + +/// Overrides one stream, leaving the others alone: seeds a redirected stdout +/// under a terminal stderr without touching the stdin probe. +pub fn setStreamTtyForTest(stream: Stream, v: ?bool) void { + if (!builtin.is_test) return; + tty_override[@intCast(fdFor(stream))] = v; } fn isTty(fd: std.posix.fd_t) bool { if (builtin.is_test) { - if (tty_override) |v| return v; + if (fd >= 0 and fd < tty_override.len) { + if (tty_override[@intCast(fd)]) |v| return v; + } } const file: std.Io.File = .{ .handle = fd, .flags = .{ .nonblocking = false } }; return file.isTty(pkg_io) catch false; @@ -290,8 +307,29 @@ test "resolveColorPolicy: NO_COLOR disables on presence alone, whatever its valu try std.testing.expectEqual(ColorPolicy.never, resolveColorPolicy("0", null, null)); } -pub fn isColorEnabled() bool { - switch (@atomicLoad(Tri, &color_enabled, .acquire)) { +/// Which stream a colour question is about. A decision made for stderr is the +/// wrong answer for a writer that targets stdout, and vice versa. +pub const Stream = enum { stdout, stderr }; + +fn fdFor(stream: Stream) std.posix.fd_t { + return switch (stream) { + .stdout => std.posix.STDOUT_FILENO, + .stderr => std.posix.STDERR_FILENO, + }; +} + +fn enabledCache(stream: Stream) *Tri { + return switch (stream) { + .stdout => &stdout_enabled, + .stderr => &stderr_enabled, + }; +} + +/// The colour question, asked about the stream the caller actually writes to. +/// Writes to stdout must ask `.stdout`; `isColorEnabled()` answers for stderr. +pub fn isColorEnabledFor(stream: Stream) bool { + const cache = enabledCache(stream); + switch (@atomicLoad(Tri, cache, .acquire)) { .yes => return true, .no => return false, .unresolved => {}, @@ -299,15 +337,81 @@ pub fn isColorEnabled() bool { const result: Tri = switch (colorPolicy()) { .always => .yes, .never => .no, - // Unchanged pre-CLICOLOR behaviour; also the only arm that costs a syscall. - .auto => if (isTty(std.posix.STDERR_FILENO)) .yes else .no, + // Unchanged pre-CLICOLOR behaviour; the only arm that costs a syscall, + // and the only one whose answer can differ between the two streams. + .auto => if (isTty(fdFor(stream))) .yes else .no, }; // Benign double-compute under a race: the value is env-deterministic, so - // last-writer-wins on identical data — no CAS needed. - @atomicStore(Tri, &color_enabled, result, .release); + // last-writer-wins on identical data — no CAS needed. Each stream stores + // only its own cache: folding a stream-independent policy into both here + // would let one stream's first touch publish a cache it does not own. + @atomicStore(Tri, cache, result, .release); return result == .yes; } +test "under .auto each stream answers from its own fd" { + // The regression this exists for: `mt info tree > out.txt` from a terminal. + // A stderr-derived answer writes raw ANSI into the file. + const io = std.Options.debug_io; + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); + defer { + setTtyForTest(null); + setForTest(null, null); + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); + } + setStreamTtyForTest(.stdout, false); // redirected to a file + setStreamTtyForTest(.stderr, true); // still attached to the terminal + + try std.testing.expect(!isColorEnabledFor(.stdout)); + try std.testing.expect(isColorEnabledFor(.stderr)); +} + +test "a forced or vetoed policy answers the same for both streams" { + // `.always`/`.never` are stream-independent by construction — that is what + // keeps per-stream evaluation an addition rather than a second decision path. + const io = std.Options.debug_io; + defer { + setTtyForTest(null); + setForTest(null, null); + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); + } + + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{"CLICOLOR_FORCE=1"} } }); + setTtyForTest(false); // no terminal anywhere, and it must not matter + try std.testing.expect(isColorEnabledFor(.stdout)); + try std.testing.expect(isColorEnabledFor(.stderr)); + + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{"NO_COLOR=1"} } }); + setTtyForTest(true); // terminals everywhere, and it must not matter + try std.testing.expect(!isColorEnabledFor(.stdout)); + try std.testing.expect(!isColorEnabledFor(.stderr)); +} + +test "isColorEnabled is the stderr answer, not a copy that can drift" { + // The shim keeps ~20 stderr call sites unchanged; if it ever stops tracking + // `.stderr` exactly, those callers silently follow the wrong stream. + const io = std.Options.debug_io; + defer { + setTtyForTest(null); + setForTest(null, null); + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); + } + + for ([_]bool{ true, false }) |stderr_is_tty| { + setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); + setStreamTtyForTest(.stdout, !stderr_is_tty); // opposite, so a stdout-derived shim shows up + setStreamTtyForTest(.stderr, stderr_is_tty); + try std.testing.expectEqual(isColorEnabledFor(.stderr), isColorEnabled()); + try std.testing.expectEqual(stderr_is_tty, isColorEnabled()); + } +} + +/// Stderr shim, kept so the many stderr writers read unchanged. Review +/// convention: a code path that writes stdout must call `isColorEnabledFor`. +pub fn isColorEnabled() bool { + return isColorEnabledFor(.stderr); +} + test "CLICOLOR_FORCE=1 emits colour even when stderr is not a terminal" { // The behaviour that is impossible without a tri-state: piping into `less -R` // or asking a CI job for ANSI. `.always` must never reach the TTY probe. @@ -368,7 +472,8 @@ pub fn isEmojiEnabled() bool { /// the next `is*Enabled` call recompute from env. pub fn setForTest(c: ?bool, e: ?bool) void { if (!builtin.is_test) return; - @atomicStore(Tri, &color_enabled, triFromOpt(c), .release); + @atomicStore(Tri, &stdout_enabled, triFromOpt(c), .release); + @atomicStore(Tri, &stderr_enabled, triFromOpt(c), .release); @atomicStore(Tri, &emoji_enabled, triFromOpt(e), .release); } @@ -409,12 +514,17 @@ fn typeIsEnum(comptime T: type) bool { } test "storage: color/policy/emoji caches are atomic sentinel enums, not optionals" { - try std.testing.expect(typeIsEnum(@TypeOf(color_enabled))); + // Both stream caches, not just one: splitting the enablement cache in two + // doubled the storage this guard has to cover. + try std.testing.expect(typeIsEnum(@TypeOf(stdout_enabled))); + try std.testing.expect(typeIsEnum(@TypeOf(stderr_enabled))); try std.testing.expect(typeIsEnum(@TypeOf(color_policy_cached))); // PolicyCache, not ?ColorPolicy try std.testing.expect(typeIsEnum(@TypeOf(emoji_enabled))); // ColorPolicy stays a clean tri-state the fold switches over exhaustively; // the unresolved state lives only in the cache enum. try std.testing.expect(!@hasField(ColorPolicy, "unresolved")); + // Stream is a plain selector, never a cache: no unresolved variant to leak. + try std.testing.expect(!@hasField(Stream, "unresolved")); } // Encodes WHY the atomic rewrite matters: workers reach these accessors through @@ -422,18 +532,27 @@ test "storage: color/policy/emoji caches are atomic sentinel enums, not optional // resolved. It cannot *prove* race-freedom (TSan is unavailable here) — the // sentinel storage provides that, pinned by the structural guard above. test "concurrent first-touch on the enabled caches agrees and resolves" { - setRuntime(std.Options.debug_io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{"NO_COLOR=1"} } }); + // Seeded divergent on purpose: racers touch BOTH streams cold, so a fold + // that published a stream-independent answer into both caches would let one + // stream's first touch decide the other's — the exact cross-cache write the + // three-cache split exists to keep out. + setRuntime(std.Options.debug_io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); defer { + setTtyForTest(null); setForTest(null, null); setRuntime(std.Options.debug_io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); } + setStreamTtyForTest(.stdout, false); + setStreamTtyForTest(.stderr, true); const Runner = struct { - color: bool = false, + out: bool = false, + err: bool = false, emoji: bool = false, - policy: ColorPolicy = .auto, + policy: ColorPolicy = .never, fn run(self: *@This()) void { - self.color = isColorEnabled(); + self.out = isColorEnabledFor(.stdout); + self.err = isColorEnabledFor(.stderr); self.emoji = isEmojiEnabled(); self.policy = colorPolicy(); } @@ -444,11 +563,13 @@ test "concurrent first-touch on the enabled caches agrees and resolves" { for (&threads) |t| t.join(); for (runners) |r| { - try std.testing.expectEqual(runners[0].color, r.color); + try std.testing.expect(!r.out); // every racer sees the redirected stdout + try std.testing.expect(r.err); // and the terminal stderr try std.testing.expectEqual(runners[0].emoji, r.emoji); - try std.testing.expectEqual(ColorPolicy.never, r.policy); // NO_COLOR vetoes for every racer + try std.testing.expectEqual(ColorPolicy.auto, r.policy); // one policy, shared by both streams } - try std.testing.expect(@atomicLoad(Tri, &color_enabled, .acquire) != .unresolved); + try std.testing.expect(@atomicLoad(Tri, &stdout_enabled, .acquire) != .unresolved); + try std.testing.expect(@atomicLoad(Tri, &stderr_enabled, .acquire) != .unresolved); try std.testing.expect(@atomicLoad(PolicyCache, &color_policy_cached, .acquire) != .unresolved); try std.testing.expect(@atomicLoad(Tri, &emoji_enabled, .acquire) != .unresolved); } @@ -482,16 +603,22 @@ test "setRuntime drops the enabled/policy/truecolor/background caches so a new e setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); } - try std.testing.expect(!isColorEnabled()); // CLICOLOR=0 beats the terminal + // Both streams touched, so the re-seed has two enablement caches to drop. + try std.testing.expect(!isColorEnabledFor(.stdout)); // CLICOLOR=0 beats the terminal + try std.testing.expect(!isColorEnabled()); try std.testing.expect(isEmojiEnabled()); try std.testing.expect(truecolorSupported()); try std.testing.expectEqual(Background.light, background()); - try std.testing.expect(@atomicLoad(Tri, &color_enabled, .acquire) != .unresolved); + try std.testing.expect(@atomicLoad(Tri, &stdout_enabled, .acquire) != .unresolved); + try std.testing.expect(@atomicLoad(Tri, &stderr_enabled, .acquire) != .unresolved); // New environ flips each derived value; a frozen cache would keep the old one. setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{ "MALT_THEME=dark", "MALT_NO_EMOJI=1" } } }); - try std.testing.expect(@atomicLoad(Tri, &color_enabled, .acquire) == .unresolved); // the cache was dropped - try std.testing.expect(isColorEnabled()); // CLICOLOR gone ⇒ back to the probe, which says terminal + try std.testing.expect(@atomicLoad(Tri, &stdout_enabled, .acquire) == .unresolved); // both caches dropped + try std.testing.expect(@atomicLoad(Tri, &stderr_enabled, .acquire) == .unresolved); + try std.testing.expect(@atomicLoad(PolicyCache, &color_policy_cached, .acquire) == .unresolved); + try std.testing.expect(isColorEnabledFor(.stdout)); // CLICOLOR gone ⇒ back to the probe, which says terminal + try std.testing.expect(isColorEnabled()); try std.testing.expect(!isEmojiEnabled()); // MALT_NO_EMOJI now set try std.testing.expect(!truecolorSupported()); // COLORTERM now gone try std.testing.expectEqual(Background.dark, background()); // MALT_THEME flipped From a40d27b84640b1d2140c7e8cc40049caa8173fb7 Mon Sep 17 00:00:00 2001 From: indaco Date: Sun, 26 Jul 2026 17:39:43 +0200 Subject: [PATCH 2/2] refactor(ui): name the stderr colour helper for its stream isColorEnabled() read like the general question but answered only for stderr, so a new stdout writer calling it by habit reintroduced the bug this branch just fixed. Renaming it to isColorEnabledForStderr leaves no default to get wrong; every caller now names the stream it writes to. Mechanical rename, no behaviour change. --- src/cli/doctor.zig | 2 +- src/cli/doctor/render.zig | 2 +- src/cli/migrate.zig | 2 +- src/cli/purge/report.zig | 8 ++++---- src/ui/color.zig | 35 ++++++++++++++++++----------------- src/ui/output.zig | 6 +++--- src/ui/progress.zig | 10 +++++----- src/update/notifier.zig | 2 +- 8 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/cli/doctor.zig b/src/cli/doctor.zig index b83c8c88..97ec30a5 100644 --- a/src/cli/doctor.zig +++ b/src/cli/doctor.zig @@ -586,7 +586,7 @@ pub fn emitFixHintIfNeeded(fix_requested: bool) void { fn writeStyledDetail(text: []const u8) void { var line_buf: [1024]u8 = undefined; const line = std.fmt.bufPrint(&line_buf, " - {s}\n", .{text}) catch return; - if (color.isColorEnabled()) { + if (color.isColorEnabledForStderr()) { output.writeStderrAll(color.SemanticStyle.detail.code()); output.writeStderrAll(line); output.writeStderrAll(color.Style.reset.code()); diff --git a/src/cli/doctor/render.zig b/src/cli/doctor/render.zig index 1411c41a..b77e09d8 100644 --- a/src/cli/doctor/render.zig +++ b/src/cli/doctor/render.zig @@ -189,7 +189,7 @@ pub fn printCheck(name: []const u8, status: CheckStatus, detail: ?[]const u8) vo var w: std.Io.Writer = .fixed(&buf); // Truncation of an oversized row is acceptable - `buffered()` still emits what fit. renderCheckRow(&w, status, name, detail, .{ - .color = color.isColorEnabled(), + .color = color.isColorEnabledForStderr(), .emoji = color.isEmojiEnabled(), }) catch {}; output.writeStderrAll(w.buffered()); diff --git a/src/cli/migrate.zig b/src/cli/migrate.zig index 78dd1cd0..04881e64 100644 --- a/src/cli/migrate.zig +++ b/src/cli/migrate.zig @@ -501,7 +501,7 @@ fn emitBrewUninstallHint( var line: std.ArrayList(u8) = .empty; defer line.deinit(allocator); - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledForStderr(); const prefix: []const u8 = if (color.isEmojiEnabled()) " ▸ " else " > "; if (use_color) try line.appendSlice(allocator, color.SemanticStyle.detail.code()); try line.appendSlice(allocator, prefix); diff --git a/src/cli/purge/report.zig b/src/cli/purge/report.zig index eb71fbfb..91a2dcc9 100644 --- a/src/cli/purge/report.zig +++ b/src/cli/purge/report.zig @@ -129,7 +129,7 @@ pub const Reporter = struct { fn writeItem(text: []const u8) void { if (output.isQuiet()) return; // raw stderr writes don't honour --quiet on their own const bullet: []const u8 = if (color.isEmojiEnabled()) " • " else " - "; - if (color.isColorEnabled()) { + if (color.isColorEnabledForStderr()) { output.writeStderrAll(color.SemanticStyle.detail.code()); output.writeStderrAll(bullet); output.writeStderrAll(color.Style.reset.code()); @@ -148,7 +148,7 @@ fn writeMore(remaining: usize) void { " … {d} more (use --verbose to show all)", .{remaining}, ) catch return; - if (color.isColorEnabled()) { + if (color.isColorEnabledForStderr()) { output.writeStderrAll(color.SemanticStyle.detail.code()); output.writeStderrAll(msg); output.writeStderrAll(color.Style.reset.code()); @@ -190,10 +190,10 @@ fn writeRule() void { if (output.isQuiet()) return; var buf: [rule_width]u8 = undefined; @memset(buf[0..], '-'); - if (color.isColorEnabled()) output.writeStderrAll(color.SemanticStyle.detail.code()); + if (color.isColorEnabledForStderr()) output.writeStderrAll(color.SemanticStyle.detail.code()); output.writeStderrAll(" "); output.writeStderrAll(buf[0..rule_width]); - if (color.isColorEnabled()) output.writeStderrAll(color.Style.reset.code()); + if (color.isColorEnabledForStderr()) output.writeStderrAll(color.Style.reset.code()); output.writeStderrAll("\n"); } diff --git a/src/ui/color.zig b/src/ui/color.zig index feffc83e..ce6efabc 100644 --- a/src/ui/color.zig +++ b/src/ui/color.zig @@ -326,7 +326,8 @@ fn enabledCache(stream: Stream) *Tri { } /// The colour question, asked about the stream the caller actually writes to. -/// Writes to stdout must ask `.stdout`; `isColorEnabled()` answers for stderr. +/// There is no default: every caller names its stream, or uses the named +/// `isColorEnabledForStderr()` helper. pub fn isColorEnabledFor(stream: Stream) bool { const cache = enabledCache(stream); switch (@atomicLoad(Tri, cache, .acquire)) { @@ -387,9 +388,9 @@ test "a forced or vetoed policy answers the same for both streams" { try std.testing.expect(!isColorEnabledFor(.stderr)); } -test "isColorEnabled is the stderr answer, not a copy that can drift" { - // The shim keeps ~20 stderr call sites unchanged; if it ever stops tracking - // `.stderr` exactly, those callers silently follow the wrong stream. +test "isColorEnabledForStderr is the stderr answer, not a copy that can drift" { + // The named helper keeps the many stderr call sites short; if it ever stops + // tracking `.stderr` exactly, those callers silently follow the wrong stream. const io = std.Options.debug_io; defer { setTtyForTest(null); @@ -401,14 +402,14 @@ test "isColorEnabled is the stderr answer, not a copy that can drift" { setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); setStreamTtyForTest(.stdout, !stderr_is_tty); // opposite, so a stdout-derived shim shows up setStreamTtyForTest(.stderr, stderr_is_tty); - try std.testing.expectEqual(isColorEnabledFor(.stderr), isColorEnabled()); - try std.testing.expectEqual(stderr_is_tty, isColorEnabled()); + try std.testing.expectEqual(isColorEnabledFor(.stderr), isColorEnabledForStderr()); + try std.testing.expectEqual(stderr_is_tty, isColorEnabledForStderr()); } } -/// Stderr shim, kept so the many stderr writers read unchanged. Review -/// convention: a code path that writes stdout must call `isColorEnabledFor`. -pub fn isColorEnabled() bool { +/// Convenience for the many stderr writers. Named for its stream so a stdout +/// writer cannot reach for it by habit and silently ask the wrong question. +pub fn isColorEnabledForStderr() bool { return isColorEnabledFor(.stderr); } @@ -423,7 +424,7 @@ test "CLICOLOR_FORCE=1 emits colour even when stderr is not a terminal" { setForTest(null, null); setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); } - try std.testing.expect(isColorEnabled()); + try std.testing.expect(isColorEnabledForStderr()); } test "CLICOLOR=0 suppresses colour even when stderr is a terminal" { @@ -435,7 +436,7 @@ test "CLICOLOR=0 suppresses colour even when stderr is a terminal" { setForTest(null, null); setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); } - try std.testing.expect(!isColorEnabled()); + try std.testing.expect(!isColorEnabledForStderr()); } test "with none of the three set, colour still follows the stderr TTY probe" { @@ -449,11 +450,11 @@ test "with none of the three set, colour still follows the stderr TTY probe" { setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); setTtyForTest(true); - try std.testing.expect(isColorEnabled()); + try std.testing.expect(isColorEnabledForStderr()); setRuntime(io, .{ .block = .{ .slice = &[_:null]?[*:0]const u8{} } }); setTtyForTest(false); - try std.testing.expect(!isColorEnabled()); + try std.testing.expect(!isColorEnabledForStderr()); } pub fn isEmojiEnabled() bool { @@ -504,7 +505,7 @@ pub fn setThemeForTest(t: ?Theme) void { // ─── atomic sentinel cache guards ──────────────────────────────────── // // The env/TTY caches are read on first touch from worker threads (a migrate -// worker reaches `isColorEnabled`/`isEmojiEnabled` via `output.warn`), so their +// worker reaches `isColorEnabledFor`/`isEmojiEnabled` via `output.warn`), so their // storage must be atomic-friendly sentinel enums — never `?T`, whose layout is // not guaranteed atomic. These structural guards fail to build the moment a // cache reverts to an optional. @@ -605,7 +606,7 @@ test "setRuntime drops the enabled/policy/truecolor/background caches so a new e // Both streams touched, so the re-seed has two enablement caches to drop. try std.testing.expect(!isColorEnabledFor(.stdout)); // CLICOLOR=0 beats the terminal - try std.testing.expect(!isColorEnabled()); + try std.testing.expect(!isColorEnabledForStderr()); try std.testing.expect(isEmojiEnabled()); try std.testing.expect(truecolorSupported()); try std.testing.expectEqual(Background.light, background()); @@ -618,7 +619,7 @@ test "setRuntime drops the enabled/policy/truecolor/background caches so a new e try std.testing.expect(@atomicLoad(Tri, &stderr_enabled, .acquire) == .unresolved); try std.testing.expect(@atomicLoad(PolicyCache, &color_policy_cached, .acquire) == .unresolved); try std.testing.expect(isColorEnabledFor(.stdout)); // CLICOLOR gone ⇒ back to the probe, which says terminal - try std.testing.expect(isColorEnabled()); + try std.testing.expect(isColorEnabledForStderr()); try std.testing.expect(!isEmojiEnabled()); // MALT_NO_EMOJI now set try std.testing.expect(!truecolorSupported()); // COLORTERM now gone try std.testing.expectEqual(Background.dark, background()); // MALT_THEME flipped @@ -647,7 +648,7 @@ test "test setters round-trip; null recomputes from env" { // Forced values are observed verbatim. setForTest(true, false); - try std.testing.expect(isColorEnabled()); + try std.testing.expect(isColorEnabledForStderr()); try std.testing.expect(!isEmojiEnabled()); setBackgroundForTest(.dark); try std.testing.expectEqual(Background.dark, background()); diff --git a/src/ui/output.zig b/src/ui/output.zig index 47ec38bb..1a53ca0e 100644 --- a/src/ui/output.zig +++ b/src/ui/output.zig @@ -282,7 +282,7 @@ fn writePrefixLine( msg: []const u8, ) void { const prefix: []const u8 = if (color.isEmojiEnabled()) emoji_prefix else plain_prefix; - const colorize = color.isColorEnabled(); + const colorize = color.isColorEnabledForStderr(); lockStderr(); defer unlockStderr(); switch (shape) { @@ -357,7 +357,7 @@ pub fn question(comptime fmt: []const u8, args: anytype) void { var buf: [4096]u8 = undefined; const msg = std.fmt.bufPrint(&buf, fmt, args) catch return; const prefix: []const u8 = " ? "; - const colorize = color.isColorEnabled(); + const colorize = color.isColorEnabledForStderr(); lockStderr(); defer unlockStderr(); if (colorize) { @@ -379,7 +379,7 @@ fn lineStyled(style_code: ?[]const u8, comptime fmt: []const u8, args: anytype) if (quiet) return; var buf: [4096]u8 = undefined; const msg = std.fmt.bufPrint(&buf, fmt, args) catch return; - const colorize = color.isColorEnabled(); + const colorize = color.isColorEnabledForStderr(); lockStderr(); defer unlockStderr(); if (style_code) |code| { diff --git a/src/ui/progress.zig b/src/ui/progress.zig index 6695da6b..a97a019d 100644 --- a/src/ui/progress.zig +++ b/src/ui/progress.zig @@ -443,7 +443,7 @@ pub const ProgressBar = struct { // Glyph: animated spinner while in progress, green ✓ when done. const done = self.total > 0 and self.cur() >= self.total; - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledForStderr(); const g = self.glyph(); if (use_color) { @@ -480,7 +480,7 @@ pub const ProgressBar = struct { /// fallback over a corrupt frame. fn writeCounterLine(self: *const ProgressBar, w: *std.Io.Writer, cols: u16, value: []const u8) std.Io.Writer.Error!void { const done = self.total > 0 and self.cur() >= self.total; - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledForStderr(); try w.writeAll(" "); @@ -563,7 +563,7 @@ pub const ProgressBar = struct { // Bar const filled: u64 = if (self.total > 0) @min((self.cur() * bw) / self.total, bw) else 0; const empty = bw - filled; - if (color.isColorEnabled()) { + if (color.isColorEnabledForStderr()) { w.writeAll(color.SemanticStyle.info.code()) catch break :body; var i: u64 = 0; while (i < filled) : (i += 1) w.writeAll("\xe2\x94\x81") catch break :body; // ━ @@ -596,7 +596,7 @@ pub const ProgressBar = struct { /// overflow — so the verbose readout gives way before the bar does. `bw` is /// the bar width already drawn; the budget is measured against it. fn writeDetail(self: *const ProgressBar, w: *std.Io.Writer, cols: ?u16, bw: u64) std.Io.Writer.Error!void { - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledForStderr(); const dim_code = if (use_color) color.SemanticStyle.detail.code() else ""; const reset_code = if (use_color) color.Style.reset.code() else ""; @@ -679,7 +679,7 @@ pub const ProgressBar = struct { // writeLabel already renders the animated spinner as the line glyph. self.writeLabel(&w) catch break :body; - const use_color = color.isColorEnabled(); + const use_color = color.isColorEnabledForStderr(); const dim_code = if (use_color) color.SemanticStyle.detail.code() else ""; const reset_code = if (use_color) color.Style.reset.code() else ""; diff --git a/src/update/notifier.zig b/src/update/notifier.zig index 45b7cf82..4f5a1eae 100644 --- a/src/update/notifier.zig +++ b/src/update/notifier.zig @@ -107,7 +107,7 @@ fn printNotice(latest_tag: []const u8, current_version: []const u8) void { ) catch return; const dim_msg = "Run 'mt version update' to upgrade, or set MALT_NO_VERSION_NOTIFIER=1 to silence this."; - const colorize = color.isColorEnabled(); + const colorize = color.isColorEnabledForStderr(); const emoji = color.isEmojiEnabled(); const notice_prefix: []const u8 = if (emoji) "ⓘ " else "i "; const dim_prefix: []const u8 = if (emoji) "▸ " else "> ";