Repository navigation
test(git): close test repositories instead of rmtree so teardown works on Windows - #4880
tsurutanmen wants to merge 1 commit into
Conversation
…s on Windows On Windows the test_repository fixture teardown raised PermissionError for 40 of 87 tests: shutil.rmtree ran while GitPython still held handles (WinError 32), and git's loose object files are read-only, which rmtree cannot delete (WinError 5). Close the repo to release its handles and leave tmp_path removal to pytest, which handles read-only files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
STiFLeR7
left a comment
There was a problem hiding this comment.
Summary. src/git's test_repository fixture manually shutil.rmtree(repo_path)s after each test. On Windows the PR reports PermissionError during teardown on 40 of 87 tests: GitPython's persistent git cat-file process still holds file handles (WinError 32), and git writes loose objects read-only, which shutil.rmtree can't delete (WinError 5). The fix calls test_repo.close() (releasing GitPython's handles) and drops the explicit rmtree, relying on pytest's own tmp_path cleanup instead.
Verified. (Windows 11, git 2.51.0, GitPython 3.2.1, pytest 8.3.3 — real run, not a mock)
- Ran
tests/test_server.pyon a fresh sparse checkout of currentmain, unpatched: 67 passed, 0 errors — I could not reproduce the claimed baseline (40 errors). Ran the fulltests/directory too: 164 passed, 1 unrelated failure (test_console_script_nonexistent_repository_exits_with_one_line_error, an environment/PATH issue from an editable install, not a teardown error). - The reasoning behind the fix is still sound even though I couldn't reproduce the symptom: pytest's
tmp_pathcleanup (_pytest.pathlib.rm_rf) alreadychmods and retries onPermissionError, so the described hand-off (fixture closes handles, pytest removes read-only files) is correct in principle, andclose()is a safe no-op-ish call on all platforms. - Didn't find a regression risk: the PR only removes an explicit
rmtreecall and adds aclose(); no behavior change on Linux/macOS, matching the PR's own claim.
Findings. None that block this — see "Not covered" for why I'm flagging the mismatch rather than treating it as settled.
Not covered / unverified. My environment (git 2.51.0, GitPython 3.2.1) didn't reproduce the 40-error baseline the PR describes, which is a meaningfully different git/GitPython version combination than whatever CI or the author's machine used — worth double-checking the PR still reflects current behavior before merging on the strength of the described repro alone, since the underlying handle-holding behavior may be version-dependent. I didn't install an older GitPython to chase the exact combination that reproduces it.
Verdict. Comment. The fix itself is safe and reasonable regardless, but I'd flag to the maintainer that the specific "40 errors" baseline didn't reproduce for me on a current Windows/git/GitPython stack, so the underlying bug may already be narrower (or different) than described.
|
Thank you for this pull request, and apologies for the long wait for a response. Since it was opened, the reference servers have been rebuilt for the 2026-07-28 MCP specification (#4857) and released as v2 (2.0.0 on npm for the TypeScript servers, 2026.10.19 on PyPI for the Python servers). The code this PR changes has been replaced, and the behavior it addresses has either been fixed in v2 or no longer exists in the same form. This is not a judgment on the change or on you; it's a consequence of the code it targets having been replaced. If the problem still happens on v2, please open a fresh issue with the version you're running and steps to reproduce. Note that this repository now accepts contributions as issues rather than pull requests from anyone but its maintainers; see This is part of the backlog triage in #4875. |
Description
On Windows, 40 of the 87 tests in
src/giterror during teardown of thetest_repositoryfixture. The assertions pass, but pytest reportsPermissionErrorfromshutil.rmtree(repo_path):[WinError 32](file in use): GitPython still holds handles, such as its persistentgit cat-fileprocesses, when the fixture removes the directory.[WinError 5](access denied): git writes loose object files as read-only, andshutil.rmtreecannot delete read-only files on Windows. This shows up in the twogit_addtests, where the git CLI writes the objects.This PR calls
test_repo.close()to release the handles, and drops the explicitrmtree. The repository lives under pytest'stmp_path, and pytest removes that directory itself, including read-only files.Server Details
Motivation and Context
Contributors on Windows currently see 40 errors from a clean checkout. That makes it hard to tell whether a change broke anything.
How Has This Been Tested?
On Windows 11, Python 3.11, git for Windows:
47 passed, 40 errors47 passedruff checkandpyrightare clean. There are no behavior changes on Linux or macOS:close()is a no-op there apart from releasing resources, andtmp_pathcleanup already applies.Breaking Changes
None. Only tests change.
Types of changes
Checklist
Additional context
I found this while working on #4879, which also touches this server.
🤖 Generated with Claude Code