Let close_fds be True on all platforms by EliahKagan · Pull Request #1753 · gitpython-developers/GitPython (original) (raw)

Since Python 3.7, subprocess.Popen supports close_fds=True on all platforms, including Windows, and it is the default, including when arguments for standard streams have non-None values passed. 3.7 is the lowest version of Python that GitPython supports. So this omits the close_fds=True argument from the calls where it was present. Not passing close_fds at all has the same effect (in 3.7 and higher) as passing close_fds=True.

When the the close_fd argument was added to the Popen call in git.cmd.Git.execute in 1ee2afb, Python 2 was still supported. In Python 2, close_fds defaulted to False. This appears to be the reason it had been passed explicitly. It was conditioned on being on a Unix-like system because having it True on Windows would prevent stdin, stdout, or stderr redirection.

• Current subprocess.Popen documentation
• Old Python 2 subprocess.Popen documentation

A subtlety: forwarding the keyword argument

In the case of the Popen call in Cmd.execute, there is actually a difference between close_fds from the argument list and writing close_fds=False. If it is omitted, then the caller of Cmd.execute can pass it, and it will be forwarded through subprocess_kwargs without error.

This does not seem like something that would be done very often, and it is not as though there is working code doing it now, but it seems to me to be a small but worthwhile benefit, sufficient to justify being less explicit.

In particular, if someone needs to get the old behavior (see below), they can get it this way in some cases. (People don't usually use Git.execute directly, but I think it's harder to imagine scenarios where someone would want to do this kind of customization when not using it directly.)

Possible blockers

The alternative of passing it explicitly should be considered, though I suggest against it as described above. If you think it's better, I'd be pleased to make that change.

The more serious blockers are:

• Are there any reasons not to do this? Could anyone reasonably be depending on passing handles through to the subprocess (as happens on Windows without close_fd)? This doesn't seem like a breaking change, but maybe there is something strange I haven't thought of. It's of course possible to come up with contrived examples of code the change would break. But Git.execute does not currently document whether it passes close_fds or with what value.
• I have not added any tests. But the effect of close_fds can be observed and tested for. There don't seem to be tests specifically for it on Unix-like systems, but that doesn't mean there shouldn't be. I am inclined to think there should be tests for this, at least this change on Windows. (Or they could be added in a separate PR.) Ideally, the tests should in some way resemble, illustrate, or give some kind of insight into the benefit of avoiding passing open file descriptors and handles to subprocesses, in the context of GitPython.