From 4e494707baf0f2bb59bfdac2731ef04278397278 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Thu, 27 Aug 2026 15:33:29 -0400 Subject: [PATCH] fix(env)!: read every ATGC_ switch one way ATGC_NO_INPUT=false switched non-interactive mode on while ATGC_USE_BOBBIN=false switched the index off; the log paths took `off` and nothing else. One reading now lives in src/env.rs and all of them go through it, so unset, empty, 0, false, no and off are off and anything else is on. CI is read the same way; NO_COLOR keeps its own spec. Change-Id: Ib733dcca81ddaabf08bdc230f4ecb384141d2601 --- src/cmd/pr/read/sources.rs | 38 ++-------- src/env.rs | 108 +++++++++++++++++++++++++++ src/logging/file.rs | 14 +++- src/main.rs | 4 + src/term/noinput.rs | 16 ++-- src/term/say.rs | 2 +- tests/env_switches.rs | 145 +++++++++++++++++++++++++++++++++++++ 7 files changed, 281 insertions(+), 46 deletions(-) create mode 100644 src/env.rs create mode 100644 tests/env_switches.rs diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index 4fbf9fc..5f4cd54 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -216,28 +216,12 @@ impl Source { } /// Whether an opt-in environment variable is switched on. -fn opted_in(var: &str) -> bool { - on_switch(std::env::var(var).ok().as_deref()) -} - -/// Whether a variable's value reads as "on", given what the environment -/// held — `None` for unset. /// -/// Anything but unset, the empty string, `0`, `false` or `no` counts as on. -/// The permissive reading is deliberate: someone exporting -/// `ATGC_USE_BOBBIN=yes` has made their intent unmistakable, and refusing it -/// over a spelling would be pedantry with a stale index as the punishment. -/// The empty string is off because that is what an unset variable looks like -/// after a shell has interpolated it — the same reasoning -/// [`crate::clients::endpoints`] applies to its host overrides. -fn on_switch(value: Option<&str>) -> bool { - match value { - Some(v) => !matches!( - v.trim().to_ascii_lowercase().as_str(), - "" | "0" | "false" | "no" - ), - None => false, - } +/// The permissive reading this module used to spell out for itself now lives +/// in [`crate::env`] and is shared with `ATGC_NO_INPUT`, `ATGC_DEBUG` and the +/// log paths, which had two other readings of the same words between them. +fn opted_in(var: &str) -> bool { + crate::env::switch(var) } impl FromStr for Source { @@ -2465,18 +2449,6 @@ mod tests { ); } - #[test] - fn the_opt_in_switch_reads_anything_but_an_off_value() { - for off in [None, Some(""), Some("0"), Some("false"), Some("no")] { - assert!(!super::on_switch(off), "{off:?} should be off"); - } - // Case and surrounding space are not a way to accidentally opt in. - assert!(!super::on_switch(Some(" FALSE "))); - for on in ["1", "true", "yes", "please"] { - assert!(super::on_switch(Some(on)), "{on:?} should be on"); - } - } - /// Only an index can see a pull this account did not write, which is /// what every "there are no pull requests" message has to know before /// choosing how big a claim to make. diff --git a/src/env.rs b/src/env.rs new file mode 100644 index 0000000..5920d92 --- /dev/null +++ b/src/env.rs @@ -0,0 +1,108 @@ +//! One reading of every boolean atgc takes from the environment. +//! +//! `ATGC_NO_INPUT`, `ATGC_DEBUG`, `ATGC_USE_BOBBIN`, `ATGC_USE_WEB` and the +//! off-switch on each `ATGC_*_LOG` path are all the same shape — a variable +//! whose value is a word meaning yes or no — and they were read three +//! different ways. The `pr` sources module rejected `false` and `no`; the +//! non-interactive gate and the debug switch accepted anything that was not +//! `0`, so `ATGC_NO_INPUT=false` turned non-interactive mode *on*; the log +//! paths accepted `off` and nothing else. Same prefix, same shape, opposite +//! answers for the same word, and which one you got depended on knowing +//! which subsystem read it. +//! +//! The permissive reading wins, and the reason is asymmetric harm. Reading +//! `false` as on is a setting that does the opposite of what it says, and +//! the user has no way to discover it short of the source: they typed a word +//! that means no and got yes. Reading an unrecognised word as on costs +//! somebody who typed `ATGC_DEBUG=nope` a screen of debug output, which is +//! visible in the first second and fixed by unsetting it. Refusing an +//! unrecognised word outright was considered and dropped: these are switches +//! people export in a shell profile, and a hard failure on every subsequent +//! command over a spelling is a worse trade than a switch that is on. +//! +//! [`NO_COLOR`](https://no-color.org/) deliberately does **not** come +//! through here. Its spec says any non-empty value disables color whatever +//! the value is, `NO_COLOR=false` included, and a tool that made an +//! exception of that spelling would be the odd one out on a machine rather +//! than consistent with itself. It is read where it is used, in +//! [`crate::term::say`] and [`crate::term::hyperlink`]. + +/// The words that read as "off", beside unset: the empty string, `0`, +/// `false`, `no` and `off`, in any case and with surrounding whitespace +/// ignored. +/// +/// The empty string is off because that is what an unset variable looks like +/// after a shell has interpolated it — `ATGC_USE_BOBBIN="$MAYBE"` — and the +/// same reasoning [`crate::clients::endpoints`] applies to its host +/// overrides. Trailing whitespace is ignored because it survives a +/// copy-pasted `export` more often than anyone would guess. +const OFF: [&str; 5] = ["", "0", "false", "no", "off"]; + +/// Whether a value means "off". The half of the reading that the `ATGC_*_LOG` +/// paths need, since for them any *other* value is a filename rather than a +/// yes. +pub(crate) fn is_off(value: &str) -> bool { + OFF.contains(&value.trim().to_ascii_lowercase().as_str()) +} + +/// Whether the environment variable `name` is switched on. +/// +/// A value that is not valid UTF-8 counts as on: it is set, it is not empty, +/// and it cannot be any of the words in [`OFF`], so the only reading left is +/// the one the user's typing supports. +pub(crate) fn switch(name: &str) -> bool { + on(std::env::var_os(name) + .map(|v| v.to_string_lossy().into_owned()) + .as_deref()) +} + +/// The decision itself, kept pure and taking the value as an argument so the +/// whole table can be asserted without setting a variable that every other +/// test in the process would then see. See [`crate::docs::testing`]. +fn on(value: Option<&str>) -> bool { + value.is_some_and(|v| !is_off(v)) +} + +#[cfg(test)] +mod tests { + use super::{is_off, on}; + + /// The whole table, in one place, because the bug this module exists for + /// was two readings that agreed on `1` and `0` and disagreed on + /// everything a person would actually type. An unset variable and every + /// spelling of no are off; every other value, including a misspelling + /// and a value that is not a word at all, is on. + #[test] + fn every_spelling_of_no_is_off_and_everything_else_is_on() { + for off in [ + "", " ", "0", "false", "FALSE", "False", "no", "NO", "off", "OFF", " false ", "\tno\n", + ] { + assert!(!on(Some(off)), "{off:?} should be off"); + assert!(is_off(off), "{off:?} should be off"); + } + for switched_on in [ + "1", + "true", + "TRUE", + "yes", + "on", + "2", + "-1", + "nope", + "falsey", + "/tmp/atgc.jsonl", + ] { + assert!(on(Some(switched_on)), "{switched_on:?} should be on"); + assert!(!is_off(switched_on), "{switched_on:?} should be on"); + } + } + + /// Unset is off, and is the only input that is not a string. The log + /// paths distinguish it from every value — unset means "the default + /// path", `off` means "no file" — which is why [`is_off`] takes a `&str` + /// and cannot answer this question. + #[test] + fn an_unset_variable_is_off() { + assert!(!on(None)); + } +} diff --git a/src/logging/file.rs b/src/logging/file.rs index 1e8cf9d..429aae3 100644 --- a/src/logging/file.rs +++ b/src/logging/file.rs @@ -310,14 +310,20 @@ impl Log { /// The configured path, or `None` when this log is switched off. /// - /// The environment variable names an explicit file; `0`, `off` or an - /// empty value disables it. Same `ATGC_` convention as `ATGC_DEBUG` and - /// `ATGC_ACCOUNT`. + /// The environment variable names an explicit file; any value meaning + /// "off" — `0`, `off`, `false`, `no` or empty — disables it. That list is + /// [`crate::env::is_off`], the one reading every boolean atgc takes from + /// the environment, so the word that switches this log off is the word + /// that switches `ATGC_USE_BOBBIN` off. + /// + /// Unset is not the same as off, which is why this reads the value rather + /// than calling [`crate::env::switch`]: unset means the default path + /// under the config directory, and off means no file at all. pub fn resolve_path(&self) -> Option { match std::env::var(self.env) { Ok(v) => { let v = v.trim(); - if v.is_empty() || v == "0" || v.eq_ignore_ascii_case("off") { + if crate::env::is_off(v) { None } else { Some(PathBuf::from(v)) diff --git a/src/main.rs b/src/main.rs index f91fc1f..c79c546 100644 --- a/src/main.rs +++ b/src/main.rs @@ -11,6 +11,10 @@ mod config; // docs/, so `cargo docs` renders prose and generated reference as one thing // and a link from prose into code is checked by the build. mod docs; +// One reading of every boolean atgc takes from the environment, so that +// `ATGC_NO_INPUT=false` and `ATGC_USE_BOBBIN=false` cannot mean opposite +// things. +mod env; // What atgc's exit status means, and how a failure comes to carry one. mod exit; mod help; diff --git a/src/term/noinput.rs b/src/term/noinput.rs index 38f6b2f..8683664 100644 --- a/src/term/noinput.rs +++ b/src/term/noinput.rs @@ -16,6 +16,12 @@ //! running" variables are deliberately ignored: agents run both with and //! without terminals, and behavior that varies by which product spawned //! the process is behavior nobody can debug. +//! +//! Both variables are read through [`crate::env::switch`], which is the one +//! reading every boolean atgc takes from the environment. `CI=false` is +//! therefore *not* CI, which is the only reading of that spelling anybody +//! means; this module used to treat it as being in CI because the variable +//! was set at all. use std::io::IsTerminal; use std::sync::OnceLock; @@ -25,8 +31,8 @@ static WHY: OnceLock> = OnceLock::new(); pub fn init(flag: bool) { let why = decide( flag, - env_truthy("ATGC_NO_INPUT"), - env_truthy("CI"), + crate::env::switch("ATGC_NO_INPUT"), + crate::env::switch("CI"), std::io::stdin().is_terminal(), ); let _ = WHY.set(why); @@ -35,12 +41,6 @@ pub fn init(flag: bool) { } } -/// The debug.rs reading of a boolean env var: set, and neither empty nor -/// "0". CI platforms disagree on `true` vs `1`; they agree on being set. -fn env_truthy(name: &str) -> bool { - std::env::var_os(name).is_some_and(|v| v != "0" && !v.is_empty()) -} - /// The one decision, kept pure so the tests can hold it still. Any single /// signal is enough; the precedence order only chooses which *reason* an /// error message repeats back, so the most explicit signal — the thing the diff --git a/src/term/say.rs b/src/term/say.rs index 052bc4e..5542da4 100644 --- a/src/term/say.rs +++ b/src/term/say.rs @@ -220,7 +220,7 @@ static PAINT: OnceLock = OnceLock::new(); /// `ATGC_LOG=off` means off, including about `ATGC_LOG`. pub fn init(quiet: u8, debug: bool) { let env = std::env::var("ATGC_LOG").ok(); - let debug = debug || std::env::var_os("ATGC_DEBUG").is_some_and(|v| v != "0" && !v.is_empty()); + let debug = debug || crate::env::switch("ATGC_DEBUG"); let (filter, complaints) = decide(quiet, debug, env.as_deref()); let _ = FILTER.set(filter); let _ = PAINT.set(crate::term::hyperlink::escapes_wanted( diff --git a/tests/env_switches.rs b/tests/env_switches.rs new file mode 100644 index 0000000..dc9d7c4 --- /dev/null +++ b/tests/env_switches.rs @@ -0,0 +1,145 @@ +//! One reading of the `ATGC_*` switches, observed through the real binary. +//! +//! `src/env.rs`'s own tests pin the table, but the table is only worth +//! anything if every switch actually goes through it. That wiring cannot be +//! asserted from inside a `cargo test` binary: `std::env::set_var` is unsafe +//! and this crate forbids unsafe, so a unit test cannot set `ATGC_NO_INPUT` +//! and watch what `main` does with it. A child process can, because +//! `Command::env` is neither — the same reason `tests/exit_status.rs` spawns. +//! +//! Hermetic throughout. `about` and `logs git` read no socket, and `HOME` is +//! moved to a temp directory or a path that does not exist. + +use std::path::Path; +use std::process::{Command, Output, Stdio}; + +use tempfile::TempDir; + +/// `atgc`, with stdin closed so that "is anybody there" has a settled answer +/// on every machine — a terminal under `cargo test` on a developer's laptop +/// and a pipe on a runner would otherwise give this suite two different +/// baselines. +fn atgc(home: &Path, env: &[(&str, &str)], args: &[&str]) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_atgc")); + cmd.args(args) + .env("HOME", home) + .env_remove("XDG_CONFIG_HOME") + .env_remove("ATGC_NO_INPUT") + .env_remove("ATGC_DEBUG") + .env_remove("ATGC_LOG") + .env_remove("ATGC_GIT_LOG") + .env_remove("CI") + .env("NO_COLOR", "1") + .stdin(Stdio::null()); + for (k, v) in env { + cmd.env(k, v); + } + cmd.output().expect("spawn atgc") +} + +fn stderr(out: &Output) -> String { + String::from_utf8_lossy(&out.stderr).into_owned() +} + +fn nowhere() -> &'static Path { + Path::new("/nonexistent-atgc-test-home") +} + +/// `ATGC_NO_INPUT=false` leaves atgc interactive. +/// +/// The failure this pins is the one that made this module: the gate used to +/// read "set and not `0`", so a user who wrote the word for no switched +/// non-interactive mode *on*. `--debug` names the reason it settled on, and +/// with stdin closed the honest reason is stdin, not the variable. +#[test] +fn a_no_input_of_false_is_not_a_no_input() { + let out = atgc( + nowhere(), + &[("ATGC_DEBUG", "1"), ("ATGC_NO_INPUT", "false")], + &["about"], + ); + assert!( + stderr(&out).contains("non-interactive: stdin is not a terminal"), + "ATGC_NO_INPUT=false was read as a yes: {}", + stderr(&out) + ); +} + +/// And the spelling that means yes still does. +#[test] +fn a_no_input_of_one_is_a_no_input() { + let out = atgc( + nowhere(), + &[("ATGC_DEBUG", "1"), ("ATGC_NO_INPUT", "1")], + &["about"], + ); + assert!( + stderr(&out).contains("non-interactive: ATGC_NO_INPUT is set"), + "{}", + stderr(&out) + ); +} + +/// `CI=false` is not CI. +/// +/// The same reading, on the one variable here that is not atgc's own. A +/// runner that exports `CI=true` is unaffected; somebody who exports +/// `CI=false` to say "not in CI" used to be told they were. +#[test] +fn a_ci_of_false_is_not_ci() { + let out = atgc( + nowhere(), + &[("ATGC_DEBUG", "1"), ("CI", "false")], + &["about"], + ); + let err = stderr(&out); + assert!( + err.contains("non-interactive: stdin is not a terminal"), + "CI=false was read as a yes: {err}" + ); + assert!(!err.contains("CI is set"), "{err}"); +} + +/// `ATGC_DEBUG=false` prints nothing, and `ATGC_DEBUG=1` prints something. +/// +/// The debug switch shared the old permissive reading, so this is the same +/// bug in the variable whose symptom is loudest: a screen of wire traffic +/// from a variable set to the word for no. +#[test] +fn the_debug_switch_reads_false_as_off() { + let off = atgc(nowhere(), &[("ATGC_DEBUG", "false")], &["about"]); + assert!( + !stderr(&off).contains("[debug]"), + "ATGC_DEBUG=false printed debug output: {}", + stderr(&off) + ); + let on = atgc(nowhere(), &[("ATGC_DEBUG", "1")], &["about"]); + assert!( + stderr(&on).contains("[debug]"), + "ATGC_DEBUG=1 printed none: {}", + stderr(&on) + ); +} + +/// A log path takes the same words for "off" as every other switch. +/// +/// `off` and `0` always worked; `false` and `no` used to be read as +/// *filenames*, so `ATGC_GIT_LOG=false` quietly wrote the git log into a +/// file called `false` in the working directory. `logs git` says out loud +/// when writing is disabled, which is the observable end of it. +#[test] +fn a_log_path_of_false_switches_the_log_off() { + let home = TempDir::new().expect("a temp home"); + for word in ["off", "0", "false", "no", ""] { + let out = atgc(home.path(), &[("ATGC_GIT_LOG", word)], &["logs", "git"]); + assert!( + stderr(&out).contains("writing is disabled by ATGC_GIT_LOG"), + "ATGC_GIT_LOG={word:?} did not switch the log off: {}", + stderr(&out) + ); + } + assert!( + !home.path().join("false").exists(), + "a log was written to a file named for the word that switches it off" + ); +} -- knotmirror 0.0.1