diff --git a/Cargo.lock b/Cargo.lock index dae60f3e..fda3f33a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1914,7 +1914,6 @@ dependencies = [ "regex", "serde", "serde_json", - "temp-env", "tempfile", "terminal_size", "tokio", diff --git a/crates/mergify-stack/Cargo.toml b/crates/mergify-stack/Cargo.toml index 87d56830..487436b1 100644 --- a/crates/mergify-stack/Cargo.toml +++ b/crates/mergify-stack/Cargo.toml @@ -38,7 +38,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] -temp-env = { workspace = true } tokio = { workspace = true } url = { workspace = true } wiremock = { workspace = true } diff --git a/crates/mergify-stack/src/commands/note.rs b/crates/mergify-stack/src/commands/note.rs index 8b496bf4..0a609020 100644 --- a/crates/mergify-stack/src/commands/note.rs +++ b/crates/mergify-stack/src/commands/note.rs @@ -16,6 +16,7 @@ use std::process::Command; use crate::git::{resolve_repo_toplevel, run_git_capture as run_git}; use mergify_core::CliError; +use mergify_core::env; use crate::change_id; use crate::local_commits::{self, STACK_NOTES_REF}; @@ -212,9 +213,9 @@ fn read_note_from_editor() -> Result { // Treat empty env-var values as unset so `GIT_EDITOR=` falls // through to `$VISUAL` / `$EDITOR` / `vi` instead of spawning // an empty command. Matches Python's `or`-chain semantics. - let editor = non_empty_env("GIT_EDITOR") - .or_else(|| non_empty_env("VISUAL")) - .or_else(|| non_empty_env("EDITOR")) + let editor = env::var_os_non_empty("GIT_EDITOR") + .or_else(|| env::var_os_non_empty("VISUAL")) + .or_else(|| env::var_os_non_empty("EDITOR")) .unwrap_or_else(|| OsString::from("vi")); let mut tmp = tempfile::Builder::new() @@ -261,14 +262,6 @@ fn read_note_from_editor() -> Result { Ok(cleaned) } -/// Read an env var, returning `None` for both unset *and* empty. -/// `OsString::is_empty` covers both `KEY` being absent and -/// `KEY=` exporting an empty string (which Python's `or` chain -/// in `_read_note_from_editor` also treats as unset). -fn non_empty_env(name: &str) -> Option { - std::env::var_os(name).filter(|v| !v.is_empty()) -} - #[cfg(unix)] fn invoke_editor(editor: &OsString, path: &str) -> Result { let cmd_line = format!("{} \"$@\"", editor.to_string_lossy()); @@ -454,7 +447,7 @@ mod tests { .unwrap(); set_executable(&editor); - temp_env::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { + env::testing::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { run(Some(dir.path()), None, Action::FromEditor).unwrap(); }); assert_eq!( @@ -479,7 +472,7 @@ mod tests { .unwrap(); set_executable(&editor); - let err = temp_env::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { + let err = env::testing::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { run(Some(dir.path()), None, Action::FromEditor).unwrap_err() }); match err { @@ -500,7 +493,7 @@ mod tests { std::fs::write(&editor, "#!/bin/sh\nprintf 'from VISUAL\\n' > \"$1\"\n").unwrap(); set_executable(&editor); - temp_env::with_vars( + env::testing::with_vars( [ ("GIT_EDITOR", Some(String::new())), ("VISUAL", Some(editor.to_str().unwrap().to_string())), diff --git a/crates/mergify-stack/src/stack_context.rs b/crates/mergify-stack/src/stack_context.rs index 9ec14426..08415d29 100644 --- a/crates/mergify-stack/src/stack_context.rs +++ b/crates/mergify-stack/src/stack_context.rs @@ -173,9 +173,7 @@ pub fn resolve_repo( /// for local wiremock servers) without the coercion getting in /// the way. pub fn resolve_github_server(repo_dir: Option<&Path>) -> Result { - if let Ok(raw) = std::env::var("MERGIFY_GITHUB_SERVER") - && !raw.is_empty() - { + if let Some(raw) = mergify_core::env::var_non_empty("MERGIFY_GITHUB_SERVER") { return Url::parse(&raw).map_err(|e| { CliError::InvalidState(format!("invalid MERGIFY_GITHUB_SERVER '{raw}': {e}")) }); diff --git a/crates/mergify-stack/src/test_env.rs b/crates/mergify-stack/src/test_env.rs index 6d46393a..9df3bf16 100644 --- a/crates/mergify-stack/src/test_env.rs +++ b/crates/mergify-stack/src/test_env.rs @@ -7,16 +7,19 @@ //! is sporadic `git failed` panics in otherwise-pure tests //! that just happen to spawn git as a side effect. //! -//! The workspace forbids `unsafe_code`, so we can't `set_var` at -//! process start. Instead, [`isolated_git`] returns a fresh -//! `Command` with `GIT_CONFIG_GLOBAL=/dev/null` and -//! `GIT_CONFIG_NOSYSTEM=1` pre-applied; child git invocations -//! made *by the production code under test* will inherit these -//! when the parent test set them via the same helper before any -//! production call — i.e. wire `isolated_git` through the test -//! fixtures that build the repository, and the production code's -//! own `git` children pick up the same env via inheritance from -//! the spawned-fixture parent process (us). +//! Nothing here mutates the process environment — `mergify_core::env` +//! says why, and the rest of the workspace is being moved onto the +//! same footing — so this cannot be a `set_var` at process start. Instead [`isolated_git`] returns a fresh `Command` +//! with `GIT_CONFIG_GLOBAL=/dev/null` and `GIT_CONFIG_NOSYSTEM=1` +//! already on it. `Command::env` sets the *child's* environment, so +//! each git invocation carries the isolation itself; nothing is +//! shared and nothing has to be restored. +//! +//! It only covers the git commands that go through it. A `git` child +//! spawned by production code under test builds its own environment +//! from ours and sees neither these variables nor a test overlay, so +//! a fixture that needs isolation must create its repository state +//! through this helper. //! //! Practically: call [`isolated_git`] wherever the tests used to //! call `std::process::Command::new("git")`.