toolchain: precheck passes under the pre-commit hook from a worktree - #1807
Open
sbryngelson wants to merge 2 commits into
Open
toolchain: precheck passes under the pre-commit hook from a worktree#1807sbryngelson wants to merge 2 commits into
sbryngelson wants to merge 2 commits into
Conversation
…eck passes from worktrees
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes failures when running the coverage toolchain under a git hook launched from a worktree by ensuring git subprocesses ignore inherited GIT_* hook environment variables and operate on the intended repository directory.
Changes:
- Centralized “scrub
GIT_*from environment” logic intomfc.test.coverageand applied it to the shared git runner. - Updated
check_coverage_map_health.pyto route its git invocations through the centralized helper. - Removed the duplicated env-scrub helper from
test_coverage_unit.pyand imported the shared one instead.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
toolchain/mfc/test/test_coverage_unit.py |
Removes local _env_without_git helper and imports the shared implementation from mfc.test.coverage. |
toolchain/mfc/test/coverage.py |
Introduces _env_without_git() and applies it in _git() so git calls don’t inherit hook-provided GIT_* variables. |
.github/scripts/check_coverage_map_health.py |
Switches raw subprocess.run(["git", ...]) calls to use the shared _git() helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "toolchain")) | ||
| from mfc.test.coverage import COVERAGE_MAP_PATH, load_map, map_health # noqa: E402 | ||
| from mfc.test.coverage import COVERAGE_MAP_PATH, _git, load_map, map_health # noqa: E402 |
Comment on lines
+220
to
+222
| # Git exports GIT_DIR and GIT_INDEX_FILE to hooks, and neither cwd nor `git -C` overrides them: | ||
| # under the pre-commit hook every call below would otherwise act on the committing repository. | ||
| return {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} |
Comment on lines
+220
to
+221
| # Git exports GIT_DIR and GIT_INDEX_FILE to hooks, and neither cwd nor `git -C` overrides them: | ||
| # under the pre-commit hook every call below would otherwise act on the committing repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Committing from a git worktree fails the pre-commit hook: git exports
GIT_DIRandGIT_INDEX_FILEto hooks, and neithercwdnorgit -Coverrides them, so the throwaway repositories intest_coverage_unit.pyend up querying the committing repository instead. Two tests fail (test_verified_after_last_change_*) and the commit is blocked, leaving--no-verifyas the only way through.The test file already scrubbed
GIT_*for its own setup calls. This moves that scrub intocoverage._gitand routes the three raw git calls incheck_coverage_map_health.pythrough it, so every git subprocess the coverage machinery makes targets the directory it was given.Verified by making this commit through the hook from a worktree: precheck passes, the two tests pass under the hook's environment, and the full toolchain suite is 583 passed.