Merge pull request #1782 from EliahKagan/shell-doc

Pre-deprecate setting Git.USE_SHELL

Author: Byron

git/cmd.py

@@ -273,23 +273,40 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
 
     # CONFIGURATION
 
-    git_exec_name = "git"  # Default that should work on Linux and Windows.
+    git_exec_name = "git"
+    """Default git command that should work on Linux, Windows, and other systems."""
 
-    # Enables debugging of GitPython's git commands.
     GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
+    """Enables debugging of GitPython's git commands."""
 
-    # If True, a shell will be used when executing git commands.
-    # This should only be desirable on Windows, see https://github.com/gitpython-developers/GitPython/pull/126
-    # and check `git/test_repo.py:TestRepo.test_untracked_files()` TC for an example where it is required.
-    # Override this value using `Git.USE_SHELL = True`.
     USE_SHELL = False
+    """If True, a shell will be used when executing git commands.
+
+    This exists to avoid breaking old code that may access it, but it is no longer
+    needed and should rarely if ever be used. Prior to GitPython 2.0.8, it had a narrow
+    purpose in suppressing console windows in graphical Windows applications. In 2.0.8
+    and higher, it provides no benefit, as GitPython solves that problem more robustly
+    and safely by using the ``CREATE_NO_WINDOW`` process creation flag on Windows.
+
+    Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
+    functions should be updated to use the default value of ``False`` instead. ``True``
+    is unsafe unless the effect of shell expansions is fully considered and accounted
+    for, which is not possible under most circumstances.
+
+    See:
+    - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
+    - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
+    """
 
-    # Provide the full path to the git executable. Otherwise it assumes git is in the path.
     _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE"
     _refresh_env_var = "GIT_PYTHON_REFRESH"
+
     GIT_PYTHON_GIT_EXECUTABLE = None
-    # Note that the git executable is actually found during the refresh step in
-    # the top level __init__.
+    """Provide the full path to the git executable. Otherwise it assumes git is in the path.
+
+    Note that the git executable is actually found during the refresh step in
+    the top level ``__init__``.
+    """
 
     @classmethod
     def refresh(cls, path: Union[None, PathLike] = None) -> bool:

git/config.py

@@ -65,12 +65,14 @@
 log.addHandler(logging.NullHandler())
 
 
-# The configuration level of a configuration file.
 CONFIG_LEVELS: ConfigLevels_Tup = ("system", "user", "global", "repository")
+"""The configuration level of a configuration file."""
 
-# Section pattern to detect conditional includes.
-# https://git-scm.com/docs/git-config#_conditional_includes
 CONDITIONAL_INCLUDE_REGEXP = re.compile(r"(?<=includeIf )\"(gitdir|gitdir/i|onbranch):(.+)\"")
+"""Section pattern to detect conditional includes.
+
+See: https://git-scm.com/docs/git-config#_conditional_includes
+"""
 
 
 class MetaParserBuilder(abc.ABCMeta):  # noqa: B024
@@ -283,12 +285,14 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder):
     """
 
     # { Configuration
-    # The lock type determines the type of lock to use in new configuration readers.
-    # They must be compatible to the LockFile interface.
-    # A suitable alternative would be the BlockingLockFile
     t_lock = LockFile
-    re_comment = re.compile(r"^\s*[#;]")
+    """The lock type determines the type of lock to use in new configuration readers.
 
+    They must be compatible to the LockFile interface.
+    A suitable alternative would be the :class:`~git.util.BlockingLockFile`.
+    """
+
+    re_comment = re.compile(r"^\s*[#;]")
     # } END configuration
 
     optvalueonly_source = r"\s*(?P<option>[^:=\s][^:=]*)"
@@ -299,8 +303,8 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder):
 
     del optvalueonly_source
 
-    # list of RawConfigParser methods able to change the instance
     _mutating_methods_ = ("add_section", "remove_section", "remove_option", "set")
+    """List of RawConfigParser methods able to change the instance."""
 
     def __init__(
         self,

git/diff.py

@@ -49,8 +49,8 @@
 
 __all__ = ("Diffable", "DiffIndex", "Diff", "NULL_TREE")
 
-# Special object to compare against the empty tree in diffs.
 NULL_TREE = object()
+"""Special object to compare against the empty tree in diffs."""
 
 _octal_byte_re = re.compile(rb"\\([0-9]{3})")
 

git/index/base.py

@@ -142,9 +142,11 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
 
     __slots__ = ("repo", "version", "entries", "_extension_data", "_file_path")
 
-    _VERSION = 2  # Latest version we support.
+    _VERSION = 2
+    """The latest version we support."""
 
-    S_IFGITLINK = S_IFGITLINK  # A submodule.
+    S_IFGITLINK = S_IFGITLINK
+    """Flags for a submodule."""
 
     def __init__(self, repo: "Repo", file_path: Union[PathLike, None] = None) -> None:
         """Initialize this Index instance, optionally from the given ``file_path``.

git/index/fun.py

