USE_SHELL = True not needed since 2.0.8 but still suggested for Windows

[EliahKagan]

The Git.USE_SHELL attribute is currently presented this way:

GitPython/git/cmd.py

Lines 281 to 285 in d986a59

	# If True, a shell will be used when executing git commands.
	# This should only be desirable on Windows, see https://github.com/gitpython-developers/GitPython/pull/126
	# and check `git/test_repo.py:TestRepo.test_untracked_files()` TC for an example where it is required.
	# Override this value using `Git.USE_SHELL = True`.
	USE_SHELL = False

I believe this encourages developers writing graphical Windows applications that use GitPython to set USE_SHELL = True, or to retain this in their code from when it may have been added in the past, at a time that it worked around a problem.

However, this is no longer necessary, even in that specific use case on Windows. The problem of console windows being created for GitPython's git subprocesses in graphical Windows applications was solved back in 2016, in 0d93908 (#469), by passing the CREATE_NO_WINDOW process creation flag. How and where that is expressed has changed over time, but the current code defines PROC_CREATIONFLAGS (which it passes to creationflags in calls to Popen):

GitPython/git/cmd.py

Lines 231 to 236 in d986a59

	if os.name == "nt":
	# CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards. See:
	# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
	PROC_CREATIONFLAGS = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
	else:
	PROC_CREATIONFLAGS = 0

That change to passing CREATE_NO_WINDOW was not the main motivation behind #469, and it appears to some extent to have escaped notice, not having made it into the manually written changelogs of any version of GitPython. The recommendation to set USE_SHELL to True had previously been entered into the changelog in 1c2dd54 for 0.3.7, when it was made no longer the default on Windows, as it had been since 3f277ba (#126). In contrast, it was no longer necessary (nor useful) to do this since 2.0.8, the first release after #469 was merged, but that change was never noted.

Using shell=True as a default is undesirable at least in the typical case that Git.execute is called indirectly through dynamic methods, because it is not typically feasible to account for the effect of shell expansions. The Git class is used in GitPython, and documented for use, in ways that do not account for characters with special meaning to the shell being present in text supplied as paths, branch and tag names, and so forth.

For that reason, combined with the better approach to #126 that came in with #469, comments or associated documentation for Git.USE_SHELL should no longer recommend its use in any likely scenarios, and should even caution against its use in the specific scenario where it was previously recommended.

---

I had suspected in #1685 (see "The pythonw use case") that calling Popen with shell=True might no longer be needed, but I did not realize that a change to GitPython, rather than Python or Windows, was responsible, and I was not sure. I now feel sure, due to a combination of the following factors:

• Official Microsoft documentation of the effect of the CREATE_NO_WINDOW process creation flag.
• Addition of a symbolic constant in Python 3.7 documented accordingly. Note that the technique worked before this, it was just necessary to define the constant or otherwise pass the correct value manually. These days, GitPython only supports Python 3.7 and higher, so the constant can be (and is) used.
• The explanation in 0d93908 (from #469) of the improvement of passing the flag.
• Manual testing, done today. I improved on the testing procedure I had used in #1685 by testing with pythonw -m idlelib on Python 3.7.9 and 3.12.1 virtual environments on Windows 10, both with git.cmd.PROC_CREATIONFLAGS unmodified, and separately by binding it to subprocess.CREATE_NEW_PROCESS_GROUP (i.e., it value without the CREATE_NO_WINDOW flag included). Removing the CREATE_NO_WINDOW flag, which I had not tried when working on #1685, reliably produced a console window that lasted the duration of a git command. I tested with the long-running command git.Repo.clone_from("https://github.com/huggingface/transformers.git", "transformers").
• Verification that the specific example in the USE_SHELL comment, to the extent to which it may have intended other situations where USE_SHELL = True was needed, is out of date. That comment points to test_untracked_files as a case that requires USE_SHELL = True on Windows. CI reveals that test case passes with no special action and with the default of False, including on Windows, on all Python versions GitPython supports: 3.7, 3.8, 3.9, 3.10, 3.11, 3.12.

It may make sense to deprecate Git.USE_SHELL, or to deprecate setting it to True. Even before that is determined, however, I think it is worthwhile to change the comment. The main reason I'm opening this as an issue, rather than detailing it in a PR, is so that if the particular wording or other aspects of my proposed change (#1782) end up needing to be rejected or deferred, the issue itself won't be lost track of.