Fix typo in _get_exe_extensions PATHEXT fallback #1890
Merged
Conversation
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
PATHEXT lists file extensions with the ".". In the fallback given in _get_exe_extensions, the other extensions had this, but ".COM" was listed without the ".". This fixes that. This is very minor because _get_exe_extensions is nonpublic and not currently used on native Windows, which is the platform where the PATHEXT fallback code would be used. Specifically, _get_exe_extensions is called only in py_where, which while named with no leading underscore is nonpublic do not being (and never having been) listed in __all__. As its docstring states, it is an implementation detail of is_cygwin_git and not intended for any other use. More specifically, is_cygwin_git currently immediately returns False on *native* Windows (even if the git executable GitPython is using is a Cygwin git executable). Only on Cygwin, or other systems that are not native Windows, does it try to check the git executable (by calling its _is_cygwin_git helper, which uses py_where).
|
Thanks a lot for the fix, a great catch!
I don't think it is intentional and would hope it could be set to what makes most sense eventually. I agree with all the other points as well and look forward to seeing them fixed/improved eventually. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This applies two minor Cygwin-related clarifications, one of which is theoretically a bugfix though it affects conceptually non-public code that is currently not used in a way that could invoke the bug. Some specific details are in commit messages.
The second commit, 988d97b, which fixes
COMto.COMin the fallback for thePATHEXTenvironment variable--whose omission is almost certainly a typo, going back to the function's introduction in e6e23ed (#533)--is theoretically a bugfix. But as noted there, this code is only called when the Python interpreter is a native Windows program, but the function it is a part of is only called when the Python interpreter is not a native Windows program. (Also, it would be pretty unusual for thegitexecutable to have a.comextension.)There are some further changes related to Cygwin detection that may make sense to make in the future, some of which might be best to make after adding relevant tests. The ones that are narrowly related to the change here are that:
PATHEXThere is still unusual, because order matters and this lists.BATbefore.COMand.EXE, and because it is missing.CMD, which is part of what a minimal value for it should typically contain. I am uncertain if this strange order is intentional..VBS,.JS,.WS, and.MSCfor compatibility with the fallback behavior of the Windowscmd.exeshell whenPATHEXTis unset, which is whatshutil.whichdoes. (I have verified the result of Eryk Sun's debugger check in WinDbg on recent builds of Windows 10 and Windows 11; they are the same.)py_whereshould really be removed and replaced either with a call toshutil.which, which did not exist at the timepy_wherewas introduced and would almost certainly have been used at that time if it had--or with other logic that works an altogether different way. Which should be done depends on the intent of the publicGit.is_cygwin_gitclass method, which is the only code in GitPython that uses theis_cygwin_gitfunction ingit/util.py, for which thepy_wherehelper exists.This PR doesn't try to cover any of that. It's really just applying some clarifications that didn't seem like they fit in any other PR that I would do soon.
Regarding the last point about
Git.is_cygwin_git, if the goal is to detect Cygwin git even on native Windows systems, then the code it uses will have to be significantly revised. That is because currently it always returnsFalseon such systems. In addition, as noted in thepy_wheredocstring and elsewhere, shell and non-shell path search differ from each other on native Windows, such that neither the nonpublic ad-hocpy_wherenor the public generalshutil.whichwill reliably tell us wheregitis. However, if a path search doesn't need to be done on native Windows, then it can be replaced withshutil.whichunless some use has come to depend on (perhaps accidentally introduced) distinctive behavior ofpy_where.I plan to open an issue about that larger matter, which I am unlikely to work on anytime soon, but that perhaps someone would be interested to take on. (If you know what
Git.is_cygwin_git's semantics are supposed to be, then I can make use of that information in the forthcoming issue, but I can still open it even if its intent is currently uncertain.)