merge queue: checking #1840 on main (1ef19c9)

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 }

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<String, CliError> {
     // 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<String, CliError> {
     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<OsString> {
-    std::env::var_os(name).filter(|v| !v.is_empty())
-}
-
 #[cfg(unix)]
 fn invoke_editor(editor: &OsString, path: &str) -> Result<std::process::ExitStatus, CliError> {
     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())),

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<Url, CliError> {
-    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}"))
         });

crates/mergify-stack/src/test_env.rs

@@ -7,16 +7,19 @@
 //! is sporadic `git <foo> 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")`.