Deprecate compat.is_<platform>, rewriting all uses

Major changes:

- Add a comment in the git.compat module above the definitions of
  is_win, is_posix, and is_darwin stating that they are deprecated
  and recommending that os.name (or, where applicable,
  sys.platform) be used directly instead for clarity (and sometimes
  accuracy).

- Remove all uses of is_win and is_posix in git/ and test/,
  replacing them with `os.name == "nt"` and `os.name == "posix"`,
  respectively.

There were no uses of is_darwin to be replaced. Although it had
been used at one time, the last reference to it appears to have
been removed in 4545762 (#1295).

This doesn't emit a DeprecationWarning when those attributes are
accessed in the git.compat module. (That might be valuable thing to
do in the future, if the git.compat module is to remain
non-deprecated overall.)

Two related less consequential changes are also included:

- Improve ordering and grouping of imports, in modules where they
  were already being changed as a result of no longer needing to
  import is_<platform> (usually is_win) from git.compat.

- Make minor revisions to a few comments, for readability. (This is
  in addition to somewhat more substantial revisions of comments
  where rewording was related to replacing uses of is_<platform>.)

Author: EliahKagan

Diff for: .github/workflows/cygwin-test.yml

@@ -72,7 +72,7 @@ jobs:
         python --version
         python -c 'import sys; print(sys.platform)'
         python -c 'import os; print(os.name)'
-        python -c 'import git; print(git.compat.is_win)'
+        python -c 'import git; print(git.compat.is_win)'  # NOTE: Deprecated. Use os.name directly.
 
     - name: Test with pytest
       run: |

Diff for: .github/workflows/pythonpackage.yml

@@ -63,7 +63,7 @@ jobs:
         python --version
         python -c 'import sys; print(sys.platform)'
         python -c 'import os; print(os.name)'
-        python -c 'import git; print(git.compat.is_win)'
+        python -c 'import git; print(git.compat.is_win)'  # NOTE: Deprecated. Use os.name directly.
 
     - name: Check types with mypy
       run: |

Diff for: git/cmd.py

@@ -17,19 +17,21 @@
 import threading
 from textwrap import dedent
 
-from git.compat import (
-    defenc,
-    force_bytes,
-    safe_decode,
-    is_posix,
-    is_win,
+from git.compat import defenc, force_bytes, safe_decode
+from git.exc import (
+    CommandError,
+    GitCommandError,
+    GitCommandNotFound,
+    UnsafeOptionError,
+    UnsafeProtocolError,
 )
-from git.exc import CommandError
-from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present, patch_env
-
-from .exc import GitCommandError, GitCommandNotFound, UnsafeOptionError, UnsafeProtocolError
-from .util import (
+from git.util import (
     LazyMixin,
+    cygpath,
+    expand_path,
+    is_cygwin_git,
+    patch_env,
+    remove_password_if_present,
     stream_copy,
 )
 
@@ -180,14 +182,13 @@ def pump_stream(
         t.start()
         threads.append(t)
 
-    ## FIXME: Why Join??  Will block if `stdin` needs feeding...
-    #
+    # FIXME: Why join? Will block if stdin needs feeding...
     for t in threads:
         t.join(timeout=kill_after_timeout)
         if t.is_alive():
             if isinstance(process, Git.AutoInterrupt):
                 process._terminate()
-            else:  # Don't want to deal with the other case
+            else:  # Don't want to deal with the other case.
                 raise RuntimeError(
                     "Thread join() timed out in cmd.handle_process_output()."
                     f" kill_after_timeout={kill_after_timeout} seconds"
@@ -197,11 +198,11 @@ def pump_stream(
                     "error: process killed because it timed out." f" kill_after_timeout={kill_after_timeout} seconds"
                 )
                 if not decode_streams and isinstance(p_stderr, BinaryIO):
-                    #  Assume stderr_handler needs binary input
+                    #  Assume stderr_handler needs binary input.
                     error_str = cast(str, error_str)
                     error_str = error_str.encode()
                 # We ignore typing on the next line because mypy does not like
-                # the way we inferred that stderr takes str or bytes
+                # the way we inferred that stderr takes str or bytes.
                 stderr_handler(error_str)  # type: ignore
 
     if finalizer:
@@ -228,14 +229,12 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
 ## -- End Utilities -- @}
 
 
-# value of Windows process creation flag taken from MSDN
-CREATE_NO_WINDOW = 0x08000000
-
-## CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards,
-# see https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
-PROC_CREATIONFLAGS = (
-    CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP if is_win else 0  # type: ignore[attr-defined]
-)  # mypy error if not Windows.
+if os.name == "nt":
+    # CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards. See:
+    # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
+    PROC_CREATIONFLAGS = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
+else:
+    PROC_CREATIONFLAGS = 0
 
 
 class Git(LazyMixin):
@@ -551,7 +550,7 @@ def _terminate(self) -> None:
                 # For some reason, providing None for stdout/stderr still prints something. This is why
                 # we simply use the shell and redirect to nul. Slower than CreateProcess. The question
                 # is whether we really want to see all these messages. It's annoying no matter what.
-                if is_win:
+                if os.name == "nt":
                     call(
                         ("TASKKILL /F /T /PID %s 2>nul 1>nul" % str(proc.pid)),
                         shell=True,
@@ -967,7 +966,7 @@ def execute(
         if inline_env is not None:
             env.update(inline_env)
 
-        if is_win:
+        if os.name == "nt":
             cmd_not_found_exception = OSError
             if kill_after_timeout is not None:
                 raise GitCommandError(
@@ -999,11 +998,11 @@ def execute(
                     env=env,
                     cwd=cwd,
                     bufsize=-1,
-                    stdin=istream or DEVNULL,
+                    stdin=(istream or DEVNULL),
                     stderr=PIPE,
                     stdout=stdout_sink,
                     shell=shell,
-                    close_fds=is_posix,  # Unsupported on Windows.
+                    close_fds=(os.name == "posix"),  # Unsupported on Windows.
                     universal_newlines=universal_newlines,
                     creationflags=PROC_CREATIONFLAGS,
                     **subprocess_kwargs,
@@ -1073,7 +1072,7 @@ def kill_process(pid: int) -> None:
                         )
                         if not universal_newlines:
                             stderr_value = stderr_value.encode(defenc)
-                # strip trailing "\n"
+                # Strip trailing "\n".
                 if stdout_value.endswith(newline) and strip_newline_in_stdout:  # type: ignore
                     stdout_value = stdout_value[:-1]
                 if stderr_value.endswith(newline):  # type: ignore
@@ -1147,11 +1146,11 @@ def update_environment(self, **kwargs: Any) -> Dict[str, Union[str, None]]:
         """
         old_env = {}
         for key, value in kwargs.items():
-            # set value if it is None
+            # Set value if it is None.
             if value is not None:
                 old_env[key] = self._environment.get(key)
                 self._environment[key] = value
-            # remove key from environment if its value is None
+            # Remove key from environment if its value is None.
             elif key in self._environment:
                 old_env[key] = self._environment[key]
                 del self._environment[key]
@@ -1330,7 +1329,8 @@ def _parse_object_header(self, header_line: str) -> Tuple[str, str, int]:
         :return: (hex_sha, type_string, size_as_int)
 
         :raise ValueError: If the header contains indication for an error due to
-            incorrect input sha"""
+            incorrect input sha
+        """
         tokens = header_line.split()
         if len(tokens) != 3:
             if not tokens:

Diff for: git/compat.py

@@ -34,9 +34,20 @@
 # ---------------------------------------------------------------------------
 
 
+# DEPRECATED attributes providing shortcuts to operating system checks based on os.name.
+#
+# - is_win and is_posix are deprecated because it is clearer, and helps avoid bugs, to
+#   write out the os.name checks explicitly. For example, is_win is False on Cygwin, but
+#   is often assumed to be True.
+#
+# - is_darwin is deprecated because it is always False on all systems, as os.name is
+#   never "darwin". For macOS, you can check for sys.platform == "darwin". (As on other
+#   Unix-like systems, os.name == "posix" on macOS. This is also the case on Cygwin.)
+#
 is_win: bool = os.name == "nt"
 is_posix = os.name == "posix"
 is_darwin = os.name == "darwin"
+
 defenc = sys.getfilesystemencoding()
 
 

Diff for: git/config.py

@@ -7,28 +7,21 @@
 """Module containing module parser implementation able to properly read and write
 configuration files."""
 
-import sys
 import abc
+import configparser as cp
+import fnmatch
 from functools import wraps
 import inspect
 from io import BufferedReader, IOBase
 import logging
 import os
+import os.path as osp
 import re
-import fnmatch
-
-from git.compat import (
-    defenc,
-    force_text,
-    is_win,
-)
+import sys
 
+from git.compat import defenc, force_text
 from git.util import LockFile
 
-import os.path as osp
-
-import configparser as cp
-
 # typing-------------------------------------------------------
 
 from typing import (
@@ -250,7 +243,7 @@ def items_all(self) -> List[Tuple[str, List[_T]]]:
 def get_config_path(config_level: Lit_config_levels) -> str:
     # We do not support an absolute path of the gitconfig on Windows.
     # Use the global config instead.
-    if is_win and config_level == "system":
+    if os.name == "nt" and config_level == "system":
         config_level = "global"
 
     if config_level == "system":

Diff for: git/index/fun.py

@@ -2,8 +2,9 @@
 # NOTE: Autodoc hates it if this is a docstring.
 
 from io import BytesIO
-from pathlib import Path
 import os
+import os.path as osp
+from pathlib import Path
 from stat import (
     S_IFDIR,
     S_IFLNK,
@@ -16,26 +17,17 @@
 import subprocess
 
 from git.cmd import PROC_CREATIONFLAGS, handle_process_output
-from git.compat import (
-    defenc,
-    force_text,
-    force_bytes,
-    is_posix,
-    is_win,
-    safe_decode,
-)
-from git.exc import UnmergedEntriesError, HookExecutionError
+from git.compat import defenc, force_bytes, force_text, safe_decode
+from git.exc import HookExecutionError, UnmergedEntriesError
 from git.objects.fun import (
-    tree_to_stream,
     traverse_tree_recursive,
     traverse_trees_recursive,
+    tree_to_stream,
 )
 from git.util import IndexFileSHA1Writer, finalize_process
 from gitdb.base import IStream
 from gitdb.typ import str_tree_type
 
-import os.path as osp
-
 from .typ import BaseIndexEntry, IndexEntry, CE_NAMEMASK, CE_STAGESHIFT
 from .util import pack, unpack
 
@@ -96,7 +88,7 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
     env["GIT_EDITOR"] = ":"
     cmd = [hp]
     try:
-        if is_win and not _has_file_extension(hp):
+        if os.name == "nt" and not _has_file_extension(hp):
             # Windows only uses extensions to determine how to open files
             # (doesn't understand shebangs). Try using bash to run the hook.
             relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix()
@@ -108,7 +100,7 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             cwd=index.repo.working_dir,
-            close_fds=is_posix,
+            close_fds=(os.name == "posix"),
             creationflags=PROC_CREATIONFLAGS,
         )
     except Exception as ex:

Diff for: git/index/util.py

@@ -2,15 +2,11 @@
 
 from functools import wraps
 import os
+import os.path as osp
 import struct
 import tempfile
 from types import TracebackType
 
-from git.compat import is_win
-
-import os.path as osp
-
-
 # typing ----------------------------------------------------------------------
 
 from typing import Any, Callable, TYPE_CHECKING, Optional, Type
@@ -58,7 +54,7 @@ def __exit__(
         exc_tb: Optional[TracebackType],
     ) -> bool:
         if osp.isfile(self.tmp_file_path):
-            if is_win and osp.exists(self.file_path):
+            if os.name == "nt" and osp.exists(self.file_path):
                 os.remove(self.file_path)
             os.rename(self.tmp_file_path, self.file_path)
 

Diff for: git/objects/submodule/base.py

@@ -7,7 +7,7 @@
 
 import git
 from git.cmd import Git
-from git.compat import defenc, is_win
+from git.compat import defenc
 from git.config import GitConfigParser, SectionConstraint, cp
 from git.exc import (
     BadName,
@@ -353,9 +353,8 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab
         """
         git_file = osp.join(working_tree_dir, ".git")
         rela_path = osp.relpath(module_abspath, start=working_tree_dir)
-        if is_win:
-            if osp.isfile(git_file):
-                os.remove(git_file)
+        if os.name == "nt" and osp.isfile(git_file):
+            os.remove(git_file)
         with open(git_file, "wb") as fp:
             fp.write(("gitdir: %s" % rela_path).encode(defenc))