Expected behavior

Theintention ingit.cmd.Git.execute is that when abool value,True orFalse, is passed forshell, that value determines whether a shell is used, and that only if this argument is omitted or passed asNone is the class or object'sUSE_SHELL attribute consulted:

The logic error

This is the codein thePopen call that is intended to implement that documented logic:

shell is not None and shell or self.USE_SHELL is intended to meanself.USE_SHELL if shell is None else shell (which could also be writtenshell if shell is not None else self.USE_SHELL). But it actually meansshell or self.USE_SHELL:

>>>defternary_truth_table(f):...return {(p,q):f(p,q)forpin (True,False,None)forqin (True,False,None)}...>>>table1=ternary_truth_table(lambdap,q:pisnotNoneandporq)>>>table2=ternary_truth_table(lambdap,q:porq)>>>table1==table2True>>>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 thetype(z)==types.ComplexType and z.real or z problem thatled to the adoption of conditional expressions.)

Possible fix

I wouldlike 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 whenUSE_SHELL isTrue. The class attributeUSE_SHELL is set toFalse 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 GitPythonis documented. In the changelog entry for0.3.7:

This iselaborated at theUSE_SHELL class attribute definition:

Thepythonw use case

In#126, a default ofshell=True had been introduced to fix a bug on Windows where console windows would flash on the screen forgit commands, when using thepythonw 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 usingPopen to call thegit executable and added ashell=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 usingpythonw -m idlelib.idle, and in IDLE, running statements such asimport git andgit.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 onGit.USE_SHELL taking precedence over the optionalshell keyword argument to theGit.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.