diff --git a/Cargo.lock b/Cargo.lock index 03410b83..a6b5a9f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1831,7 +1831,6 @@ dependencies = [ "serde", "serde_json", "serde_yaml_ng", - "temp-env", "tempfile", "tokio", "url", @@ -1848,7 +1847,6 @@ dependencies = [ "reqwest", "serde", "serde_json", - "temp-env", "tempfile", "thiserror", "tokio", diff --git a/crates/mergify-config/Cargo.toml b/crates/mergify-config/Cargo.toml index b83e950b..b8941d5d 100644 --- a/crates/mergify-config/Cargo.toml +++ b/crates/mergify-config/Cargo.toml @@ -20,7 +20,6 @@ url = { workspace = true } [dev-dependencies] mergify-test-support = { path = "../mergify-test-support" } tempfile = { workspace = true } -temp-env = { workspace = true } tokio = { workspace = true } wiremock = { workspace = true } diff --git a/crates/mergify-core/Cargo.toml b/crates/mergify-core/Cargo.toml index ceb96d0d..e9898ae4 100644 --- a/crates/mergify-core/Cargo.toml +++ b/crates/mergify-core/Cargo.toml @@ -29,7 +29,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] -temp-env = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-core/src/auth.rs b/crates/mergify-core/src/auth.rs index 10329635..1c494cb5 100644 --- a/crates/mergify-core/src/auth.rs +++ b/crates/mergify-core/src/auth.rs @@ -127,7 +127,8 @@ pub fn resolve_mergify_token( audience: Audience, ) -> Result { let store = CredentialStore::discover(); - let resolved = resolve_mergify_token_with(explicit, api_url, audience, &store)?; + let resolved = + resolve_mergify_token_with(explicit, api_url, audience, &store, || gh_auth_token().ok())?; if let Some(notice) = deprecation_notice(resolved.source, audience) { warn_once(¬ice); } @@ -149,6 +150,7 @@ fn resolve_mergify_token_with( api_url: &Url, audience: Audience, store: &CredentialStore, + gh_token: impl FnOnce() -> Option, ) -> Result { if let Some(value) = explicit.filter(|s| !s.is_empty()) { return Ok(ResolvedToken { @@ -185,9 +187,7 @@ fn resolve_mergify_token_with( source: TokenSource::GitHubTokenEnv, }); } - if let Ok(token) = gh_auth_token() - && !token.is_empty() - { + if let Some(token) = gh_token().filter(|t| !t.is_empty()) { return Ok(ResolvedToken { token, source: TokenSource::GhCli, @@ -258,6 +258,22 @@ fn warn_once(message: &str) { /// next source tried. If nothing is left, the failure names the /// sources it skipped rather than the ones the user should have set. pub fn resolve_github_token(explicit: Option<&str>) -> Result { + resolve_github_token_with(explicit, || gh_auth_token().ok()) +} + +/// [`resolve_github_token`] with its `gh auth token` leg supplied by +/// the caller. +/// +/// The tests drive it with a closure rather than putting a fake `gh` +/// on `PATH`: `PATH` only reaches a child process by mutating this +/// one's environment, which nothing in this workspace does. It also +/// makes the cases that matter here — `gh` absent, `gh` echoing the +/// `mut_` value it was handed — expressible without a shell script, +/// a temp directory, or a `cfg(unix)` gate. +fn resolve_github_token_with( + explicit: Option<&str>, + gh_token: impl FnOnce() -> Option, +) -> Result { if let Some(value) = explicit.filter(|s| !s.is_empty()) { return Ok(value.to_string()); } @@ -294,9 +310,7 @@ pub fn resolve_github_token(explicit: Option<&str>) -> Result // the value out of the request: a GitHub Actions job exporting a // `mut_` token gets it handed straight back here. Check what // came out, not where it came from. - if let Ok(token) = gh_auth_token() - && !token.is_empty() - { + if let Some(token) = gh_token().filter(|t| !t.is_empty()) { if token.starts_with(MERGIFY_USER_TOKEN_PREFIX) { tracing::debug!( "`gh auth token` returned a Mergify user token, which GitHub cannot accept; \ @@ -457,6 +471,7 @@ fn parse_slug(url: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use crate::env; /// A store that cannot reach the developer's real keychain, /// optionally holding a credential for [`api_url`]. @@ -481,20 +496,36 @@ mod tests { Url::parse("https://api.mergify.com").unwrap() } - /// The chain, with no keychain anywhere near it. `None` for the - /// store means "this machine has no credential store at all". + /// The chain, with no keychain anywhere near it and no `gh`. + /// + /// A machine with an authenticated `gh` used to answer the last + /// step and make these tests assert on the wrong thing, which is + /// why so many of them pointed `PATH` at a nonexistent + /// directory. Injecting the step says the same thing without + /// needing the process environment to change. fn resolve( explicit: Option<&str>, audience: Audience, store: &CredentialStore, ) -> Result { - resolve_mergify_token_with(explicit, &api_url(), audience, store) + resolve_with_gh(explicit, audience, store, || None) + } + + /// [`resolve`] for a case that is about what `gh auth token` + /// answered. + fn resolve_with_gh( + explicit: Option<&str>, + audience: Audience, + store: &CredentialStore, + gh_token: impl FnOnce() -> Option, + ) -> Result { + resolve_mergify_token_with(explicit, &api_url(), audience, store, gh_token) } #[test] fn resolve_mergify_token_prefers_explicit_over_everything() { let (_dir, store) = store_with(Some("stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -512,7 +543,7 @@ mod tests { #[test] fn resolve_mergify_token_prefers_the_mergify_env_var_over_the_store() { let (_dir, store) = store_with(Some("stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -531,7 +562,7 @@ mod tests { #[test] fn the_stored_credential_beats_github_token() { let (_dir, store) = store_with(Some("mut_stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -550,7 +581,7 @@ mod tests { #[test] fn the_ci_audience_never_uses_the_stored_credential() { let (_dir, store) = store_with(Some("mut_stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -566,7 +597,7 @@ mod tests { #[test] fn resolve_mergify_token_falls_back_to_github_env_when_nothing_is_stored() { let (_dir, store) = store_with(None); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -584,45 +615,26 @@ mod tests { #[test] fn a_credential_stored_for_another_deployment_is_not_used() { let (_dir, store) = store_with(Some("mut_stored")); - // `PATH` too: without it a developer machine with an - // authenticated `gh` answers the last step of the chain and - // the test asserts on the wrong thing. - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let other = Url::parse("https://mergify.internal.example/api").unwrap(); - let err = - resolve_mergify_token_with(None, &other, Audience::User, &store).unwrap_err(); - assert!( - err.to_string().contains("no Mergify credential"), - "got {err}" - ); - }, - ); + env::testing::with_no_vars(|| { + let other = Url::parse("https://mergify.internal.example/api").unwrap(); + let err = resolve_mergify_token_with(None, &other, Audience::User, &store, || None) + .unwrap_err(); + assert!( + err.to_string().contains("no Mergify credential"), + "got {err}" + ); + }); } #[test] fn resolve_mergify_token_error_names_the_command_that_fixes_it() { - // Forcing PATH to a directory with no `gh` keeps the test - // hermetic on machines that do have the GitHub CLI installed. let (_dir, store) = store_with(None); - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let err = resolve(None, Audience::User, &store).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("mergify auth login"), "got {msg:?}"); - assert!(msg.contains("MERGIFY_TOKEN"), "got {msg:?}"); - }, - ); + env::testing::with_no_vars(|| { + let err = resolve(None, Audience::User, &store).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("mergify auth login"), "got {msg:?}"); + assert!(msg.contains("MERGIFY_TOKEN"), "got {msg:?}"); + }); } // A `ci` command cannot be fixed by `auth login`; the message it @@ -630,19 +642,12 @@ mod tests { #[test] fn the_ci_audience_is_not_told_to_run_auth_login() { let (_dir, store) = store_with(None); - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let err = resolve(None, Audience::ApplicationKey, &store).unwrap_err(); - let msg = err.to_string(); - assert!(!msg.contains("auth login"), "got {msg:?}"); - assert!(msg.contains("application key"), "got {msg:?}"); - }, - ); + env::testing::with_no_vars(|| { + let err = resolve(None, Audience::ApplicationKey, &store).unwrap_err(); + let msg = err.to_string(); + assert!(!msg.contains("auth login"), "got {msg:?}"); + assert!(msg.contains("application key"), "got {msg:?}"); + }); } #[test] @@ -689,7 +694,7 @@ mod tests { // wrapper itself must at least be wired to the chain. #[test] fn the_public_resolver_answers_the_chain() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", None), @@ -705,10 +710,10 @@ mod tests { #[test] fn overriding_env_var_reports_what_outranks_the_stored_credential() { - temp_env::with_var("MERGIFY_TOKEN", Some("env-mergify"), || { + env::testing::with_var("MERGIFY_TOKEN", Some("env-mergify"), || { assert_eq!(overriding_env_var(), Some("MERGIFY_TOKEN")); }); - temp_env::with_var("MERGIFY_TOKEN", None::<&str>, || { + env::testing::with_var("MERGIFY_TOKEN", None::<&str>, || { assert_eq!(overriding_env_var(), None); }); } @@ -719,13 +724,16 @@ mod tests { // about why. #[test] fn resolve_github_token_skips_a_mergify_user_token() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-github"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-github" + ); }, ); } @@ -735,14 +743,13 @@ mod tests { // set, for a reason they cannot see. #[test] fn resolve_github_token_says_why_it_skipped_the_mergify_token() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let err = resolve_github_token(None).unwrap_err(); + let err = resolve_github_token_with(None, || None).unwrap_err(); let message = err.to_string(); assert!( message.contains("GitHub does not accept"), @@ -764,14 +771,15 @@ mod tests { // the broken one. #[test] fn resolve_github_token_names_the_variable_it_actually_skipped() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("mut_from_auth_login")), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + let message = resolve_github_token_with(None, || None) + .unwrap_err() + .to_string(); assert!(message.starts_with("GITHUB_TOKEN holds"), "got {message:?}"); assert!( !message.contains("MERGIFY_TOKEN"), @@ -787,14 +795,15 @@ mod tests { #[test] fn resolve_github_token_names_both_variables_when_both_were_skipped() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_one")), ("GITHUB_TOKEN", Some("mut_two")), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + let message = resolve_github_token_with(None, || None) + .unwrap_err() + .to_string(); assert!( message.starts_with("MERGIFY_TOKEN and GITHUB_TOKEN hold a"), "got {message:?}", @@ -803,35 +812,24 @@ mod tests { ); } - // A `gh` on `PATH` answering `auth token` with whatever the body - // prints. The real one echoes `$GITHUB_TOKEN` when that is set, - // which is the case worth reproducing. - #[cfg(unix)] - fn fake_gh(body: &str) -> tempfile::TempDir { - use std::os::unix::fs::PermissionsExt; - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("gh"); - std::fs::write(&path, format!("#!/bin/sh\n{body}\n")).unwrap(); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); - dir - } - // Skipping the variable is not enough on its own: `gh auth token` // prints `$GITHUB_TOKEN`, so a GitHub Actions job exporting a // `mut_` value gets the same token back through the fallback and // sends it to GitHub anyway. - #[cfg(unix)] #[test] fn resolve_github_token_does_not_let_gh_hand_back_the_skipped_variable() { - let gh = fake_gh("printf '%s' \"$GITHUB_TOKEN\""); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("mut_from_auth_login")), - ("PATH", Some(gh.path().to_str().unwrap())), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + // What the real `gh auth token` prints when + // `GITHUB_TOKEN` is set: the variable, straight back. + let message = + resolve_github_token_with(None, || Some("mut_from_auth_login".to_string())) + .unwrap_err() + .to_string(); assert!( message.starts_with("GITHUB_TOKEN and `gh auth token` hold a"), "got {message:?}", @@ -848,19 +846,17 @@ mod tests { // The filter is on the value, not on the source: a GitHub token // from `gh` is still the answer when the variables hold nothing // usable. - #[cfg(unix)] #[test] fn resolve_github_token_still_takes_a_real_gh_token() { - let gh = fake_gh("printf 'ghp_from_gh_auth_login'"); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", None), - ("PATH", Some(gh.path().to_str().unwrap())), ], || { assert_eq!( - resolve_github_token(None).unwrap(), + resolve_github_token_with(None, || Some("ghp_from_gh_auth_login".to_string())) + .unwrap(), "ghp_from_gh_auth_login" ); }, @@ -872,7 +868,7 @@ mod tests { // the more surprising failure. #[test] fn resolve_github_token_still_honours_an_explicit_mergify_token() { - temp_env::with_var("GITHUB_TOKEN", Some("env-github"), || { + env::testing::with_var("GITHUB_TOKEN", Some("env-github"), || { assert_eq!( resolve_github_token(Some("mut_explicit")).unwrap(), "mut_explicit", @@ -887,7 +883,7 @@ mod tests { // if that divergence ever reaches the GitHub side. #[test] fn resolve_github_token_prefers_explicit_over_env() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -903,33 +899,39 @@ mod tests { #[test] fn resolve_github_token_falls_back_to_mergify_env() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-mergify"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-mergify" + ); }, ); } #[test] fn resolve_github_token_falls_back_to_github_env_when_mergify_unset() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-github"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-github" + ); }, ); } #[test] fn resolve_api_url_default() { - temp_env::with_var("MERGIFY_API_URL", None::<&str>, || { + env::testing::with_var("MERGIFY_API_URL", None::<&str>, || { let url = resolve_api_url(None).unwrap(); assert_eq!(url.as_str(), "https://api.mergify.com/"); }); @@ -937,7 +939,7 @@ mod tests { #[test] fn resolve_api_url_prefers_explicit() { - temp_env::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { + env::testing::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { let url = resolve_api_url(Some("https://explicit.example/")).unwrap(); assert_eq!(url.as_str(), "https://explicit.example/"); }); @@ -945,7 +947,7 @@ mod tests { #[test] fn resolve_api_url_uses_env_var_when_explicit_empty() { - temp_env::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { + env::testing::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { let url = resolve_api_url(None).unwrap(); assert_eq!(url.as_str(), "https://from-env.example/"); }); @@ -953,7 +955,7 @@ mod tests { #[test] fn resolve_api_url_rejects_garbage() { - temp_env::with_var("MERGIFY_API_URL", None::<&str>, || { + env::testing::with_var("MERGIFY_API_URL", None::<&str>, || { let err = resolve_api_url(Some("not a url")).unwrap_err(); assert!(err.to_string().contains("invalid --api-url")); }); @@ -961,7 +963,7 @@ mod tests { #[test] fn resolve_repository_prefers_explicit() { - temp_env::with_var("GITHUB_REPOSITORY", Some("owner-from-env/repo"), || { + env::testing::with_var("GITHUB_REPOSITORY", Some("owner-from-env/repo"), || { assert_eq!( resolve_repository(Some("explicit/repo")).unwrap(), "explicit/repo", @@ -971,7 +973,7 @@ mod tests { #[test] fn resolve_repository_falls_back_to_env() { - temp_env::with_var("GITHUB_REPOSITORY", Some("owner/repo"), || { + env::testing::with_var("GITHUB_REPOSITORY", Some("owner/repo"), || { assert_eq!(resolve_repository(None).unwrap(), "owner/repo"); }); } diff --git a/crates/mergify-core/src/env.rs b/crates/mergify-core/src/env.rs index d651e383..1c837d44 100644 --- a/crates/mergify-core/src/env.rs +++ b/crates/mergify-core/src/env.rs @@ -1,12 +1,76 @@ -//! Environment-variable helpers shared across commands. +//! The one place *this workspace* reads the process environment, and +//! the only way a test changes what it says. +//! +//! Not the only place the process reads it: `tracing-subscriber` +//! reads `RUST_LOG`, `dirs` reads `HOME` / `XDG_CONFIG_HOME` / +//! `APPDATA` under `credentials_file()`, and `reqwest` reads +//! `HTTP_PROXY` / `NO_PROXY` when it builds a client. Those are +//! inside dependencies, so neither the lint nor a test overlay +//! reaches them: overlaying `HOME` compiles, runs, and changes +//! nothing. +//! +//! # Why this is not `std::env` +//! +//! Tests used to set up environment-dependent behaviour with +//! `temp_env`, which mutates the process environment for the duration +//! of a closure. `setenv` is `unsafe` since Rust 1.80 for a concrete +//! reason: on Unix it can reallocate `environ` while another thread is +//! inside `getenv`, and that is a use-after-free, not a race whose +//! worst case is a wrong value. A libtest binary runs its tests on +//! many threads at once, and the concurrent reader does not have to be +//! ours — `std::env::temp_dir` (every `tempfile::tempdir()`), +//! `Command::spawn` building a child's environment, and any dependency +//! that consults the environment are all `getenv` on another thread. +//! `temp_env`'s own lock cannot serialise against any of them; it only +//! serialises against other `temp_env` calls. +//! +//! So nothing in this workspace mutates the process environment, in +//! tests or anywhere else. Production reads it through [`var`], +//! [`var_os`] and [`var_non_empty`]; a test replaces what those return +//! with [`testing::with_vars`], which touches no global state at all. +//! +//! # What a test overlay does +//! +//! [`testing::with_vars`] installs a **replacement** environment on the +//! current thread. While it is installed, the process environment is +//! invisible: a name the test did not list reads as unset. That is the +//! point — it is what makes a test that reads `GITHUB_ACTIONS` behave +//! the same on a laptop and on a GitHub Actions runner, without a +//! hand-maintained list of variables to scrub first. +//! +//! The overlay covers *our* reads, and only ours. A dependency that +//! consults the environment (`dirs`, `keyring`, `reqwest`'s proxy +//! variables) and any process we spawn both see the real one, and +//! they see it silently — an overlay that sets `HOME` or `PATH` +//! changes nothing for them. Setting a variable for a child is a +//! different job with a different tool: `Command::env`, which touches +//! no shared state and is not affected by any of this. +//! +//! Keys match exactly. On Windows the real environment does not — +//! `std::env::var_os("PATH")` finds a `Path` — so an overlay on +//! `Path` leaves `var("PATH")` reading the host's. Nothing here reads +//! a name in a case other than the one it writes, so no call site can +//! tell the difference, and matching the platform would mean a +//! Windows-only normalisation that this suite never runs: the unit +//! tests are a Linux and macOS job, Windows only builds the binary. +//! +//! The overlay is thread-local, and it is compiled into release builds +//! rather than gated behind `cfg(test)`. `cfg(test)` cannot do the job: +//! it is false whenever this crate is compiled as a dependency, so +//! every consumer crate's tests would read the real environment +//! anyway. `mergify_tui::theme` carries the same note for the same +//! reason. The cost is one thread-local read per environment lookup on +//! a path that runs a handful of times per process. +//! +//! # The empty-string rule //! //! The CLI's resolver pattern (`flag → env → default`) treats the //! empty string as "not set", because callers in the wild — most //! notably the `gha-mergify-ci` GitHub Action — `export VAR=""` //! when no value is available. Inlining -//! `std::env::var(NAME).ok().filter(|s| !s.is_empty())` on every call -//! site looks innocuous but invites the same bug we've now hit -//! twice (monorepo#33423, `MERGIFY_CONFIG_PATH` and +//! `var(NAME).filter(|s| !s.is_empty())` on every call site looks +//! innocuous but invites the same bug we've now hit twice +//! (monorepo#33423, `MERGIFY_CONFIG_PATH` and //! `MERGIFY_TEST_EXIT_CODE`): a contributor adds clap's //! `env = "MERGIFY_FOO"` attribute on a flag instead, and clap's //! parser treats an empty env value as a present-but-empty flag @@ -16,7 +80,72 @@ //! → default` chain. Do **not** wire env vars through clap's //! `env = ...` attribute for any of the `MERGIFY_*` namespace. -use std::env; +use std::cell::Cell; +use std::cell::RefCell; +use std::collections::HashMap; +use std::ffi::OsStr; +use std::ffi::OsString; + +thread_local! { + /// The replacement environment installed by + /// [`testing::with_vars`], or `None` when the real process + /// environment is in effect. `Some(map)` is exhaustive: names + /// absent from `map` read as unset. + static OVERLAY: RefCell>> = const { RefCell::new(None) }; + + /// How many overlay scopes are open on this thread. Each scope + /// records the value it pushed and checks it again on the way + /// out, so two overlapping scopes are a panic rather than a + /// wrong answer — see `testing::Restore`'s `Drop`. + static DEPTH: Cell = const { Cell::new(0) }; +} + +/// Read an environment variable as an `OsString`. +/// +/// Returns the test overlay's value when one is installed on this +/// thread (see the module docs), otherwise the process environment's. +#[must_use] +pub fn var_os(name: &str) -> Option { + // `try_with`, not `with`: `OVERLAY` owns a `HashMap` and so + // registers a thread-local destructor, and `with` panics once + // that has run. A `Drop` impl reading a variable while its thread + // tears down would abort the process, which `std::env::var_os` + // never did. No overlay can exist at that point anyway. + OVERLAY + .try_with(|slot| { + slot.borrow().as_ref().map(|map| { + // An installed overlay *is* the environment: a name + // it does not carry reads as unset. + map.get(OsStr::new(name)).cloned() + }) + }) + .unwrap_or(None) + .unwrap_or_else(|| { + // The single sanctioned process-environment read in + // the workspace: every other caller goes through the + // functions above. + #[allow(clippy::disallowed_methods)] + std::env::var_os(name) + }) +} + +/// Read an environment variable as a `String`. +/// +/// A value that is not valid UTF-8 reads as unset, matching +/// `std::env::var(name).ok()`. +#[must_use] +pub fn var(name: &str) -> Option { + var_os(name).and_then(|value| value.into_string().ok()) +} + +/// [`var_non_empty`] for a value that need not be UTF-8 — the editor +/// chain (`GIT_EDITOR` / `VISUAL` / `EDITOR`) hands its value +/// straight to a `Command`, so decoding it would only be to re-encode +/// it. +#[must_use] +pub fn var_os_non_empty(name: &str) -> Option { + var_os(name).filter(|value| !value.is_empty()) +} /// Read an environment variable and return its value if it's set /// to a non-empty string. Unset or empty both collapse to `None`. @@ -26,7 +155,228 @@ use std::env; /// command. See the module doc for the empty-as-unset rationale. #[must_use] pub fn var_non_empty(name: &str) -> Option { - env::var(name).ok().filter(|s| !s.is_empty()) + var(name).filter(|value| !value.is_empty()) +} + +/// Give a test environment-dependent behaviour without mutating the +/// process environment. +/// +/// Compiled unconditionally, not behind `cfg(test)` or a feature — +/// see the module docs for why that is the only thing that works. +pub mod testing { + use std::collections::HashMap; + use std::ffi::OsStr; + use std::ffi::OsString; + use std::future::Future; + use std::marker::PhantomData; + use std::rc::Rc; + + use super::DEPTH; + use super::OVERLAY; + + /// Restores the enclosing overlay (usually `None`) when the scope + /// ends, including while a panic unwinds. + /// + /// Deliberately `!Send` (the `Rc` marker). The overlay lives on + /// the thread that installed it, so a guard that reached another + /// thread would restore the wrong thread's slot and strand the + /// overlay on the original one — permanently, since nothing else + /// clears it. Making the guard unsendable makes the future + /// returned by [`with_vars_async`] unsendable too, so + /// `tokio::spawn`-ing it is a compile error rather than a silent + /// leak. + struct Restore { + previous: Option>, + /// The value this scope pushed onto `DEPTH`, checked again on + /// the way out. + depth: u64, + _unsend: PhantomData>, + } + + impl Restore { + /// Make `map` this thread's environment until the returned + /// guard drops. + fn install_map(map: HashMap) -> Self { + let depth = DEPTH.with(|open| { + let pushed = open.get() + 1; + open.set(pushed); + pushed + }); + Self { + previous: OVERLAY.with_borrow_mut(|slot| slot.replace(map)), + depth, + _unsend: PhantomData, + } + } + } + + impl Drop for Restore { + fn drop(&mut self) { + // Ignore a thread already past its destructors: there is + // nothing left to restore, and panicking in a `Drop` + // during unwinding aborts. + let _ = OVERLAY.try_with(|slot| *slot.borrow_mut() = self.previous.take()); + let Ok(innermost) = DEPTH.try_with(|open| { + let innermost = open.get(); + open.set(self.depth - 1); + innermost + }) else { + return; + }; + // Never report while already unwinding: a panic inside a + // `Drop` during a panic aborts the process. + if std::thread::panicking() { + return; + } + // Scopes have to close in the order they opened, and the + // sync helpers cannot do otherwise: the guard is a local. + // `join!` of two `with_vars_async` on one thread can — + // each future holds its guard across an await, so the + // first to finish restores the state saved before the + // second, and the second resumes without its variables. + // Restoring `previous` above has already done that damage + // by the time we get here; what this turns into a failure + // is the silence. + assert_eq!( + innermost, self.depth, + "environment overlay scopes closed out of order: two \ + overlays overlap on this thread. Wrap the `join!`, \ + do not wrap each arm." + ); + } + } + + fn install(vars: I) -> Restore + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + { + // Nesting layers onto the enclosing overlay rather than + // replacing it, so an inner `with_var` reads like the + // process-environment version it replaces. + let mut map = OVERLAY.with_borrow(|slot| slot.clone().unwrap_or_default()); + for (name, value) in vars { + match value { + Some(value) => { + map.insert(name.as_ref().to_os_string(), value.as_ref().to_os_string()); + } + None => { + map.remove(name.as_ref()); + } + } + } + Restore::install_map(map) + } + + /// Run `f` with `vars` as the entire environment, as far as + /// [`super::var`] and friends are concerned. + /// + /// A `None` value means "unset", which only matters when nesting: + /// at the outermost level every name not listed is already unset. + pub fn with_vars(vars: I, f: F) -> R + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + F: FnOnce() -> R, + { + let _restore = install(vars); + f() + } + + /// The empty environment: every name reads as unset. + /// + /// What a case whose whole point is "none of this is configured" + /// wants. Unlike [`with_vars`], this does *not* inherit an + /// enclosing overlay — an empty `with_vars` would be a no-op + /// inside one, which is the opposite of what the name promises. + pub fn with_no_vars(f: F) -> R + where + F: FnOnce() -> R, + { + let _restore = Restore::install_map(HashMap::new()); + f() + } + + /// [`with_no_vars`] for an `async` body. + pub async fn with_no_vars_async(future: Fut) -> Fut::Output + where + Fut: Future, + { + let _restore = Restore::install_map(HashMap::new()); + future.await + } + + /// [`with_vars`] for a single variable. + pub fn with_var(name: K, value: Option, f: F) -> R + where + K: AsRef, + V: AsRef, + F: FnOnce() -> R, + { + with_vars([(name, value)], f) + } + + /// [`with_vars`] for an `async` body. + /// + /// The overlay lives on the thread that polls `future`. The + /// returned future is `!Send`, so `tokio::spawn`ing *it* is a + /// compile error rather than a silent read of the real + /// environment. + /// + /// Work the body hands to another thread is a different matter + /// and is not guarded: `spawn_blocking`, `std::thread::spawn`, + /// and `tokio::spawn` / `JoinSet::spawn` on a `multi_thread` + /// runtime all run where there is no overlay. Every + /// `#[tokio::test]` here is the default `current_thread` flavor, + /// where a spawned task stays on this thread and does see it; + /// adding `flavor = "multi_thread"` to a test that spawns under + /// an overlay would silently start reading the host's + /// environment. Such a test needs the value passed in. + /// + /// # Panics + /// + /// Guards must also nest. Two overlay scopes overlapping on one + /// thread (`join!` of two `with_vars_async`) would see each + /// other's variables and restore in the wrong order, so the + /// second scope to close panics instead. Wrap the `join!`, don't + /// wrap each arm. + pub async fn with_vars_async(vars: I, future: Fut) -> Fut::Output + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + Fut: Future, + { + let _restore = install(vars); + future.await + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + #[should_panic(expected = "closed out of order")] + fn overlapping_scopes_panic_instead_of_answering_wrongly() { + // What `join!`-ing two `with_vars_async` arms does, with + // the awaits taken out: the outer guard drops first. + let outer = install([("MERGIFY_TEST_A", Some("outer"))]); + let inner = install([("MERGIFY_TEST_B", Some("inner"))]); + drop(outer); + drop(inner); + } + + #[test] + fn nesting_in_order_does_not_panic() { + let outer = install([("MERGIFY_TEST_A", Some("outer"))]); + let inner = install([("MERGIFY_TEST_B", Some("inner"))]); + drop(inner); + drop(outer); + assert_eq!(super::super::var("MERGIFY_TEST_A"), None); + } + } } #[cfg(test)] @@ -35,7 +385,7 @@ mod tests { #[test] fn returns_some_for_non_empty_value() { - let got = temp_env::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || { + let got = testing::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || { var_non_empty("MERGIFY_TEST_HELPER_X") }); assert_eq!(got.as_deref(), Some("hello")); @@ -46,7 +396,7 @@ mod tests { // The whole point of this helper: an empty env var is // treated as if it were not set. Regression-prone enough // that we pin it explicitly. - let got = temp_env::with_var("MERGIFY_TEST_HELPER_Y", Some(""), || { + let got = testing::with_var("MERGIFY_TEST_HELPER_Y", Some(""), || { var_non_empty("MERGIFY_TEST_HELPER_Y") }); assert_eq!(got, None); @@ -54,9 +404,105 @@ mod tests { #[test] fn returns_none_when_unset() { - let got = temp_env::with_var_unset("MERGIFY_TEST_HELPER_Z", || { + let got = testing::with_var("MERGIFY_TEST_HELPER_Z", None::<&str>, || { var_non_empty("MERGIFY_TEST_HELPER_Z") }); assert_eq!(got, None); } + + #[test] + fn var_os_non_empty_treats_empty_as_unset() { + // Same empty-as-unset rule as `var_non_empty`, on the path + // that keeps the value an `OsString`. + testing::with_vars( + [("MERGIFY_TEST_A", Some("x")), ("MERGIFY_TEST_B", Some(""))], + || { + assert_eq!(var_os_non_empty("MERGIFY_TEST_A"), Some("x".into())); + assert_eq!(var_os_non_empty("MERGIFY_TEST_B"), None); + assert_eq!(var_os_non_empty("MERGIFY_TEST_C"), None); + }, + ); + } + + #[test] + fn with_no_vars_is_the_empty_environment() { + assert_eq!(testing::with_no_vars(|| var("PATH")), None); + } + + #[test] + fn with_no_vars_does_not_inherit_an_enclosing_overlay() { + // The trap this exists to avoid: `with_vars` layers onto the + // enclosing overlay, so an *empty* `with_vars` nested inside + // one would keep everything and scrub nothing. + testing::with_var("MERGIFY_TEST_A", Some("outer"), || { + testing::with_no_vars(|| assert_eq!(var("MERGIFY_TEST_A"), None)); + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("outer")); + }); + } + + #[test] + fn overlay_hides_the_process_environment() { + // The property the old `temp_env` scrub lists existed to + // fake: a variable the test did not list is unset, whatever + // the host exports. `PATH` is the likeliest name to be + // exported, but nothing guarantees it — a process started + // with a cleared environment has none — so read the host's + // value rather than requiring one. + let host_path = var("PATH"); + let got = testing::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || var("PATH")); + assert_eq!(got, None, "an installed overlay is the whole environment"); + assert_eq!( + var("PATH"), + host_path, + "outside an overlay the real environment is visible again" + ); + } + + #[test] + fn nested_overlays_layer_and_unwind() { + testing::with_vars( + [ + ("MERGIFY_TEST_A", Some("outer")), + ("MERGIFY_TEST_B", Some("b")), + ], + || { + testing::with_vars( + [("MERGIFY_TEST_A", Some("inner")), ("MERGIFY_TEST_B", None)], + || { + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("inner")); + assert_eq!(var("MERGIFY_TEST_B"), None); + }, + ); + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("outer")); + assert_eq!(var("MERGIFY_TEST_B").as_deref(), Some("b")); + }, + ); + assert_eq!(var("MERGIFY_TEST_A"), None); + } + + #[test] + fn overlay_is_restored_after_a_panic() { + let panicked = std::panic::catch_unwind(|| { + testing::with_var("MERGIFY_TEST_A", Some("x"), || panic!("boom")); + }); + assert!(panicked.is_err()); + // The overlay's own variable, not a host one: `PATH` reads as + // unset both under a leaked overlay and on a host that does + // not export it, so it cannot tell those two apart. + assert_eq!( + var("MERGIFY_TEST_A"), + None, + "the overlay outlived the panic that unwound through it" + ); + } + + #[tokio::test] + async fn overlay_spans_await_points() { + let got = testing::with_vars_async([("MERGIFY_TEST_A", Some("x"))], async { + tokio::task::yield_now().await; + var("MERGIFY_TEST_A") + }) + .await; + assert_eq!(got.as_deref(), Some("x")); + } }