@@ -49,7 +49,9 @@
 # ------------------------------------------------------------------------------------
 
 
-S_IFGITLINK = S_IFLNK | S_IFDIR  # A submodule.
+S_IFGITLINK = S_IFLNK | S_IFDIR
+"""Flags for a submodule."""
+
 CE_NAMEMASK_INV = ~CE_NAMEMASK
 
 __all__ = (

git/objects/submodule/base.py

@@ -77,9 +77,9 @@ class UpdateProgress(RemoteProgress):
 UPDWKTREE = UpdateProgress.UPDWKTREE
 
 
-# IndexObject comes via util module, its a 'hacky' fix thanks to pythons import
-# mechanism which cause plenty of trouble of the only reason for packages and
-# modules is refactoring - subpackages shouldn't depend on parent packages
+# IndexObject comes via the util module. It's a 'hacky' fix thanks to Python's import
+# mechanism, which causes plenty of trouble if the only reason for packages and
+# modules is refactoring - subpackages shouldn't depend on parent packages.
 class Submodule(IndexObject, TraversableIterableObj):
     """Implements access to a git submodule. They are special in that their sha
     represents a commit in the submodule's repository which is to be checked out
@@ -95,10 +95,11 @@ class Submodule(IndexObject, TraversableIterableObj):
     k_modules_file = ".gitmodules"
     k_head_option = "branch"
     k_head_default = "master"
-    k_default_mode = stat.S_IFDIR | stat.S_IFLNK  # Submodules are directories with link-status.
+    k_default_mode = stat.S_IFDIR | stat.S_IFLNK
+    """Submodule flags. Submodules are directories with link-status."""
 
-    # This is a bogus type for base class compatibility.
     type: Literal["submodule"] = "submodule"  # type: ignore
+    """This is a bogus type for base class compatibility."""
 
     __slots__ = ("_parent_commit", "_url", "_branch_path", "_name", "__weakref__")
 

git/repo/base.py

@@ -114,17 +114,18 @@ class Repo:
     'working_tree_dir' is the working tree directory, but will return None
     if we are a bare repository.
 
-    'git_dir' is the .git repository directory, which is always set."""
+    'git_dir' is the .git repository directory, which is always set.
+    """
 
     DAEMON_EXPORT_FILE = "git-daemon-export-ok"
 
-    git = cast("Git", None)  # Must exist, or  __del__  will fail in case we raise on `__init__()`
+    git = cast("Git", None)  # Must exist, or  __del__  will fail in case we raise on `__init__()`.
     working_dir: PathLike
     _working_tree_dir: Optional[PathLike] = None
     git_dir: PathLike
     _common_dir: PathLike = ""
 
-    # precompiled regex
+    # Precompiled regex
     re_whitespace = re.compile(r"\s+")
     re_hexsha_only = re.compile(r"^[0-9A-Fa-f]{40}$")
     re_hexsha_shortened = re.compile(r"^[0-9A-Fa-f]{4,40}$")
@@ -133,24 +134,32 @@ class Repo:
     re_tab_full_line = re.compile(r"^\t(.*)$")
 
     unsafe_git_clone_options = [
-        # This option allows users to execute arbitrary commands.
-        # https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---upload-packltupload-packgt
+        # Executes arbitrary commands:
         "--upload-pack",
         "-u",
-        # Users can override configuration variables
-        # like `protocol.allow` or `core.gitProxy` to execute arbitrary commands.
-        # https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt
+        # Can override configuration variables that execute arbitrary commands:
         "--config",
         "-c",
     ]
+    """Options to ``git clone`` that allow arbitrary commands to be executed.
 
-    # invariants
-    # represents the configuration level of a configuration file
+    The ``--upload-pack``/``-u`` option allows users to execute arbitrary commands
+    directly:
+    https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---upload-packltupload-packgt
+
+    The ``--config``/``-c`` option allows users to override configuration variables like
+    ``protocol.allow`` and ``core.gitProxy`` to execute arbitrary commands:
+    https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt
+    """
+
+    # Invariants
     config_level: ConfigLevels_Tup = ("system", "user", "global", "repository")
+    """Represents the configuration level of a configuration file."""
 
     # Subclass configuration
-    # Subclasses may easily bring in their own custom types by placing a constructor or type here
     GitCommandWrapperType = Git
+    """Subclasses may easily bring in their own custom types by placing a constructor or
+    type here."""
 
     def __init__(
         self,

git/util.py

@@ -557,8 +557,8 @@ class RemoteProgress:
         "_cur_line",
         "_seen_ops",
         "error_lines",  # Lines that started with 'error:' or 'fatal:'.
-        "other_lines",
-    )  # Lines not denoting progress (i.e.g. push-infos).
+        "other_lines",  # Lines not denoting progress (i.e.g. push-infos).
+    )
     re_op_absolute = re.compile(r"(remote: )?([\w\s]+):\s+()(\d+)()(.*)")
     re_op_relative = re.compile(r"(remote: )?([\w\s]+):\s+(\d+)% \((\d+)/(\d+)\)(.*)")