only this PR or#2026 should be merged

Thanks for the thorough review!

Neither the value ofsys.platform nor “uname in the same directory asgit” seem like a bulletproof way to tell whether thegit in question is Cygwin git or not. I’m wondering if the git executable itself can be convinced to tell us about itself, but don’t currently have a way to test that. I’ll try to poke around at that later this week.

I've looked into this further.

Keepinguname can be temporary, I hope

I’m wondering if the git executable itself can be convinced to tell us about itself, but don’t currently have a way to test that. I’ll try to poke around at that later this week.

Thanks! Yes, usinguname for this in the way we have been doing (or in any way at all, really) doesn't facilitate high-quality detection of whethergit is a Cygwin program. Nonetheless, I think the changes in#2026 up tocffa264 (i.e. the tip of the branch prior to recent changes) are strictly improvements compared to what is on ourmain now, and that they can probably be integrated as an initial mitigation of--though still not a complete fix for--#1979.

I agree that, ultimately, we will want to do something else. I do think that will involve inspecting the contents of thegit executable; though it might imaginably use some other technique to figure it out; or it might turn out not to be worthwhile for GitPython to try to figure this out, in which case it could still be changed to just checksys.platform == "cygwin", as in this PR.

Why I advocate starting that way

The reason I would favor the (original) approach taken in#2026 as a first step is that it is maximallyconsistent with the previous behavior. This is valuable because GitPython iswidely used, and it's easy to break existing usage by accident, including in Cygwin-related ways (e.g.#1646).

I think that makes it a good initial change, and since it addresses#1979 in the cases where people seem usually to have encountered it--where the environment has nothing to do with Windows or Cygwin--we could even maybe do a patch release shortly afterwards. Even if not, it would be in place so that, if a patch release is done for some other reason, it will have it.

Also, as detailed below (at the very end), I am not sure if always returningTrue whensys.platform == "cygwin" does the right thing on MSYS2.

Counterpoints/caveats

Although my position is that we should not rush to replace the implementation ofis_cygwin_git with a simple check tosys.platform == "cygwin", my analysis would be incomplete if I did not acknowledge three arguments for doing so:

1. It's very simple.
2. It's not necessarily less accurate than what we've been doing.
3. Code from before 2022 that it would break has probably already broken.

The first point is self-evident, while the second and third touch on some things that are potentially of interest to continuing work onis_cygwin_git no matter how we proceed, and are thus detailed below.

2. Theuname way isn't very accurate

Neither the value ofsys.platform nor “uname in the same directory asgit” seem like a bulletproof way to tell whether thegit in question is Cygwin git or not.

Yes, the "uname in the same directory asgit" way we've been doing is admittedly not good, and very brittle.

