Let close_fds be True on all platforms

[EliahKagan]

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.

[Byron]

Thanks a lot for this fix! I wasn't aware anymore, and agree that by now there is no need to carry on with this special-casing.

Even though having more tests for desired behaviour is generally a good thing, I tend to prioritise and ask if this time is better spent elsewhere. Ultimately, you will be the one to answer that question and if it's even a little fun to add these tests, that's reason enough to do so :) (at least if there is nothing else that's more rewarding).

[EliahKagan]

(at least if there is nothing else that's more rewarding)

There's more-fun stuff I have in mind, some of which may be more useful too, so I'll probably put off adding tests for this change, but maybe circle back to it later.