Fix mypy error with creationflags in subprocess module

On Windows, Python's subprocess module contains constants useful to
pass to the `creationflags` parameter of subprocess.Popen. These
are absent on other platforms, where they are not meaningful.

The code was already safe at runtime from any AttributeError
related to these constants, because they were only used in
git.cmd._safer_popen_windows, which was defined on Windows.

But in gitpython-developers#1792 I did not write the code in a way where mypy can
verify its correctness. So if a regression of the kind mypy can in
principle catch were to occur, it would be harder to notice it.

This refactors the code, keeping the same behavior but expressing
it in a way mypy can understand. This consists of two changes:

1. Only define _safer_popen_windows when the platform is Windows,
   placing it in the first branch of the `if` statement.

   This is needed because mypy will not take the only current call
   to that nonpublic function being on Windows as sufficient
   evidence that the platform is always Windows when it is run.

2. Determine the platform, for this purpose, using sys.platform
   instead of os.name.

   These are far from equivalent in general (see the deprecation
   rationale for is_<platform> in gitpython-developers#1732, revised in a0fa2bd
   in gitpython-developers#1787). However, in Python 3 (GitPython no longer supports
   Python 2), in the specific case of Windows, we have a choice of
   which to use, as both `sys.platform == "win32"` and
   `os.name == "nt"`.

   os.name is "nt" on native Windows, and "posix" on Cygwin.
   sys.platform is "win32" on native Windows (including 64-bit
   systems with 64-bit Python builds), and "cygwin" on Cygwin. See:
   https://docs.python.org/3/library/sys.html#sys.platform

   This is needed because the type stubs for the subprocess module
   use this sys.platform check (rather than an os.name check) to
   determine if the platform is Windows for the purpose of deciding
   which constants to say the subprocess module defines.

I have verified that neither of these changes is enough by itself.

Author: EliahKagan

Diff for: git/cmd.py

@@ -14,6 +14,7 @@
 import signal
 from subprocess import Popen, PIPE, DEVNULL
 import subprocess
+import sys
 import threading
 from textwrap import dedent
 
@@ -220,67 +221,67 @@ def pump_stream(
         finalizer(process)
 
 
-def _safer_popen_windows(
-    command: Union[str, Sequence[Any]],
-    *,
-    shell: bool = False,
-    env: Optional[Mapping[str, str]] = None,
-    **kwargs: Any,
-) -> Popen:
-    """Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
-
-    This avoids an untrusted search path condition where a file like ``git.exe`` in a
-    malicious repository would be run when GitPython operates on the repository. The
-    process using GitPython may have an untrusted repository's working tree as its
-    current working directory. Some operations may temporarily change to that directory
-    before running a subprocess. In addition, while by default GitPython does not run
-    external commands with a shell, it can be made to do so, in which case the CWD of
-    the subprocess, which GitPython usually sets to a repository working tree, can
-    itself be searched automatically by the shell. This wrapper covers all those cases.
+safer_popen: Callable[..., Popen]
 
-    :note:
-        This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath`
-        environment variable during subprocess creation. It also takes care of passing
-        Windows-specific process creation flags, but that is unrelated to path search.
+if sys.platform == "win32":
 
-    :note:
-        The current implementation contains a race condition on :attr:`os.environ`.
-        GitPython isn't thread-safe, but a program using it on one thread should ideally
-        be able to mutate :attr:`os.environ` on another, without unpredictable results.
-        See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
-    """
-    # CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
-    # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
-    # https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
-    creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
-
-    # When using a shell, the shell is the direct subprocess, so the variable must be
-    # set in its environment, to affect its search behavior. (The "1" can be any value.)
-    if shell:
-        # The original may be immutable or reused by the caller. Make changes in a copy.
-        env = {} if env is None else dict(env)
-        env["NoDefaultCurrentDirectoryInExePath"] = "1"
-
-    # When not using a shell, the current process does the search in a CreateProcessW
-    # API call, so the variable must be set in our environment. With a shell, this is
-    # unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
-    # patched. If that is unpatched, then in the rare case the ComSpec environment
-    # variable is unset, the search for the shell itself is unsafe. Setting
-    # NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and
-    # protects against that. (As above, the "1" can be any value.)
-    with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
-        return Popen(
-            command,
-            shell=shell,
-            env=env,
-            creationflags=creationflags,
-            **kwargs,
-        )
+    def _safer_popen_windows(
+        command: Union[str, Sequence[Any]],
+        *,
+        shell: bool = False,
+        env: Optional[Mapping[str, str]] = None,
+        **kwargs: Any,
+    ) -> Popen:
+        """Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
+
+        This avoids an untrusted search path condition where a file like ``git.exe`` in a
+        malicious repository would be run when GitPython operates on the repository. The
+        process using GitPython may have an untrusted repository's working tree as its
+        current working directory. Some operations may temporarily change to that directory
+        before running a subprocess. In addition, while by default GitPython does not run
+        external commands with a shell, it can be made to do so, in which case the CWD of
+        the subprocess, which GitPython usually sets to a repository working tree, can
+        itself be searched automatically by the shell. This wrapper covers all those cases.
 
+        :note:
+            This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath`
+            environment variable during subprocess creation. It also takes care of passing
+            Windows-specific process creation flags, but that is unrelated to path search.
 
-safer_popen: Callable[..., Popen]
+        :note:
+            The current implementation contains a race condition on :attr:`os.environ`.
+            GitPython isn't thread-safe, but a program using it on one thread should ideally
+            be able to mutate :attr:`os.environ` on another, without unpredictable results.
+            See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
+        """
+        # CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
+        # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
+        # https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
+        creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
+
+        # When using a shell, the shell is the direct subprocess, so the variable must be
+        # set in its environment, to affect its search behavior. (The "1" can be any value.)
+        if shell:
+            # The original may be immutable or reused by the caller. Make changes in a copy.
+            env = {} if env is None else dict(env)
+            env["NoDefaultCurrentDirectoryInExePath"] = "1"
+
+        # When not using a shell, the current process does the search in a CreateProcessW
+        # API call, so the variable must be set in our environment. With a shell, this is
+        # unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
+        # patched. If that is unpatched, then in the rare case the ComSpec environment
+        # variable is unset, the search for the shell itself is unsafe. Setting
+        # NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and
+        # protects against that. (As above, the "1" can be any value.)
+        with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
+            return Popen(
+                command,
+                shell=shell,
+                env=env,
+                creationflags=creationflags,
+                **kwargs,
+            )
 
-if os.name == "nt":
     safer_popen = _safer_popen_windows
 else:
     safer_popen = Popen