In terms of accuracy, it may be worse than the approach here of just checkingsys.platform == "cygwin" and assuminggit is a Cygwin program if the Python interpreter is a Cygwin program. This may be so even when changed to include such a check (though that change, in3f5a942, is good and definitely one of the benefits of#2026). Here are a few examples of why this may be so:

• GitPython could be used in a Python script runby (or otherwise through)git, such as a customgit command, a transport command, or a hook. Then thegit executable GitPython finds will usually be the one ingit'slibexec/git-core directory--more precisely, the one in the directorygit --exec-path reports. This is becausegit currently unconditionally prepends that directory toPATH when performing most subprocess invocations. That directory does not also contain auname executable.

• Checking if thegit executable is in the same directory as auname executable that reports the operating system as Cygwin is--when it works--effectively a way to check ifgit is in a Cygwin/usr/bin directory where the Cygwin package manager places both binaries.

  is_cygwin_git and supportingpy_where functions are written as they are for historical reasons. As detailed below, when they were introduced, the check had different semantics, and the standard library had fewer facilities. But if we were to introduce wholly new code today that guesses, based on the location ofgit, whethergit is from Cygwin, then (ignoring the MSYS2 issue) we would probably do something like this instead:

  sys.platform=="cygwin"andgit_executableandshutil.which(git_executable)=="/usr/bin/git"

  The problem is that one can have Cygwingit elsewhere--even when using the "primary"git executable rather than the one inlibexec/git-core. This happens if one builds it from source oneself to test a patch or use a different version, or if one runs it through a symlink.

• Eliminating the existing implementation, as done in this PR, would solve other problems unrelated to#1979.

  Whenis_cygwin_git is used, the value of itsgit_executable argument is most often"git". To avoid an excessive slowdown,is_cygwin_git caches results per-argument in_is_cygwin_cache. This cache entry is never invalidated. But which executable"git" runs--and even where that executable is found--can change. This can happen by changing the contents of the filesystem or changing the value of thePATH environment variable. Such a change has other notable effects, including that the cachedgit version number may no longer be correct. We intend that programs that use GitPython can adjust to such a change by calling itstop-levelrefresh function. For the most part, this works properly (including invalidating the version). But it does not modify_is_cygwin_cache.

  This can, of course, be fixed. But I acknowledge that if we no longer try to figure out if thegit executable is a Cygwin program, then that would eliminate this bug automatically. (To be clear, the onus is not on#2026 to fix this bug, since nothing in that PR makes it any worse.)

So it may be that such cases--of which I suspect there are various others--make it so that the simpler change here of just usingsys.platform == "cygwin" is more accurate not only than the current implementation but than anything resembling it. However, even if improving overall accuracy, we would run the risk of introducing new inaccuracies in particular unanticipated use cases.

3. The semantics ofis_cygwin_git changed a few years ago

In#2027 (review) I noted that the distinction between the Python interpreter being a Cygwin program and thegit executable being a Cygwin program is long-standing, and that it can be important. That's true. However, the current behavior ofis_cygwin_git only really dates back to 2022. Anything in software using GitPython that a major change tois_cygwin_git would break now is probably not older than that, or it would've been broken then.

There have been two big pushes to improve Cygwin compatibility in GitPython:

• The first wasSupport Cygwin's Git on Windows #533 in 2016, which introducedis_cygwin_git ine6e23ed. At that time, it began:

  GitPython/git/util.py

  Lines 287 to 288 ine6e23ed

  	ifnotis_win:
  	returnFalse

  Where:

  GitPython/git/compat.py

  Line 28 ine6e23ed

  is_win= (os.name=='nt')

  I believe that, even at that time and even in Python 2,os.name on Cygwin evaluated to"posix" as it does today. Thus, the scenario whereis_cygwin_git would returnTrue was when the Python interpreter was anative Windows executable andgit was a Cygwin executable! This makes sense, since the title of that PR was "Support Cygwin's Git on Windows".

  At that time, AppVeyor was used for CI. The.appveyor.yml file from that time confirms that this is what was going on--the original purpose ofis_cygwin_git was to allownative Windows Python programs to use GitPython with either Git for Windows or Cygwingit. In relevant part:

  GitPython/.appveyor.yml

  Lines 2 to 38 incc77e6b

  	environment:
  	GIT_DAEMON_PATH:"C:\\Program Files\\Git\\mingw64\\libexec\\git-core"
  	CYGWIN_GIT_PATH:"C:\\cygwin\\bin;%GIT_DAEMON_PATH%"
  	CYGWIN64_GIT_PATH:"C:\\cygwin64\\bin;%GIT_DAEMON_PATH%"
  	matrix:
  	## MINGW
  	#
  	-PYTHON:"C:\\Python27"
  	PYTHON_VERSION:"2.7"
  	GIT_PATH:"%GIT_DAEMON_PATH%"
  	-PYTHON:"C:\\Python34-x64"
  	PYTHON_VERSION:"3.4"
  	GIT_PATH:"%GIT_DAEMON_PATH%"
  	-PYTHON:"C:\\Python35-x64"
  	PYTHON_VERSION:"3.5"
  	GIT_PATH:"%GIT_DAEMON_PATH%"
  	-PYTHON:"C:\\Miniconda35-x64"
  	PYTHON_VERSION:"3.5"
  	IS_CONDA:"yes"
  	GIT_PATH:"%GIT_DAEMON_PATH%"
  	## Cygwin
  	#
  	-PYTHON:"C:\\Miniconda-x64"
  	PYTHON_VERSION:"2.7"
  	IS_CONDA:"yes"
  	IS_CYGWIN:"yes"
  	GIT_PATH:"%CYGWIN_GIT_PATH%"
  	-PYTHON:"C:\\Python35-x64"
  	PYTHON_VERSION:"3.5"
  	GIT_PATH:"%CYGWIN64_GIT_PATH%"
  	IS_CYGWIN:"yes"
  	install:
  	-set PATH=%PYTHON%;%PYTHON%\Scripts;%GIT_PATH%;%PATH%

• The second wasRe-enable Cygwin CI and get most tests passing #1455 in 2022, which included this very significant change tois_cygwin_git in96fae83:

  def is_cygwin_git(git_executable: Union[None, PathLike]) -> bool:-    if not is_win:+    if is_win:+        # is_win seems to be true only for Windows-native pythons+        # cygwin has os.name = posix, I think       return False

  Much as#533 was guided by testing on AppVeyor set up so that Cygwingit was being called only from native Windows builds of Python,#1455 was guided by testing on GitHub Actions set up so that Cygwingit was being called only from Cygwin builds of Python.

  I cannot tell from the history whether this distinction was recognized at the time! It may be that this change was made based on the belief thatis_cygwin_git had already been intended to returnFalse except in Cygwin builds of Python, I am not sure. But it may be that it was intentional. It would make sense if, over time, it has become rarer and rarer to use anything butGit for Windows forgit on Windows, except when operating entirely within some other specific environment.

An important question is whether, since#1455, there is anything in GitPython that actually works because of the ability to identify a non-Cygwingit executable from Cygwin. Operations with such an executable seem either to work onmain, in the original#2026 (cffa264), and here...or not to work in any of the three. An example of something that should in principle work if GitPython on Cygwin supports non-Cygwingit, but that fails and fails the same way in all three, is:

(.venv) ✔ ~/repos-cygwin/GitPython [main|⚑ 1]09:27 $ GIT_PYTHON_GIT_EXECUTABLE=/cygdrive/c/Users/ek/scoop/shims/git python3.9 -c 'import git; r1 = git.Repo.clone_from("https://github.com/EliahKagan/trivial.git", "trivial"); r2 = r1.clone("trivial-clone", bare=True); print(r2.git_dir)'Traceback (most recent call last):  File "<string>", line 1, in <module>  File "/home/ek/repos-cygwin/GitPython/git/repo/base.py", line 1482, in clone    return self._clone(  File "/home/ek/repos-cygwin/GitPython/git/repo/base.py", line 1414, in _clone    finalize_process(proc, stderr=stderr)  File "/home/ek/repos-cygwin/GitPython/git/util.py", line 504, in finalize_process    proc.wait(**kwargs)  File "/home/ek/repos-cygwin/GitPython/git/cmd.py", line 834, in wait    raise GitCommandError(remove_password_if_present(self.args), status, errstr)git.exc.GitCommandError: Cmd('/cygdrive/c/Users/ek/scoop/shims/git') failed due to: exit code(128)  cmdline: /cygdrive/c/Users/ek/scoop/shims/git clone -v --bare -- /home/ek/repos-cygwin/GitPython/trivial/.git trivial-clone  stderr: 'fatal: repository '/home/ek/repos-cygwin/GitPython/trivial/.git' does not exist'

What happens there is that adecygpath operation is either not being done or not being done correctly in at least one place where it would need to happen to support Git for Windowsgit from Cygwin. I am not sure if this has ever worked.

So that's admittedly a reason to prefer this PR: it's not clear under what circumstances, if any,is_cygwin_git has done any better than this, since#1455.

But I think this examination of the history also shows that the original#2026 (up tocffa264) is safe and strictly improves correctness. Thesys.platform == "win32" condition onmain descends from theis_win check. (sys.platform == "win32" always has the same value asos.name == "nt", at least in Python 3 and I think since well before that.) When I introducedthe TODO comment in42e10c0 (#1859), I was worried that doing the check even when the platform doesn't seem to be Cygwin might've been intentional, and that checking ifgit seems to be a Cygwin executable only whensys.platform == "cygwin" might somehow break something. I think looking at the history confirms that this is not the case.

But another reason to start slow is MSYS2

Somewhat confusingly, Cygwin is not the only platform in whichsys.platform is"cygwin".

We don't test MSYS2 on CI, though maybe we should. I don't know how often, if at all, people use GitPython in a Python interpreter that targets MSYS2. (This should not be confused with other environments that MSYS2 supplies, such as MINGW64, which is actually a native Windows target rather than a Cygwin-like target. I am talking about the MSYSenvironment of MSYS2, which is a non-Cygwin but Cygwin-like target.)

In MSYS2,sys.platform is"cygwin":

ek@Glub MSYS ~$ python -c 'import sys; print(sys.platform)'cygwin

Butuname does not reportCYGWIN:

ek@Glub MSYS ~$ unameMSYS_NT-10.0-19045

The facilities within GitPython that do Cygwin-related things, such ascygpath anddecygpath, and their helpers, ingit/util.py--which are used in more places whengit is detected to be a Cygwin build--don't look like they will always work on MSYS2. While a MSYS2 does have/proc/cygdrive, it does not have/cygdrive; a path like/cygdrive/c in Cygwin is just/c is MSYS2. There may be other relevant differences.

As things stand now, where MSYS2git run from MSYS2 is not considered to be a Cygwingit, MSYS2 does seem to work pretty well, though maybe not perfectly. I say that based onthis test run; see#1988 (comment) for context. It may be good to look into that further before doing anything that would cause MSYS2 to be treated more like Cygwin.

This also applies tothestrings approach pushed to#2026 aftercffa264--strings will turn upcygwin in MSYS2git as well:

ek@Glub MSYS ~$ strings /usr/bin/git | grep -F cygwincygwin_conv_pathcygwin_internal__imp_cygwin_internal__imp_cygwin_conv_path

So this is part of why I'd be interested to integratecffa264 first.