From 3e6412200b9a605f4f84374d9a14cdb542444124 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 28 Jul 2026 13:39:34 -0400 Subject: [PATCH] test: stop the coverage unit tests committing into the real repository test_coverage_unit.py builds a throwaway git repo with `git -C `. That sets the working directory but does NOT override an inherited GIT_DIR or GIT_INDEX_FILE. Git exports both when it runs a hook. MFC's pre-commit hook runs precheck, precheck runs this suite (toolchain/bootstrap/lint.sh), so committing anything ran three `git commit` calls against the developer's own checkout instead of the temporary one. The result is three commits titled "map" on the current branch, the first of which deletes every file in the repository, because the helper's `git add -A` had replaced the index with a single-file tree. It is silent: pytest reports every test passing while it happens, and the only visible symptom is the in-flight commit dying with "cannot lock ref 'HEAD'" because the child commits moved HEAD underneath it. Observed on a real branch, which then pushed the deletion to its pull request. Scrub GIT_* from the environment for the four git invocations that back these fixtures, and add a regression test that pins GIT_DIR at a path git cannot write and asserts the commit still lands in the throwaway repo. Without the scrub that test fails with "/nonexistent.git: Permission denied"; the rest of the suite passes either way, which is the point. --- toolchain/mfc/test/test_coverage_unit.py | 37 +++++++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/toolchain/mfc/test/test_coverage_unit.py b/toolchain/mfc/test/test_coverage_unit.py index 4b83fbb9ed..c4a0f558ab 100644 --- a/toolchain/mfc/test/test_coverage_unit.py +++ b/toolchain/mfc/test/test_coverage_unit.py @@ -1,3 +1,4 @@ +import os import subprocess import sys import tempfile @@ -461,19 +462,47 @@ def test_health_fails_immediately_when_map_predates_last_source_change(): CHANGED_SCRIPT = Path(__file__).resolve().parents[3] / ".github" / "scripts" / "coverage_map_changed.py" +def _env_without_git(): + """The environment minus every GIT_* variable. + + `git -C ` changes directory but does NOT override an inherited GIT_DIR or + GIT_INDEX_FILE. Git exports both when it runs a hook, and MFC's pre-commit hook runs + precheck, which runs this suite -- so without this scrub the commits below are made + against the real repository instead of the throwaway one. + """ + return {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} + + def _repo_with_committed_map(d, entries): """A throwaway git repo whose HEAD holds `entries` as the coverage map.""" repo = Path(d) + env = _env_without_git() git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] - subprocess.run([*git, "init", "-q"], check=True) + subprocess.run([*git, "init", "-q"], check=True, env=env) save_map(repo / "tests" / "coverage_map.json.gz", entries, n_tests=len(entries), git_sha="aaa", gfortran_version="13") - subprocess.run([*git, "add", "-A"], check=True) - subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "map"], check=True) + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "map"], check=True, env=env) return repo def _run_guard(repo): - return subprocess.run([sys.executable, str(CHANGED_SCRIPT)], cwd=repo, capture_output=True, text=True, check=False).returncode + return subprocess.run([sys.executable, str(CHANGED_SCRIPT)], cwd=repo, capture_output=True, text=True, check=False, env=_env_without_git()).returncode + + +def test_throwaway_repos_are_isolated_from_an_inherited_git_dir(): + """The helper above must not commit into whatever repo GIT_DIR names. + + Precheck runs this suite from the pre-commit hook, where git exports GIT_DIR and + GIT_INDEX_FILE. Without the scrub, `git -C tmpdir commit` rewrites the developer's + checked-out branch -- silently, while every test still reports as passing. + """ + with patch.dict(os.environ, {"GIT_DIR": "/nonexistent.git", "GIT_INDEX_FILE": "/nonexistent.index"}): + assert not [k for k in _env_without_git() if k.startswith("GIT_")] + with tempfile.TemporaryDirectory() as d: + repo = _repo_with_committed_map(d, {"k1": ["src/simulation/m_rhs.fpp"]}) + # The commit is in the throwaway repo, so it went nowhere near GIT_DIR. + log = subprocess.run(["git", "-C", str(repo), "log", "--oneline"], capture_output=True, text=True, check=True, env=_env_without_git()) + assert log.stdout.strip().endswith("map") def test_guard_reports_unchanged_with_a_code_that_is_not_the_crash_code():