use sys.platform == "cygwin" to figure out when we are using cygwin
#2027
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
|
only this PR or #2026 should be merged |
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the detection of Cygwin Git by replacing the former multi-step logic with a concise check that relies on sys.platform.
- Removed deprecated functions and caching logic for Cygwin detection.
- Updated the Git command interface to directly use sys.platform and the Git executable's existence.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| git/util.py | Removed the py_where and _is_cygwin_git functions along with unused imports. |
| git/cmd.py | Updated the is_cygwin method to use a simplified sys.platform check. |
| @@ -661,7 +660,7 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: | |||
|
|
|||
| @classmethod | |||
| def is_cygwin(cls) -> bool: | |||
| return is_cygwin_git(cls.GIT_PYTHON_GIT_EXECUTABLE) | |||
| return cls.GIT_PYTHON_GIT_EXECUTABLE is not None and sys.platform == "cygwin" | |||
There was a problem hiding this comment.
Consider adding an inline comment explaining that sys.platform being 'cygwin' reliably indicates a Cygwin environment per Python 3.7+, to improve clarity for future maintainers.
Copilot uses AI. Check for mistakes.
sys.platform == "cygwin" to figure out when we are using cygwin
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.
The current logic for detecting cygwin is:
sys.platform == "win32", then it's not cygwinunameexecutable in the same folder as the git executable, and if so, does the output of that command include"CYGWIN"? if so, then it's cygwinIn the python 3.7 docs for sys.platform, Cygwin systems have
sys.platform == "cygwin". Since Python 3.7 is the oldest version still supported by GitPython, it stands to reason that we can rely on that being true for all supported Python versions.The logic I propose is:
sys.platform == 'cygwin', then it's cygwin gitThis is simple enough for a single expression, so I replaced the body of the
is_cygwinfunction with that expression and removedis_cygwin_gitand friends completely.I don't know if there's such a thing as
sys.platform == 'cygwin'and thegitnot being cygwingit, but if there is, I don't think the existing code deals with that anyway, so this seems to return the same result and would fail in the same way.