Fix Permission Error [WinError32] during cleanup#253
Conversation
|
Hi @desmondwong1215, thank you for your contribution! 🎉 This PR comes from your fork Before you request for a review, please ensure that you have tested your changes locally! Important The previously recommended way of using Please read the following instructions for the latest instructions. PrerequisitesEnsure that you have the Testing stepsIf you already have a local Git-Mastery root to test, you can skip the following step. Create a Git-Mastery root locally: gitmastery setupNavigate into the Git-Mastery root (defaults to cd gitmastery-exercises/Edit the {
# other fields...
"exercises_source": {
"username": "desmondwong1215",
"repository": "exercises",
"branch": "fix/clean-up"
}
}Then, you can use the gitmastery download <your new change>
gitmastery verifyChecklist
Important To any reviewers of this pull request, please use the same instructions above to test the changes. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a PermissionError (WinError 32) that occurs on Windows when cleaning up temporary directories containing Git repositories. The issue stems from Windows file locking behavior where open file handles and the current working directory prevent deletion of the temporary directory.
Changes:
- Added explicit
repo.close()calls for Git repository objects before cleanup - Changed working directory to parent of temp directory before attempting cleanup
- Reordered cleanup sequence to close repos → exit contexts → change directory → cleanup temp directories
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@desmondwong1215 On the app side, we likely also want to give an appropriate error/warning message to the user |
|
Yeah, I will add an error/warning message to the user when fixing this issue on the app side. |
|
|
||
| if self.__temp_dir is not None: | ||
| self.__temp_dir.cleanup() | ||
| if self.__rs and self.__rs.repo: |
There was a problem hiding this comment.
rs is used for testing only, is this related to the bug?
There was a problem hiding this comment.
When we try to clean up the temporary directory on line 224 (self.__temp_dir.cleanup()), Windows can't delete it because the Git repository objects still have open file handles.
jovnc
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the work!
Fix #252
I have tested it using WSL (it also works before these changes), PowerShell, and Git Bash. It works for me.