René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"`USE_SHELL = True` not needed since 2.0.8 but still suggested for Windows","articleBody":"The `Git.USE_SHELL` attribute is currently presented this way:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d986a59467e586afb815d71845f29b605b756cdf/git/cmd.py#L281-L285\r\n\r\nI 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.\r\n\r\nHowever, 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](https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags). 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`):\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d986a59467e586afb815d71845f29b605b756cdf/git/cmd.py#L231-L236\r\n\r\nThat 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](https://github.com/gitpython-developers/GitPython/blob/main/doc/source/changes.rst#208---features-and-bugfixes), the first release after #469 was merged, but that change was never noted.\r\n\r\nUsing `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.\r\n\r\nFor 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.\r\n\r\n---\r\n\r\nI 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:\r\n\r\n- Official Microsoft documentation of the effect of the `CREATE_NO_WINDOW` [process creation flag](https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags).\r\n- Addition of a symbolic constant in Python 3.7 [documented accordingly](https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NO_WINDOW). 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.\r\n- The explanation in 0d93908 (from [#469](https://github.com/gitpython-developers/GitPython/pull/469)) of the improvement of passing the flag.\r\n- **Manual testing, done today.** I improved on the testing procedure I had used in [#1685](https://github.com/gitpython-developers/GitPython/issues/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](https://github.com/gitpython-developers/GitPython/issues/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\")`.\r\n- 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](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886104405#step:12:477), [3.8](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886104650#step:12:477), [3.9](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886104817#step:12:477), [3.10](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886105001#step:12:477), [3.11](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886105114#step:12:477), [3.12](https://github.com/gitpython-developers/GitPython/actions/runs/7297204582/job/19886105203#step:12:477).\r\n\r\nIt 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.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-12-22T23:30:51.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":0},"url":"https://github.com/1781/GitPython/issues/1781"}