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:

The logic error

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

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:

This is elaborated at the USE_SHELL class attribute definition:

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.