Report actual attempted Git command when Git.refresh fails by EliahKagan · Pull Request #1812 · gitpython-developers/GitPython
added 10 commits
January 24, 2024 13:38shutil.which does not always find the git that Popen would run, at least on Windows, but it is no worse than running a "command -v" or "where" command as was previously done here via os.popen.
This extends test_refresh_bad_git_path so that it asserts that the exception message shows the command the failed refresh used. This test fails due to a bug where "git" is always shown (gitpython-developers#1809).
+ Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state. See gitpython-developers#1811. This addresses it incompletely, because while it is probably not necessary while running the test suite to preserve an old False value of git.GIT_OK (most tests can't work if that happens anyway), the value of Git.GIT_PYTHON_GIT_EXECUTABLE is not the only other global state that effects the behavior of subsequently run tests and that may be changed as a result of the refresh tests. 1. After the git.refresh function calls git.cmd.Git.refresh, it calls git.remote.FetchInfo.refresh, which rebinds the git.remote.FetchInfo._flag_map attribute. 2. Future changes to git.cmd.Git.refresh may mutate other state in the Git class, and ideally the coupling would be loose enough that the refresh tests wouldn't have to be updated for that if the behavior being tested does not change. 3. Future changes to git.refresh may perform other refreshing actions, and ideally it would be easy (and obvious) what has to be done to patch it back. In particular, it will likely call another Git method that mutates class-wide state due to gitpython-developers#1791, and for such state that is also of the Git class, ideally no further changes would have to be made to the code that restores state after the refresh tests. If we assume git.refresh is working at least in the case that it is called with no arguments, then the cleanup can just be a call to git.refresh(). Otherwise, sufficiently general cleanup may be more complicated.
For gitpython-developers#1811. This is the simple way to do it. This relies on git.refresh working when called when no arguments, so if that ever breaks, then it may be difficult or inefficient to investigate. But this is simpler than any alternatives that are equally or more robust. This is already better than the previous incomplete way that only restored Git.GIT_PYTHON_GIT_EXECUTABLE (and nothing in FetchInfo). Furthermore, because the tests already depend to a large extent on git.refreh working when called with no arguments, as otherwise the initial refresh may not have set Git.GIT_PYTHON_GIT_EXECUTABLE usably, it might not be worthwhile to explore other approaches.
I think this refactoring slightly improves readability. + Condense/sort/group imports (while adding contextlib import). + Minor style tweaks in FetchInfo.refresh comments. + Make a comment in Git.refresh clearer, building on revisions in: gitpython-developers#1810 (comment)
lettuce-bot bot referenced this pull request in lettuce-financial/github-bot-signed-commit
Feb 15, 2024This 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