Git.execute treats shell=False the same as shell=None

[EliahKagan]

Expected behavior

The intention in git.cmd.Git.execute is that when a bool value, True or False, is passed for shell, that value determines whether a shell is used, and that only if this argument is omitted or passed as None is the class or object's USE_SHELL attribute consulted:

GitPython/git/cmd.py

Line 839 in 7d4f6c6

shell: Union[None, bool] = None,

GitPython/git/cmd.py

Lines 903 to 905 in 7d4f6c6

	:param shell:
	Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
	It overrides :attr:`USE_SHELL` if it is not `None`.

The logic error

This is the code in the Popen call that is intended to implement that documented logic:

GitPython/git/cmd.py

Line 995 in 7d4f6c6

shell=shell is not None and shell or self.USE_SHELL,

shell is not None and shell or self.USE_SHELL is intended to mean self.USE_SHELL if shell is None else shell (which could also be written shell if shell is not None else self.USE_SHELL). But it actually means shell or self.USE_SHELL:

>>> def ternary_truth_table(f):
...     return {(p, q): f(p, q) for p in (True, False, None) for q in (True, False, None)}
...
>>> table1 = ternary_truth_table(lambda p, q: p is not None and p or q)
>>> table2 = ternary_truth_table(lambda p, q: p or q)
>>> table1 == table2
True
>>> table1
{(True, True): True, (True, False): True, (True, None): True, (False, True): True, (False, False): False, (False, None): None, (None, True): True, (None, False): False, (None, None): None}

(This resembles the type(z)==types.ComplexType and z.real or z problem that led to the adoption of conditional expressions.)

Possible fix

I would like to fix this by doing:

-                    shell=shell is not None and shell or self.USE_SHELL,
+                    shell=self.USE_SHELL if shell is None else shell,

Or, perhaps even more clearly:

+        if shell is None:
+            shell = self.USE_SHELL
...
-                    shell=shell is not None and shell or self.USE_SHELL,
+                    shell=shell,

Could users be relying on the current (wrong) behavior?

On the surface that seems okay. The problem only happens when USE_SHELL is True. The class attribute USE_SHELL is set to False by default. It is named as a constant so, unless documented otherwise, it should really only be set during initialization, or by being monkey-patched inside GitPython's own tests. So it's tempting to think nobody should be relying on what happens if they change it.

Except... setting it from outside GitPython is documented. In the changelog entry for 0.3.7:

GitPython/doc/source/changes.rst

Lines 679 to 682 in 7d4f6c6

	* If the git executable can't be found in the PATH or at the path provided by `GIT_PYTHON_GIT_EXECUTABLE`, this is made
	obvious by throwing `GitCommandNotFound`, both on unix and on windows.
	- Those who support **GUI on windows** will now have to set `git.Git.USE_SHELL = True` to get the previous behaviour.

This is elaborated at the USE_SHELL class attribute definition:

GitPython/git/cmd.py

Lines 282 to 286 in 7d4f6c6

	# 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

The pythonw use case

In #126, a default of shell=True had been introduced to fix a bug on Windows where console windows would flash on the screen for git commands, when using the pythonw interpreter executable (selected to open .pyw files on Windows):

The GitPython module works great, but when called from a windowless script (.pyw) each command spawns a console window. Those windows are empty and last only a second, but I think they should not be there at all.

I saw it was using Popen to call the git executable and added a shell=True parameter to avoid spawning those windows.

I don't know if that is in practice ever still an issue or not. On my Windows 10 system, I created a Python 3.7.9 virtual environment (since 3.7 is the lowest version GitPython currently supports), installing GitPython in it, running IDLE in it using pythonw -m idlelib.idle, and in IDLE, running statements such as import git and git.Git().execute("git version"). No console window was created. However, it may be that this particular manual test is flawed, as IDLE, being a REPL, must be handling standard streams in a way that most other .pyw applications aren't. Nonetheless, the unwanted window creation seems like it should only have happened due to a bug in Python or Windows. But I am not sure.

Impact of a fix?

My concern is that existing software may be inadvertently relying on Git.USE_SHELL taking precedence over the optional shell keyword argument to the Git.execute method or to any function that forwards that argument to that method.

I believe this should be fixed, because applications that build on GitPython should be able to rely on documented claims about when shells are used to create subprocesses. But I am unsure if there is anything that should be documented or warned about, or taken account in deciding exactly how to fix this and how, if at all, the impact of the fix should be documented in the changelog or elsewhere.

I've proposed a fix, along the lines of those I suggested above, in #1687. But I wanted to open an issue to explore this, in case that fix is unsatisfactory, requires refinements, or has obscure effects (perhaps even only in cases that GitPython is used incorrectly) that people might find this by searching about.

[Byron]

Thanks for detailing the bug and the thought-process around it, while having done more due diligence than anyone could have hoped to get.

It's also interesting how a small logic bug like this can accumulate this much debt in the course of a decade - the more time passes the more software might depend on it, and the more one has to invest to make the right choice while trying to improve the software (instead of admitting defeat and leaving everything as is without any change).

I perfectly agree that this issue is a great platform for further discussion if a fix stirs up undesirable outcomes downstream.

---

I had to think about how this wouldn't have happened in Rust with the following result.

• use_shell would be of type Option<bool>
• when used, it would be done like this: shell.unwrap_or_else(git::use_shell)
• git::use_shell() is a function that returns some global configuration that users can change for good measure.

There is no logic to go wrong as the type-system enforces correct usage even for this trivial case where Option<T> does all the work essentially.