Keep test git from following a hook's GIT_DIR into this repo

.cargo/config.toml

@@ -0,0 +1,6 @@
+# Hooks, `rebase -x` and `bisect run` export GIT_DIR/GIT_INDEX_FILE (absolute in a worktree), and the
+# tests' temp-repo git would follow them into this repo. The runner clears them; see
+# tests/git_env_isolation_test.rs. If your own cargo config also sets a `cfg(...)` runner, cargo refuses
+# to pick one: override with CARGO_TARGET_<TRIPLE>_RUNNER, and the tests' tripwire still guards git.
+[target.'cfg(unix)']
+runner = "dev/without-git-repo-env"

dev/without-git-repo-env

@@ -0,0 +1,6 @@
+#!/bin/sh
+# Cargo runner (.cargo/config.toml): exec the binary without git's repo-local env.
+# Fails closed: if git can't list the variables, nothing runs.
+vars=$(git rev-parse --local-env-vars) || exit
+unset $vars
+exec "$@"

src/tracked_files.rs

@@ -29,8 +29,11 @@ pub(crate) fn find_tracked_files(base_path: &Path) -> Option<HashMap<PathBuf, bo
 mod tests {
     use super::*;
 
+    include!("../tests/support/git_env.rs");
+
     #[test]
     fn test_untracked_files() {
+        assert_git_env_isolated();
         let tmp_dir = tempfile::tempdir().unwrap();
         assert!(find_tracked_files(tmp_dir.path()).is_none());
 
@@ -58,6 +61,7 @@ mod tests {
 
     #[test]
     fn test_tracked_files_from_subdirectory() {
+        assert_git_env_isolated();
         let tmp_dir = tempfile::tempdir().unwrap();
         let backend_dir = tmp_dir.path().join("backend");
         let tracked_file = backend_dir.join("app/models/foo.rb");

tests/common/mod.rs

@@ -12,6 +12,8 @@ pub enum OutputStream {
     Stderr,
 }
 
+include!("../support/git_env.rs");
+
 #[allow(dead_code)]
 pub fn run_codeowners<I, P>(
     relative_fixture_path: &str,
@@ -75,6 +77,7 @@ pub fn copy_dir_recursive(from: &Path, to: &Path) {
 
 #[allow(dead_code)]
 pub fn git_reset_all(path: &Path) {
+    assert_git_env_isolated();
     let status = Command::new("git")
         .arg("reset")
         .current_dir(path)
@@ -89,6 +92,7 @@ pub fn git_reset_all(path: &Path) {
 
 #[allow(dead_code)]
 pub fn git_add_all_files(path: &Path) {
+    assert_git_env_isolated();
     let status = Command::new("git")
         .arg("add")
         .arg("--all")
@@ -104,6 +108,7 @@ pub fn git_add_all_files(path: &Path) {
 
 #[allow(dead_code)]
 pub fn init_git_repo(path: &Path) {
+    assert_git_env_isolated();
     let status = Command::new("git")
         .arg("init")
         .current_dir(path)
@@ -131,6 +136,7 @@ pub fn init_git_repo(path: &Path) {
 
 #[allow(dead_code)]
 pub fn is_file_staged(repo_root: &Path, rel_path: &str) -> bool {
+    assert_git_env_isolated();
     let output = Command::new("git")
         .arg("diff")
         .arg("--name-only")
@@ -149,6 +155,8 @@ pub fn is_file_staged(repo_root: &Path, rel_path: &str) -> bool {
 
 #[allow(dead_code)]
 pub fn build_run_config(project_root: &Path, codeowners_rel_path: &str) -> RunConfig {
+    // Callers pass the config to in-process runs that may stage.
+    assert_git_env_isolated();
     let project_root = project_root.canonicalize().expect("failed to canonicalize project root");
     let codeowners_file_path = project_root.join(codeowners_rel_path);
     let config_path = project_root.join("config/code_ownership.yml");

tests/git_env_isolation_test.rs

@@ -0,0 +1,146 @@
+use std::process::Command;
+
+mod common;
+
+const CHILD_ENV: &str = "CODEOWNERS_GIT_ENV_TRIPWIRE_CHILD";
+
+#[cfg(unix)]
+fn cargo_runner() -> Command {
+    Command::new(std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("dev/without-git-repo-env"))
+}
+
+#[cfg(unix)]
+#[test]
+fn test_cargo_runner_strips_repo_local_git_env() {
+    let temp_dir = tempfile::tempdir().unwrap();
+    let vars = common::repo_local_git_env_vars();
+    let mut cmd = cargo_runner();
+    for var in &vars {
+        cmd.env(var, temp_dir.path().join("outer.git"));
+    }
+    let output = cmd
+        .env("GIT_EDITOR", ":")
+        .arg("env")
+        .output()
+        .expect("failed to run the cargo runner");
+    assert!(
+        output.status.success(),
+        "runner failed: {}",
+        String::from_utf8_lossy(&output.stderr)
+    );
+
+    let env = String::from_utf8(output.stdout).unwrap();
+    let leaked: Vec<&String> = vars
+        .iter()
+        .filter(|var| env.lines().any(|line| line.starts_with(&format!("{var}="))))
+        .collect();
+    assert!(leaked.is_empty(), "runner passed {leaked:?} through to the test binary");
+    assert!(
+        env.lines().any(|line| line == "GIT_EDITOR=:"),
+        "runner dropped a git var that isn't repo-local"
+    );
+}
+
+#[cfg(unix)]
+#[test]
+fn test_cargo_runner_refuses_to_run_without_git() {
+    let empty_path = tempfile::tempdir().unwrap();
+    let output = cargo_runner()
+        .env("PATH", empty_path.path())
+        .args(["/bin/echo", "ran"])
+        .output()
+        .expect("failed to run the cargo runner");
+    assert!(!output.status.success(), "runner ran the binary without clearing git's env");
+    assert!(!String::from_utf8_lossy(&output.stdout).contains("ran"));
+}
+
+#[cfg(unix)]
+#[test]
+fn test_cargo_runner_preserves_the_exit_status() {
+    let status = cargo_runner()
+        .args(["/bin/sh", "-c", "exit 3"])
+        .status()
+        .expect("failed to run the cargo runner");
+    assert_eq!(status.code(), Some(3), "runner must not mask a failing test binary");
+}
+
+const HELPERS: &[&str] = &[
+    "init_git_repo",
+    "git_add_all_files",
+    "git_reset_all",
+    "is_file_staged",
+    "build_run_config",
+];
+
+// Runs one tests/common helper in a fresh copy of this binary, bypassing the cargo runner.
+fn run_git_helper_child(helper: &str, envs: &[(&str, std::path::PathBuf)]) -> (bool, String) {
+    let mut cmd = Command::new(std::env::current_exe().unwrap());
+    cmd.args(["git_helper_child", "--exact", "--ignored", "--test-threads=1"])
+        .env(CHILD_ENV, helper);
+    for (var, value) in envs {
+        cmd.env(var, value);
+    }
+    let output = cmd.output().expect("failed to re-run the test binary");
+    let log = format!(
+        "{}{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+    (output.status.success(), log)
+}
+
+// Ignored so an ambient CHILD_ENV can't turn a parent test into a no-op; a plain `--include-ignored` run does nothing.
+#[test]
+#[ignore = "run by the tests below"]
+fn git_helper_child() {
+    let Some(helper) = std::env::var_os(CHILD_ENV) else {
+        return;
+    };
+    let temp_dir = tempfile::tempdir().unwrap();
+    let path = temp_dir.path();
+    match helper.to_str().unwrap() {
+        "init_git_repo" => common::init_git_repo(path),
+        "git_add_all_files" => common::git_add_all_files(path),
+        "git_reset_all" => common::git_reset_all(path),
+        "is_file_staged" => {
+            common::is_file_staged(path, "CODEOWNERS");
+        }
+        "build_run_config" => {
+            common::build_run_config(path, "CODEOWNERS");
+        }
+        other => panic!("unknown helper {other}"),
+    }
+}
+
+#[test]
+fn test_git_helpers_refuse_inherited_repo_env() {
+    for helper in HELPERS {
+        // A pre-commit hook in a worktree exports both, as absolute paths.
+        let outer = tempfile::tempdir().unwrap();
+        let outer_git_dir = outer.path().join("outer.git");
+        let (succeeded, log) = run_git_helper_child(
+            helper,
+            &[("GIT_DIR", outer_git_dir.clone()), ("GIT_INDEX_FILE", outer_git_dir.join("index"))],
+        );
+        assert!(!succeeded, "{helper} should have refused: {log}");
+        assert!(
+            log.contains("refusing to run git with inherited"),
+            "{helper} failed unexpectedly: {log}"
+        );
+        assert!(!outer_git_dir.exists(), "{helper} wrote to the repo named by the inherited GIT_DIR");
+    }
+}
+
+#[test]
+fn test_git_helpers_refuse_an_inherited_index_file_alone() {
+    // `git commit -a` or `git commit <path>` in a regular clone exports only an absolute GIT_INDEX_FILE.
+    let outer = tempfile::tempdir().unwrap();
+    let outer_index = outer.path().join("index.lock");
+    let (succeeded, log) = run_git_helper_child("init_git_repo", &[("GIT_INDEX_FILE", outer_index.clone())]);
+    assert!(!succeeded, "child should have refused: {log}");
+    assert!(log.contains("\"GIT_INDEX_FILE\""), "tripwire didn't name GIT_INDEX_FILE: {log}");
+    assert!(
+        !outer_index.exists(),
+        "test git wrote to the index named by the inherited GIT_INDEX_FILE"
+    );
+}

tests/runner_api.rs

@@ -2,6 +2,8 @@ use std::path::Path;
 
 use codeowners::runner::{self, RunConfig};
 
+mod common;
+
 fn write_file(temp_dir: &Path, file_path: &str, content: &str) {
     let file_path = temp_dir.join(file_path);
     let _ = std::fs::create_dir_all(file_path.parent().unwrap());
@@ -176,6 +178,8 @@ javascript_package_paths:
         executable_name: None,
     };
 
+    // Stages in-process, so this process's own git env decides which index it writes.
+    common::assert_git_env_isolated();
     let gv = runner::generate_and_validate(&rc, vec![], true);
     assert!(gv.io_errors.is_empty(), "io: {:?}", gv.io_errors);
     assert!(gv.validation_errors.is_empty(), "val: {:?}", gv.validation_errors);

tests/support/git_env.rs

@@ -0,0 +1,28 @@
+// Spliced with include!() into tests/common and src/tracked_files.rs's unit tests, which can't share a module.
+
+// The variables that pin git to one repository, per git itself.
+#[allow(dead_code)]
+pub fn repo_local_git_env_vars() -> Vec<String> {
+    let output = std::process::Command::new("git")
+        .args(["rev-parse", "--local-env-vars"])
+        .output()
+        .expect("failed to run git rev-parse --local-env-vars");
+    assert!(output.status.success(), "git rev-parse --local-env-vars failed");
+    String::from_utf8_lossy(&output.stdout).lines().map(str::to_owned).collect()
+}
+
+// Inherited GIT_DIR/GIT_INDEX_FILE (hooks, worktrees) override current_dir and aim test git at the enclosing repo.
+// Call before any test git write, including in-process `runner::generate(_, true)` and a CLI `generate`;
+// the shared repo helpers in tests/common already do. Deliberately checks git's whole list, so a bypassed
+// runner fails closed even on benign vars like GIT_CONFIG_PARAMETERS.
+#[allow(dead_code)]
+pub fn assert_git_env_isolated() {
+    let leaked: Vec<String> = repo_local_git_env_vars()
+        .into_iter()
+        .filter(|var| std::env::var_os(var).is_some())
+        .collect();
+    assert!(
+        leaked.is_empty(),
+        "refusing to run git with inherited {leaked:?}; unset them, or run tests through this repo's cargo runner (.cargo/config.toml)"
+    );
+}