Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork939
correctly handleuname-cmd that doesn't point to an executable file#2026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…re trying to run it"This reverts commitde5e57c.
… it could both exist and have os.X_OK but not work
honestly, after having written this code, I'm not sure why we're not just delegating |
see#2027 for a different implementation of the same basic logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes the handling of cases where the uname command (used to detect Cygwin Git) is not an executable file and adds unit tests for verifying the behavior of is_cygwin_git. It also includes an update to the AUTHORS file to add a new contributor.
- Updated test_util.py to add tests for is_cygwin_git.
- Revised git/util.py to check if uname_cmd is an executable file before invoking it.
- Updated AUTHORS list.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/test_util.py | Added unit tests for is_cygwin_git functionality |
| git/util.py | Updated uname command executable check and caching logic |
| AUTHORS | Added new contributor |
Uh oh!
There was an error while loading.Please reload this page.
Thanks for contributing a fix! Indeed, I don't know why we are in the current place, but@EliahKagan probably has more information and I hope he can chime in. Besides that, my apologies, I did click the "copilot" review button out of curiosity, please feel free to completely ignore it. |
...incidentally, is there a way for me to run the test pipeline locally without pushing my changes up first? |
EliahKagan left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As argued in#2027 (comment), I think we should actually mergecffa264 -- that is, this PR as it was before the change to usingstrings -- before proceeding with further changes. I think that could be done at any time. I suggest rewinding this PR to that point (you can preserve the extra commits on their own separate branch of course).
There are some minor refinements that could be included with that, but they can just as easily be done afterwards (for example, I can make a small PR later that includes them), so they're not blocking. I've included review comments below showing them as diffs.
In addition to the reasons given in#2027 (comment) for starting that way, I also think it matches the scope you may have intended for this PR originally, based on the PR title.
This is not to say that the attempt to examine the contents of thegit executable file to discern if it is a Cygwin build should be discarded entirely -- only that I recommend integrating the working changes from before that, and then continuing with that subsequently. To that end, I've also included some review comments pertaining to those changes. To avoid confusion, I have refrained from including any diffs in those.
...incidentally, is there a way for me to run the test pipeline locally without pushing my changes up first?
Yes, certainly. There are instructions in the readme, there istox.ini if you want to use thetox runner, and the commands in the CI workflows in.github/workflows can sometimes be useful. I'm also happy to help with any questions about setting that up or problems that arise with it (and please feel free to ping me about it). There is also software to attempt to run GitHub Actions workflows locally, such asact, though I have not tried that on this repository and I don't know how well it would work.
However, in order to run the Cygwin tests on your local machine--using any of those techniques or others--I think you would need to have a Windows system (or other system capable of running Windows programs) with Cygwin installed on it.
| @@ -484,7 +493,8 @@ def is_cygwin_git(git_executable: PathLike) -> bool: ... | |||
| def is_cygwin_git(git_executable: Union[None, PathLike]) -> bool: | |||
| if sys.platform == "win32": # TODO: See if we can use `sys.platform != "cygwin"`. | |||
| _logger.debug(f"sys.platform = {sys.platform}, git_executable = {git_executable}") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The commit history indicates that the Python 3.8+ syntax{sys.platform=} was attempted and it was changed to this for compatibility. So I recommend:
| _logger.debug(f"sys.platform ={sys.platform}, git_executable ={git_executable}") | |
| _logger.debug(f"sys.platform={sys.platform!r}, git_executable={git_executable!r}") |
That's equivalent to the meaning of the original attempt, because the{var=} interpolation syntax does not add spaces around= unless written literally, and it implicitly callsrepr on the right-hand side, as!r does.
| class TestIsCygwinGit: | ||
| """Tests for :func:`is_cygwin_git`""" | ||
| def test_on_path_executable(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This may help clarify what's going with the test looking like#2027 even though the implementation does not, though the wording wouldn't have to be exactly this, and the references to#533 and#1455 could be removed if desired:
| deftest_on_path_executable(self): | |
| deftest_on_path_executable(self): | |
| # Currently we assume tests run on Cygwin use Cygwin git. See #533 and #1455 for background. |
(See also "The semantics ofis_cygwin_git changed a few years ago" in#2027 (comment).)
| git_dir = osp.dirname(git_executable) | ||
| git_cmd = pathlib.Path(git_executable) | ||
| git_dir = git_cmd.parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
git_executable is most commonly just"git" here. Callingos.path.dirname on that gave the empty string, which is falsy. That would cause the suite underif not git_dir: to run, performing an explicitPATH search forgit via thepy_where helper.
But under this change, whengit_executable is"git", callingpathlib.Path on it givesPosixPath('git'). Theparent attribute of that isPosixPath('.'). This is a truthy value, so the suite underif not git_dir: does not run. I don't think that's intentional.
| is_cygwin = "CYGWIN" in uname_out | ||
| git_dir = pathlib.Path(res[0]).parent if res else "" | ||
| # If it's a cygwin git, it'll have cygwin in the output of `strings git` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this comment is meant to go on thesubprocess.Popen call.
| git_dir = pathlib.Path(res[0]).parent if res else "" | ||
| # If it's a cygwin git, it'll have cygwin in the output of `strings git` | ||
| strings_cmd = pathlib.Path(git_dir, "strings") |
EliahKaganMay 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ifstrings is to be used here, then unlikeuname, its significance is not related to where it is located relative to thegit executable. Withuname, the idea is that if it's in the samebin directory asgit then hopefully they are the same kind of program. Withstrings, the goal is just to use a workingstrings command. That can be run asstrings. But it shouldn't be found relative to thegit executable.
(Or, since this is thought to be a Cygwin environment, it could be run as/usr/bin/strings, though I think that's less reliable and not necessary. In native Windows, implicitPATH searches can be dangerous because they usually include the current directory, leading toCWE-426 vulnerabilities likeCVE-2023-40590 andCVE-2024-22190. But that doesn't affect Cygwin builds of Python--at least not when they are directly invoking the subprocesses.)
In the common case thatgit_executable was"git", and with the other code as it currently stands, this is actually just assigning"strings" tostrings_cmd. That happens because, as noted in an above comment,git_dir starts out asPosixPath('.'), which it remains rather than being resolved to the actual directory wheregit is located. Different ways of joining a path after"." have different effects, but the waypathlib.Path does it is to discard the. component, so this just returns"strings".
| if not (pathlib.Path(strings_cmd).is_file() and os.access(strings_cmd, os.X_OK)): | ||
| _logger.debug(f"Failed checking if running in CYGWIN: {strings_cmd} is not an executable") | ||
| _is_cygwin_cache[git_executable] = False | ||
| return is_cygwin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If"strings" were just used literally, that would not facilitate this check, because we wouldn't know wherestrings is to check it. But that's okay, because usingstrings literally also makes it unnecessary to do this check. Either the system has astrings command available for general use, in which case attempting to run it with the simple namestrings will work, or it doesn't, in which case it will fail.
| process = subprocess.Popen([strings_cmd, git_cmd], stdout=subprocess.PIPE, text=True) | ||
| strings_output, _ = process.communicate() | ||
| is_cygwin = any(x for x in strings_output if "cygwin" in x.lower()) |
EliahKaganMay 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
git_cmd ispathlib.Path(git_executable), so in the common case thatgit_executable is"git", the value ofgit_cmd is justPosixPath('git'). (This is independent of whether the suite underif not git_dir: runs, since nothing in it referencesgit_cmd.) When passed tostrings, that's justgit in the current directory.
Usually, that would fail because there would be no such entry. When run from the top-level directory of the GitPython repository--as in the test suite--it is thegit subdirectory in the source code. This printsstrings: Warning: 'git' is a directory on standard error and does not read any strings.
Ifgit were resolved properly first, then this would work. However, I suspect there may be a better alternative tostrings here.
Cygwin programs link tocygwin1.dll
There's no guarantee that non-Cygwingit executables don't have a mention of Cygwin somewhere in the text they carry, so I think usingstrings has a small risk of false positives in general. In addition, it will treat MSYS2git as Cygwingit, as described in the "But another reason to start slow is MSYS2" section of#2027 (comment).
A more reliable indicator that an executable targets Cygwin is that it links tocygwin1.dll. Interactively, with executables one trusts, this is natural to check withldd. The Cygwingit links tocygwin1.dll:
(.venv) ✔ ~/repos-cygwin/GitPython [gcmarx-main|…1⚑ 1]23:47 $ ldd /usr/bin/git ntdll.dll => /cygdrive/c/Windows/SYSTEM32/ntdll.dll (0x7ff99a1b0000) KERNEL32.DLL => /cygdrive/c/Windows/System32/KERNEL32.DLL (0x7ff999cd0000) KERNELBASE.dll => /cygdrive/c/Windows/System32/KERNELBASE.dll (0x7ff9979b0000) cygpcre2-8-0.dll => /usr/bin/cygpcre2-8-0.dll (0x3ff260000) cygintl-8.dll => /usr/bin/cygintl-8.dll (0x3ff640000) cygiconv-2.dll => /usr/bin/cygiconv-2.dll (0x3ff670000) cygwin1.dll => /usr/bin/cygwin1.dll (0x7ff93d550000) cygz.dll => /usr/bin/cygz.dll (0x3fedc0000)While the MSYS2git does not:
(.venv) ✔ ~/repos-cygwin/GitPython [gcmarx-main|…1⚑ 1]23:56 $ ldd /cygdrive/c/msys64/usr/bin/git ntdll.dll => /cygdrive/c/Windows/SYSTEM32/ntdll.dll (0x7ff99a1b0000) KERNEL32.DLL => /cygdrive/c/Windows/System32/KERNEL32.DLL (0x7ff999cd0000) KERNELBASE.dll => /cygdrive/c/Windows/System32/KERNELBASE.dll (0x7ff9979b0000) msys-iconv-2.dll => /cygdrive/c/msys64/usr/bin/msys-iconv-2.dll (0x5603f0000) msys-intl-8.dll => /cygdrive/c/msys64/usr/bin/msys-intl-8.dll (0x430b30000) msys-2.0.dll => /cygdrive/c/msys64/usr/bin/msys-2.0.dll (0x180040000) msys-pcre2-8-0.dll => /cygdrive/c/msys64/usr/bin/msys-pcre2-8-0.dll (0x5d1c20000) msys-z.dll => /cygdrive/c/msys64/usr/bin/msys-z.dll (0x522fe0000)And the Git for Windowsgit does not:
(.venv) ✔ ~/repos-cygwin/GitPython [gcmarx-main|…1⚑ 1]23:54 $ ldd /cygdrive/c/Users/ek/scoop/apps/git/current/mingw64/bin/git ntdll.dll => /cygdrive/c/Windows/SYSTEM32/ntdll.dll (0x7ff99a1b0000) KERNEL32.DLL => /cygdrive/c/Windows/System32/KERNEL32.DLL (0x7ff999cd0000) KERNELBASE.dll => /cygdrive/c/Windows/System32/KERNELBASE.dll (0x7ff9979b0000) ADVAPI32.dll => /cygdrive/c/Windows/System32/ADVAPI32.dll (0x7ff9998a0000) msvcrt.dll => /cygdrive/c/Windows/System32/msvcrt.dll (0x7ff998f60000) sechost.dll => /cygdrive/c/Windows/System32/sechost.dll (0x7ff998210000) RPCRT4.dll => /cygdrive/c/Windows/System32/RPCRT4.dll (0x7ff999a80000) bcrypt.dll => /cygdrive/c/Windows/System32/bcrypt.dll (0x7ff997ff0000) USER32.dll => /cygdrive/c/Windows/System32/USER32.dll (0x7ff998360000) win32u.dll => /cygdrive/c/Windows/System32/win32u.dll (0x7ff997ea0000) libiconv-2.dll => /cygdrive/c/Users/ek/scoop/apps/git/2.49.0/mingw64/bin/libiconv-2.dll (0x7ff94eeb0000) GDI32.dll => /cygdrive/c/Windows/System32/GDI32.dll (0x7ff9981e0000) libpcre2-8-0.dll => /cygdrive/c/Users/ek/scoop/apps/git/2.49.0/mingw64/bin/libpcre2-8-0.dll (0x7ff9592e0000) gdi32full.dll => /cygdrive/c/Windows/System32/gdi32full.dll (0x7ff997ed0000) libintl-8.dll => /cygdrive/c/Users/ek/scoop/apps/git/2.49.0/mingw64/bin/libintl-8.dll (0x7ff97d720000) libwinpthread-1.dll => /cygdrive/c/Users/ek/scoop/apps/git/2.49.0/mingw64/bin/libwinpthread-1.dll (0x7ff97dfa0000) msvcp_win.dll => /cygdrive/c/Windows/System32/msvcp_win.dll (0x7ff997e00000) ucrtbase.dll => /cygdrive/c/Windows/System32/ucrtbase.dll (0x7ff997d00000) WS2_32.dll => /cygdrive/c/Windows/System32/WS2_32.dll (0x7ff999950000) zlib1.dll => /cygdrive/c/Users/ek/scoop/apps/git/2.49.0/mingw64/bin/zlib1.dll (0x7ff97a000000) CRYPTBASE.DLL => /cygdrive/c/Windows/SYSTEM32/CRYPTBASE.DLL (0x7ff996ac0000)This does not require thatldd be the Cygwinldd. For example, if we are in an MSYS2 environment, itsldd would also work. (If we are in the portable MSYS2 environment that ships with Git for Windows, that is likewise the MSYS2ldd command, and it works too.) Theseldd commands seem to work fine with all kinds of dynamically linked Windows executables. I don't think statically linkingcygwin1.dll is supported, but even if it were, Cygwingit doesn't do that.
It is dangerous to runldd on an untrusted executable. However, so long as we have resolvedgit correctly, it will be the samegit command we are running to perform Git operations, so we do trust it. So it might be okay to actually useldd for this, in a manner similar tostrings (albeit with different parsing).
However, I think it might also be feasible to examine the executable more directly. Surely there are Python libraries that help with this, but one of the long-standing benefits of GitPython is its extremely minimal default dependencies. I'm unsure how complicated it would be to reliably determine this by examining the executable using only facilities available in the Python standard library. So maybeldd is the way to go after all.
I suspect there may also be altogether different strategies I have not thought of.
I also ran into the issue from#1979. My proposed solution is that
GitPythonshould only try to rununame_cmdif it points to an executable file. I also wrote a short test class for theis_cygwin_gitfunction. I don't have a machine with Cygwin, so I can't test that it actually does work, but I trust thePython docs when they say that on Cygwin,sys.platformwill becygwin.