René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Git.execute treats shell=False the same as shell=None","articleBody":"### Expected behavior\r\n\r\nThe *intention* in [`git.cmd.Git.execute`](https://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L827) 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:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L839\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L903-L905\r\n\r\n### The logic error\r\n\r\nThis is the code [in the `Popen` call](https://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L987) that is intended to implement that documented logic:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L995\r\n\r\n`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`:\r\n\r\n```python\r\n\u003e\u003e\u003e def ternary_truth_table(f):\r\n...     return {(p, q): f(p, q) for p in (True, False, None) for q in (True, False, None)}\r\n...\r\n\u003e\u003e\u003e table1 = ternary_truth_table(lambda p, q: p is not None and p or q)\r\n\u003e\u003e\u003e table2 = ternary_truth_table(lambda p, q: p or q)\r\n\u003e\u003e\u003e table1 == table2\r\nTrue\r\n\u003e\u003e\u003e table1\r\n{(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}\r\n```\r\n\r\n(This resembles the [`type(z)==types.ComplexType and z.real or z`](https://mail.python.org/pipermail/python-dev/2005-September/056510.html) problem that [led to the adoption of conditional expressions](https://peps.python.org/pep-0308/#id5).)\r\n\r\n### Possible fix\r\n\r\nI would *like* to fix this by doing:\r\n\r\n```diff\r\n-                    shell=shell is not None and shell or self.USE_SHELL,\r\n+                    shell=self.USE_SHELL if shell is None else shell,\r\n```\r\n\r\nOr, perhaps even more clearly:\r\n\r\n```diff\r\n+        if shell is None:\r\n+            shell = self.USE_SHELL\r\n...\r\n-                    shell=shell is not None and shell or self.USE_SHELL,\r\n+                    shell=shell,\r\n```\r\n\r\n### Could users be relying on the current (wrong) behavior?\r\n\r\nOn 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.\r\n\r\nExcept... setting it from outside GitPython *is* documented. In the changelog entry for [0.3.7](https://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/doc/source/changes.rst?plain=1#L669-L684):\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/doc/source/changes.rst?plain=1#L679-L682\r\n\r\nThis is [elaborated](https://github.com/gitpython-developers/GitPython/compare/0.3.6...0.3.7#diff-35a18a749eb4d6efad45e56e78a9554926be5526e2ba2159b44311e718450e88) at the `USE_SHELL` class attribute definition:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/git/cmd.py#L282-L286\r\n\r\n### The `pythonw` use case\r\n\r\nIn #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):\r\n\r\n\u003e 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.\r\n\u003e\r\n\u003e I saw it was using `Popen` to call the `git` executable and added a `shell=True` parameter to avoid spawning those windows.\r\n\r\nI 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.\r\n\r\n### Impact of a fix?\r\n\r\nMy 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.\r\n\r\nI 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.\r\n\r\nI'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.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-10-02T09:20:12.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":1},"url":"https://github.com/1685/GitPython/issues/1685"}