From 5fd795a4981d02c1cd9050e41793b3dc64ff2438 Mon Sep 17 00:00:00 2001 From: Roshan Ramani Date: Mon, 7 Sep 2026 12:10:23 +0530 Subject: [PATCH 01/19] Read git's yes/no and on/off booleans in get_value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git accepts yes/no and on/off for a boolean as well as true/false (git_parse_maybe_bool_text in parse.c), and ConfigParser.getboolean on this class already accepted all of them. _string_to_value handled only true/false, under a comment claiming to "try boolean values as git uses them", so the two accessors disagreed about the same file: value get_value() getboolean() git config --type=bool yes 'yes' True true no 'no' False false on 'on' True true off 'off' False false Returning them as strings was worse than merely inexact. "no" and "off" are non-empty, so a caller testing the result of get_value got True for a value git reads as false — the inversion is silent, since nothing raises. _string_to_value now recognises the same spellings getboolean does. A value that is not a boolean is untouched, so "meld" is still returned as a string, and numeric values keep their existing behavior. Not changed here: get_value also diverges on numeric bases and suffixes ("0x10" and "1k" come back as strings, "010" as 10 where git reads octal 8). Those change the value rather than its type and are worth their own commit. Validation: test_get_value_reads_git_boolean_spellings covers the ten spellings and asserts the two accessors agree; it fails on the previous revision. test/test_config.py passes (40 passed, 2 skipped), ruff check and ruff format are clean. test/test_repo.py errors on this clone because init-tests-after-clone.sh has not been run, unchanged by this commit. --- git/config.py | 11 ++++++++--- test/test_config.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/git/config.py b/git/config.py index aef881d2e..bb7fda19f 100644 --- a/git/config.py +++ b/git/config.py @@ -956,11 +956,16 @@ def _string_to_value(self, valuestr: str) -> Union[int, float, str, bool]: continue # END for each numeric type - # Try boolean values as git uses them. + # Try boolean values as git uses them. git accepts yes/no and on/off as + # well as true/false (git_parse_maybe_bool_text in parse.c), and so does + # ConfigParser.getboolean on this class, so only get_value lagged behind. + # Leaving them as strings was worse than merely inexact: "no" and "off" + # are non-empty, so a caller testing the result got True for a value git + # reads as false. vl = valuestr.lower() - if vl == "false": + if vl in ("false", "no", "off"): return False - if vl == "true": + if vl in ("true", "yes", "on"): return True if not isinstance(valuestr, str): diff --git a/test/test_config.py b/test/test_config.py index 3107d8074..3031721de 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -167,6 +167,38 @@ def test_backslash_line_continuation(self): key = "co" if section == "alias" else "k" self.assertEqual(config.get_value(section, key), expected) + def test_get_value_reads_git_boolean_spellings(self): + # git accepts yes/no and on/off as well as true/false, and getboolean on + # this class already did. get_value returned them as strings, so "no" and + # "off" arrived as non-empty (truthy) values for a caller testing them. + cases = [ + (b"true", True), + (b"TRUE", True), + (b"yes", True), + (b"Yes", True), + (b"on", True), + (b"On", True), + (b"false", False), + (b"no", False), + (b"off", False), + (b"Off", False), + ] + for raw, expected in cases: + config_file = io.BytesIO(b"[core]\n\tflag = " + raw + b"\n") + config_file.name = "boolean_spellings.config" + config = GitConfigParser(config_file) + config.read() + self.assertIs(config.get_value("core", "flag"), expected, raw.decode()) + # The two accessors must not disagree about the same value. + self.assertIs(config.getboolean("core", "flag"), expected, raw.decode()) + + # A value that is not a boolean at all still comes back untouched. + config_file = io.BytesIO(b"[core]\n\tflag = meld\n") + config_file.name = "boolean_spellings.config" + config = GitConfigParser(config_file) + config.read() + self.assertEqual(config.get_value("core", "flag"), "meld") + @with_rw_directory def test_comment_backslash_does_not_continue_value(self, rw_dir): config_path = osp.join(rw_dir, "config") From 297cd26412e63c1f94c3ef8bdcf27c54956328ab Mon Sep 17 00:00:00 2001 From: Byron Date: Mon, 7 Sep 2026 11:26:45 +0200 Subject: [PATCH 02/19] review --- git/config.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/git/config.py b/git/config.py index bb7fda19f..0da90bab2 100644 --- a/git/config.py +++ b/git/config.py @@ -10,36 +10,34 @@ 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 sys - -from git.compat import defenc, force_text -from git.util import LockFile +from functools import wraps +from io import BufferedReader, IOBase # typing------------------------------------------------------- - from typing import ( + IO, + TYPE_CHECKING, Any, Callable, + Dict, Generic, - IO, List, - Dict, Sequence, - TYPE_CHECKING, Tuple, TypeVar, Union, cast, ) -from git.types import Lit_config_levels, ConfigLevels_Tup, PathLike, assert_never, _T +from git.compat import defenc, force_text +from git.types import _T, ConfigLevels_Tup, Lit_config_levels, PathLike, assert_never +from git.util import LockFile if TYPE_CHECKING: from io import BytesIO @@ -956,12 +954,7 @@ def _string_to_value(self, valuestr: str) -> Union[int, float, str, bool]: continue # END for each numeric type - # Try boolean values as git uses them. git accepts yes/no and on/off as - # well as true/false (git_parse_maybe_bool_text in parse.c), and so does - # ConfigParser.getboolean on this class, so only get_value lagged behind. - # Leaving them as strings was worse than merely inexact: "no" and "off" - # are non-empty, so a caller testing the result got True for a value git - # reads as false. + # Try boolean values as git uses them. vl = valuestr.lower() if vl in ("false", "no", "off"): return False From 6bcb25b80071cfc59c72f2bbb03318a87f769a39 Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 9 Sep 2026 18:43:25 +0200 Subject: [PATCH 03/19] Clarify detached HEAD access and attach writable test fixtures (#2230) Reading head.reference or active_branch raises TypeError when HEAD points directly to a commit, but the public documentation did not clearly explain how to access that commit. Document head.commit.hexsha for attached and detached HEADs, explain the reference setter/getter asymmetry, and clarify that active_branch requires an attached HEAD. Preserve the exception type and existing message prefix while adding a hint to use .commit or .object. Writable test fixtures assumed cloning produced an attached HEAD, so their branch access could fail when the source checkout was detached. Have with_rw_repo create and attach master at the requested revision when its clone is detached, retaining the clone's branch and tracking configuration otherwise. Explicitly attach the temporary bare remote to its own master branch before cloning it for remote tests. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- doc/source/tutorial.rst | 8 +++++++- git/refs/symbolic.py | 37 +++++++++++++++++++++++++++++++------ git/repo/base.py | 6 +++++- test/lib/helper.py | 11 +++++++++-- 4 files changed, 52 insertions(+), 10 deletions(-) diff --git a/doc/source/tutorial.rst b/doc/source/tutorial.rst index d095d3be3..1edfa4e11 100644 --- a/doc/source/tutorial.rst +++ b/doc/source/tutorial.rst @@ -78,6 +78,12 @@ Query relevant repository paths ... :class:`Heads ` Heads are branches in git-speak. :class:`References ` are pointers to a specific commit or to other references. Heads and :class:`Tags ` are a kind of references. GitPython allows you to query them rather intuitively. +To obtain the current commit ID, use ``repo.head.commit.hexsha``. This works both +on a branch and with a detached HEAD, provided HEAD resolves to an existing commit. +When ``repo.head.is_detached`` is true, HEAD points directly to a commit and there +is no active branch: reading ``repo.head.reference`` or ``repo.active_branch`` +raises :exc:`TypeError`. The branch examples below assume an attached HEAD. + .. literalinclude:: ../../test/test_docs.py :language: python :dedent: 8 @@ -152,7 +158,7 @@ Examining References :start-after: # [2-test_references_and_objects] :end-before: # ![2-test_references_and_objects] -A :class:`symbolic reference ` is a special case of a reference as it points to another reference instead of a commit. +A :class:`symbolic reference ` can point to another reference. When detached, it points directly to a commit instead. Reading its ``commit`` property resolves the commit in either state. Assigning a commit to ``reference`` detaches it; reading ``reference`` then raises :exc:`TypeError`. .. literalinclude:: ../../test/test_docs.py :language: python diff --git a/git/refs/symbolic.py b/git/refs/symbolic.py index 824d0c46c..7c3eac508 100644 --- a/git/refs/symbolic.py +++ b/git/refs/symbolic.py @@ -59,12 +59,14 @@ def _git_dir(repo: "Repo", path: Union[PathLike, None]) -> PathLike: class SymbolicReference: - """Special case of a reference that is symbolic. + """A reference that can point to another reference or be detached. - This does not point to a specific commit, but to another - :class:`~git.refs.head.Head`, which itself specifies a commit. + An attached :class:`~git.refs.head.HEAD` usually points to a + :class:`~git.refs.head.Head`, which itself specifies a commit. A detached + :class:`~git.refs.head.HEAD` points directly to a commit instead. - A typical example for a symbolic reference is :class:`~git.refs.head.HEAD`. + Use :attr:`commit` to access the commit in either case, and :attr:`reference` + to access the target reference when attached. """ __slots__ = ("repo", "path") @@ -416,7 +418,15 @@ def set_object( @property def commit(self) -> "Commit": - """Query or set commits directly""" + """The commit this reference resolves to, whether detached or symbolic. + + For example, ``repo.head.commit.hexsha`` returns the current commit ID + both on a branch and with a detached HEAD. HEAD must resolve to an + existing commit; an unborn branch in an empty repository has none. + + Assigning updates the commit without changing whether this reference + is detached. + """ return self._get_commit() @commit.setter @@ -443,7 +453,10 @@ def _get_reference(self) -> "Reference": """ sha, target_ref_path = self._get_ref_info(self.repo, self.path) if target_ref_path is None: - raise TypeError("%s is a detached symbolic reference as it points to %r" % (self, sha)) + raise TypeError( + "%s is a detached symbolic reference as it points to %r. " + "Use .commit or .object to access the target directly." % (self, sha) + ) return cast("Reference", self.from_path(self.repo, target_ref_path)) def set_reference( @@ -531,6 +544,18 @@ def set_reference( # Aliased reference @property def reference(self) -> "Reference": + """The reference we point to, available only when not detached. + + Check :attr:`is_detached` before reading this property if a target + reference is required. To access the target commit or object in either + state, use :attr:`commit` or :attr:`object` instead. + + Assigning a reference keeps this reference symbolic. Assigning a git + object or revision string detaches it; reading this property then raises. + + :raise TypeError: + If this reference is detached when reading the property. + """ return self._get_reference() @reference.setter diff --git a/git/repo/base.py b/git/repo/base.py index 7039d0320..f326266d6 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1150,7 +1150,11 @@ def ignored(self, *paths: PathLike) -> List[str]: @property def active_branch(self) -> Head: - """The name of the currently active branch. + """The currently active branch. + + Check ``repo.head.is_detached`` before accessing this property if HEAD + may be detached. To access the current commit in either state, use + ``repo.head.commit`` instead. :raise TypeError: If HEAD is detached. diff --git a/test/lib/helper.py b/test/lib/helper.py index 4135fe5dd..b4399ca62 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -139,7 +139,8 @@ def wrapper(self, *args, **kwargs): def with_rw_repo(working_tree_ref, bare=False): """Same as with_bare_repo, but clones the rorepo as non-bare repository, checking - out the working tree at the given working_tree_ref. + out the working tree at the given working_tree_ref with an attached HEAD, + regardless of the source repository's HEAD state. This repository type is more costly due to the working copy checkout. @@ -158,7 +159,12 @@ def repo_creator(self): repo_dir = tempfile.mktemp(prefix="%sbare_%s" % (prefix, func.__name__)) rw_repo = self.rorepo.clone(repo_dir, shared=True, bare=bare, n=True) - rw_repo.head.commit = rw_repo.commit(working_tree_ref) + if rw_repo.head.is_detached: + rw_repo.head.reference = rw_repo.create_head( + "master", working_tree_ref, force=True, logmsg="Create test branch" + ) + else: + rw_repo.head.commit = rw_repo.commit(working_tree_ref) if not bare: rw_repo.head.reference.checkout() # END handle checkout @@ -294,6 +300,7 @@ def remote_repo_creator(self): rw_repo_dir = tempfile.mktemp(prefix="daemon_cloned_repo-%s-" % func.__name__) rw_daemon_repo = self.rorepo.clone(rw_daemon_repo_dir, shared=True, bare=True) + rw_daemon_repo.head.reference = rw_daemon_repo.create_head("master", force=True) # Recursive alternates info? rw_repo = rw_daemon_repo.clone(rw_repo_dir, shared=True, bare=False, n=True) try: From 24b6f95d4b90ab1483fb601c736ac6e7c059cd61 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 04:59:45 +0200 Subject: [PATCH 04/19] fix: Reject submodule move destinations through intermediate symlinks Also, sloppy review of the tests which are assumpted to not make things worse. Submodule.move() checked lexical containment but did not validate intermediate destination components before filesystem and repository updates. GHSA-gq48-pqfc-9p58 identifies the resulting checkout-path boundary violation. The new regression failed before the fix because move() returned successfully. Share the existing abspath component walk with move() and validate the normalized destination before any mutation, including configuration-only and module-only calls. Preserve the no-op early return and existing final-component symlink handling; abspath still rejects every symlink component. This addresses pre-existing links, not concurrent directory replacement races. The Git reference checkout at 1630431f326e15fcde608827b5ff38422528eb59 uses has_symlink_leading_path() in builtin/mv.c and tests rejection without index changes in t/t7001-mv.sh. The fix follows that intermediate-component rule while retaining GitPython leaf-link compatibility. Validation: the 30 new parameterized cases pass, covering relative and absolute destinations and link targets, internal and dangling links, all move flag combinations, unchanged repository state after rejection, ordinary and no-op moves, and leaf-link compatibility. The complete test/test_submodule.py suite passes: 75 passed, 3 skipped, 1 xfailed. Test-process commit.gpgsign=false avoids sandbox GPG failures. Ruff lint and format checks, mypy (45 source files), and git diff --check pass. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- doc/source/changes.rst | 13 ++++ git/objects/submodule/base.py | 20 ++++-- test/test_submodule.py | 125 ++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 5 deletions(-) diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 20bfaeae6..4803790b9 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,19 @@ Changelog ========= +3.1.63 +====== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58 + +If you can, also try and provide feedback on the upcoming v4 branch +https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.1.63 + 3.1.62 ====== diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 563b20a18..5314ccf62 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -419,11 +419,20 @@ def abspath(self) -> PathLike: root = self.repo.working_tree_dir if root is None: return super().abspath - path = root - for component in os.fspath(self._to_relative_path(self.repo, self.path)).split("/"): + return self._checkout_abspath(self._to_relative_path(self.repo, self.path)) + + def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike: + """Check a checkout path already normalized by :meth:`_to_relative_path`.""" + path = self.repo.working_tree_dir + if path is None: + raise NotADirectoryError("Submodules require a working tree") + components = os.fspath(relative_path).split("/") + for index, component in enumerate(components): path = join_path_native(path, component) + if allow_final_symlink and index == len(components) - 1: + break if osp.islink(path): - raise ValueError("Submodule checkout path %r contains a symbolic link" % self.path) + raise ValueError("Submodule checkout path %r contains a symbolic link" % relative_path) return path @classmethod @@ -1039,7 +1048,8 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = self :raise ValueError: - If the module path existed and was not empty, or was a file. + If the module path existed and was not empty, was a file, or had a + symbolic link in an intermediate component. :note: Currently the method is not atomic, and it could leave the repository in an @@ -1057,7 +1067,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = return self # END handle no change - module_checkout_abspath = join_path_native(str(self.repo.working_tree_dir), module_checkout_path) + module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True) if osp.isfile(module_checkout_abspath): raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath) # END handle target files diff --git a/test/test_submodule.py b/test/test_submodule.py index ca9078aac..0d0275f21 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -51,6 +51,131 @@ def _patch_git_config(name, value): yield +@pytest.fixture +def movable_submodule(tmp_path): + """Create a committed local submodule whose logical name stays fixed when moved.""" + with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: + (tmp_path / "source" / "file").write_text("content", encoding="utf-8") + source.index.add(["file"]) + source.index.commit("Create source") + with _patch_git_config("protocol.file.allow", "always"): + submodule = parent.create_submodule("logical-name", "module", source.working_tree_dir) + parent.index.commit("Create submodule") + # Release clone handles before Windows moves the checkout. + submodule.module().close() + yield submodule + + +def _move_snapshot(submodule): + """Capture index, configuration, and path state to detect side effects of rejected moves.""" + parent = submodule.repo + with submodule.module() as module: + config = Path(module.git_dir, "config").read_bytes() + return ( + Path(parent.index.path).read_bytes(), + Path(parent.working_tree_dir, ".gitmodules").read_bytes(), + Path(submodule.abspath, ".git").read_bytes(), + config, + submodule.path, + ) + + +@pytest.mark.parametrize("target_kind", ["relative", "absolute", "internal", "dangling"]) +@pytest.mark.parametrize("configuration,module", [(True, True), (False, True), (True, False)]) +@pytest.mark.parametrize("absolute_path", [False, True]) +def test_move_rejects_intermediate_symlink( + movable_submodule, tmp_path, target_kind, configuration, module, absolute_path +): + """Reject intermediate symlinks before changing repository state or their targets. + + Cover relative and absolute destinations in every move mode, including links + within the repository and dangling links, which must also be rejected. + """ + submodule = movable_submodule + parent = submodule.repo + root = Path(parent.working_tree_dir) + target = root / "target" if target_kind == "internal" else tmp_path / "outside" + if target_kind != "dangling": + target.mkdir() + (root / "nested").mkdir() + link = root / "nested" / "link" + link.symlink_to( + target if target_kind == "absolute" else os.path.relpath(target, link.parent), target_is_directory=True + ) + parent.index.add(["nested/link"]) + parent.index.commit("Record layout") + tree = parent.git.write_tree() + before = _move_snapshot(submodule) + destination = root / "nested/link/new/moved" if absolute_path else "nested/link/new/moved" + + with pytest.raises(ValueError, match="contains a symbolic link"): + submodule.move(destination, configuration=configuration, module=module) + + assert _move_snapshot(submodule) == before + assert parent.git.write_tree() == tree + assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content" + if target_kind == "dangling": + assert not target.exists() + else: + assert list(target.iterdir()) == [] + + +@pytest.mark.parametrize("absolute_path", [False, True]) +def test_move_normal_destination(movable_submodule, absolute_path): + """Allow ordinary relative and absolute moves, and make a repeated move a no-op.""" + submodule = movable_submodule + root = Path(submodule.repo.working_tree_dir) + destination = root / "nested/moved" if absolute_path else "nested/moved" + assert submodule.move(destination) is submodule + assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content" + assert not (root / "module").exists() + assert submodule.path == "nested/moved" + submodule.repo.git.write_tree() + before = _move_snapshot(submodule) + assert submodule.move(destination) is submodule + assert _move_snapshot(submodule) == before + + +@pytest.mark.parametrize("kind", ["empty", "nonempty", "file", "dangling"]) +def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind): + """Preserve leaf-symlink replacement without modifying the external target. + + Moving onto a link to an empty directory replaces the link; nonempty, file, + and dangling targets fail without changing repository state. Direct checkout + path access must still reject every leaf symlink. + """ + submodule = movable_submodule + root = Path(submodule.repo.working_tree_dir) + target = tmp_path / "outside" + if kind in ("empty", "nonempty"): + target.mkdir() + if kind == "nonempty": + (target / "keep").write_text("keep", encoding="utf-8") + if kind == "file": + target.write_text("keep", encoding="utf-8") + destination = root / "destination" + destination.symlink_to(target, target_is_directory=kind != "file") + with pytest.raises(ValueError, match="contains a symbolic link"): + Submodule(submodule.repo, Submodule.NULL_BIN_SHA, name="unused", path="destination").abspath + before = _move_snapshot(submodule) + if kind == "empty": + assert submodule.move("destination") is submodule + assert not destination.is_symlink() + assert (destination / "file").is_file() + assert list(target.iterdir()) == [] + else: + with pytest.raises(OSError if kind == "dangling" else ValueError): + submodule.move("destination") + assert _move_snapshot(submodule) == before + assert destination.is_symlink() + if kind == "nonempty": + assert (target / "keep").read_text(encoding="utf-8") == "keep" + elif kind == "file": + assert target.read_text(encoding="utf-8") == "keep" + else: + assert not target.exists() + + class TestRootProgress(RootUpdateProgress): """Just prints messages, for now without checking the correctness of the states""" From 43a43cd800200f75ae84957f37b0e8b27c52f403 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 06:52:24 +0200 Subject: [PATCH 05/19] fix: Validate submodule checkout and metadata paths before mutation A bit of a sloppy review, rubber-stamping the tests based on the assumption that they are validating it's conforming to Git, probably also while increasing coverage. Submodule.add() could clone through a checkout symlink after module_exists() swallowed the validation error. Metadata paths had a similar gap: locally planted symlinks under .git/modules could redirect cloning, reconnecting, renaming, updating, or removing a submodule. Some failures were detected only after changing configuration or moving or removing checkout directories. Reuse the checkout component check for metadata paths and validate checkout paths in the shared clone helper, including legacy embedded repositories. Check .gitfiles, submodule configuration files, and the actual repository path named by a gitfile, which can differ from .git/modules/. Reject symlinked .gitmodules files as well. Preflight move and rename sources and destinations before mutation, including the implicit metadata rename when a default-named submodule moves. Keep module_exists()'s boolean contract and the existing supported replacement of a leaf symlink during a move. Add 56 regression cases covering checkout and metadata links, dangling links, redirected gitfiles, legacy clone layouts, and rejected operations preserving external targets, configuration, the index, and an empty move destination. The initial 36 cases reproduced failures before the fix. These checks reject existing symlinks; they do not prevent concurrent filesystem replacement between validation and use. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/objects/submodule/base.py | 49 +++++++--- test/test_submodule.py | 165 ++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 14 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 5314ccf62..461b3068d 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -255,7 +255,7 @@ def _config_parser( # END handle parent_commit fp_module: Union[str, BytesIO] if not repo.bare and parent_matches_head and repo.working_tree_dir: - fp_module = osp.join(repo.working_tree_dir, cls.k_modules_file) + fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file) else: assert parent_commit is not None, "need valid parent_commit in bare repositories" try: @@ -322,7 +322,7 @@ def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> Path if cls._need_gitfile_submodules(parent_repo.git): return osp.join(parent_repo.git_dir, "modules", name) if parent_repo.working_tree_dir: - return osp.join(parent_repo.working_tree_dir, path) + return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path)) raise NotADirectoryError() @classmethod @@ -361,8 +361,11 @@ def _clone_repo( :param kwargs: Additional arguments given to :manpage:`git-clone(1)`. """ + path = cls._to_relative_path(repo, path) + if repo.working_tree_dir is None: + raise NotADirectoryError("Submodules require a working tree") + module_checkout_path = cls._checked_abspath(repo.working_tree_dir, path) module_abspath = cls._module_abspath(repo, path, name) - module_checkout_path = module_abspath if cls._need_gitfile_submodules(repo.git): if not allow_unsafe_options: Git.check_unsafe_options(Git._option_candidates([], kwargs), repo.unsafe_git_clone_options) @@ -377,7 +380,6 @@ def _clone_repo( module_abspath_dir = osp.dirname(module_abspath) if not osp.isdir(module_abspath_dir): os.makedirs(module_abspath_dir) - module_checkout_path = osp.join(repo.working_tree_dir, path) # type: ignore[arg-type] if url.startswith("../"): remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name @@ -423,16 +425,23 @@ def abspath(self) -> PathLike: def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike: """Check a checkout path already normalized by :meth:`_to_relative_path`.""" - path = self.repo.working_tree_dir - if path is None: + return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink) + + @classmethod + def _checked_abspath( + cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False + ) -> str: + """Reject symlinks below a trusted root before accessing submodule paths.""" + if root is None: raise NotADirectoryError("Submodules require a working tree") - components = os.fspath(relative_path).split("/") + path = os.fspath(root) + components = to_native_path_linux(relative_path).split("/") for index, component in enumerate(components): - path = join_path_native(path, component) + path = os.fspath(join_path_native(path, component)) if allow_final_symlink and index == len(components) - 1: break if osp.islink(path): - raise ValueError("Submodule checkout path %r contains a symbolic link" % relative_path) + raise ValueError("Submodule path %r contains a symbolic link" % relative_path) return path @classmethod @@ -458,14 +467,18 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab :param module_abspath: Absolute path to the bare repository. """ + # Git resolves metadata symlinks before interpreting core.worktree. + module_abspath = osp.realpath(module_abspath) + working_tree_dir = osp.realpath(working_tree_dir) git_file = osp.join(working_tree_dir, ".git") + module_config = osp.join(module_abspath, "config") rela_path = osp.relpath(module_abspath, start=working_tree_dir) if sys.platform == "win32" and osp.isfile(git_file): os.remove(git_file) with open(git_file, "wb") as fp: fp.write(("gitdir: %s" % rela_path).encode(defenc)) - with GitConfigParser(osp.join(module_abspath, "config"), read_only=False, merge_includes=False) as writer: + with GitConfigParser(module_config, read_only=False, merge_includes=False) as writer: writer.set_value( "core", "worktree", @@ -576,6 +589,8 @@ def add( name, url="invalid-temporary", ) + cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file) + sm._checkout_abspath(path) if sm.exists(): # Reretrieve submodule from tree. try: @@ -1067,6 +1082,10 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = return self # END handle no change + if configuration: + self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) + # Validate the source before removing the destination. + cur_path = self.abspath module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True) if osp.isfile(module_checkout_abspath): raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath) @@ -1099,7 +1118,6 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = # END handle module # Move the module into place if possible. - cur_path = self.abspath renamed_module = False if module and osp.exists(cur_path): os.renames(cur_path, module_checkout_abspath) @@ -1201,6 +1219,8 @@ def remove( # END handle parameters self._validated_name(self.name) + if configuration: + self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # Recursively remove children of this submodule. nc = 0 for csm in self.children(): @@ -1219,7 +1239,7 @@ def remove( ################################ if module and self.module_exists(): mod = self.module() - git_dir = mod.git_dir + git_dir = osp.realpath(mod.git_dir) if force: # Take the fast lane and just delete everything in our module path. # TODO: If we run into permission problems, we have a highly @@ -1460,6 +1480,9 @@ def rename(self, new_name: str) -> "Submodule": self._validated_name(self.name) self._validated_name(new_name) + destination_module_abspath = self._module_abspath(self.repo, self.path, new_name) + mod = self.module() + self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # .git/config with self.repo.config_writer() as pw: @@ -1476,9 +1499,7 @@ def rename(self, new_name: str) -> "Submodule": self._name = new_name # .git/modules - mod = self.module() if mod.has_separate_working_tree(): - destination_module_abspath = self._module_abspath(self.repo, self.path, new_name) source_dir = mod.git_dir # Let's be sure the submodule name is not so obviously tied to a directory. if str(destination_module_abspath).startswith(str(mod.git_dir)): diff --git a/test/test_submodule.py b/test/test_submodule.py index 0d0275f21..1fcf3ae23 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -176,6 +176,171 @@ def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind): assert not target.exists() +@pytest.mark.parametrize("leaf", [False, True]) +@pytest.mark.parametrize("dangling", [False, True]) +@pytest.mark.parametrize("operation", ["add", "clone"]) +@pytest.mark.parametrize("gitfile", [False, True]) +def test_clone_rejects_checkout_symlinks(movable_submodule, tmp_path, leaf, dangling, operation, gitfile): + """Reject checkout symlinks before add or clone creates metadata or touches the target. + + Cover leaf and intermediate links, including dangling targets, with both + embedded and separate Git directories. + """ + sm = movable_submodule + root = Path(sm.repo.working_tree_dir) + target = tmp_path / "outside" + if not dangling: + target.mkdir() + (root / "link").symlink_to(target, target_is_directory=True) + path = "link" if leaf else "link/new/module" + before = _move_snapshot(sm) + with mock.patch.object(Submodule, "_need_gitfile_submodules", return_value=gitfile): + with pytest.raises(ValueError, match="contains a symbolic link"): + if operation == "add": + Submodule.add(sm.repo, "new", path, sm.url) + else: + Submodule._clone_repo(sm.repo, sm.url, path, "new") + assert _move_snapshot(sm) == before + assert not (Path(sm.repo.git_dir) / "modules/new").exists() + assert not target.exists() if dangling else list(target.iterdir()) == [] + + +@pytest.mark.parametrize("link_kind", ["gitmodules", "checkout"]) +@pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"]) +def test_submodule_rejects_checkout_and_gitmodules_symlinks(movable_submodule, tmp_path, link_kind, operation): + """Reject operations on symlinked checkouts or .gitmodules without side effects. + + Update, move, rename, and forced removal must preserve the external target, + repository configuration, index, checkout, and any existing move destination. + """ + sm = movable_submodule + sm.rename("nested/module") + root = Path(sm.repo.working_tree_dir) + path = root / (".gitmodules" if link_kind == "gitmodules" else "module") + target = tmp_path / "outside" + path.rename(target) + path.symlink_to(target, target_is_directory=target.is_dir()) + before = ( + {p.relative_to(target): p.read_bytes() for p in target.rglob("*") if p.is_file()} + if target.is_dir() + else target.read_bytes() + ) + config = Path(sm.repo.git_dir, "config").read_bytes() + index = Path(sm.repo.index.path).read_bytes() + gitmodules = (root / ".gitmodules").read_bytes() + (root / "moved").mkdir() + with pytest.raises(ValueError, match="contains a symbolic link"): + if operation == "update": + sm.update() + elif operation == "move": + sm.move("moved") + elif operation == "rename": + sm.rename("renamed") + else: + sm.remove(force=True) + after = ( + {p.relative_to(target): p.read_bytes() for p in target.rglob("*") if p.is_file()} + if target.is_dir() + else target.read_bytes() + ) + assert after == before + assert Path(sm.repo.git_dir, "config").read_bytes() == config + assert Path(sm.repo.index.path).read_bytes() == index + assert (root / ".gitmodules").read_bytes() == gitmodules + assert (root / "module/file").read_text() == "content" + assert path.is_symlink() + assert (root / "moved").is_dir() + + +def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path): + """Allow adding and moving submodules when the parent is opened through a symlink.""" + sm = movable_submodule + alias = tmp_path / "alias" + alias.symlink_to(sm.repo.working_tree_dir, target_is_directory=True) + with git.Repo(alias) as parent: + added = Submodule.add(parent, "new", "new", sm.url) + added.move("moved") + with added.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(added.abspath).resolve() + assert Path(added.abspath, "file").read_text() == "content" + + +@pytest.mark.parametrize("operation", ["add", "reconnect", "rename"]) +def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation): + """Allow linked metadata destinations while keeping the checkout correctly connected. + + Adding, reconnecting after deinit, and renaming may store metadata outside the + parent repository through a symlink under .git/modules, preserving that link. + """ + sm = movable_submodule + root = Path(sm.repo.working_tree_dir) + outside = tmp_path / "outside" + outside.mkdir() + link = Path(sm.repo.git_dir) / "modules/link" + link.symlink_to(outside, target_is_directory=True) + if operation == "rename": + sm.rename("link/new") + else: + sm = Submodule.add(sm.repo, "link/new", "new", sm.url) + if operation == "reconnect": + sm.repo.index.commit("Add linked metadata submodule") + sm.repo.git.submodule("deinit", "--force", "new") + sm.update(init=True) + assert link.is_symlink() + assert (outside / "new/HEAD").is_file() + with sm.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve() + assert Path(sm.abspath, "file").read_text() == "content" + assert (root / ".gitmodules").is_file() + + +@pytest.mark.parametrize("kind", ["modules", "intermediate", "leaf", "gitfile", "config", "alias"]) +@pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"]) +def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path, kind, operation): + """Keep submodule operations compatible with existing symlinks in Git metadata. + + Cover linked metadata directories, gitfiles, configs, and internal aliases. + Update, move, and rename must retain a usable checkout; forced removal must + still remove it. + """ + sm = movable_submodule + sm.rename("nested/module") + root = Path(sm.repo.working_tree_dir) + modules = Path(sm.repo.git_dir) / "modules" + paths = { + "modules": modules, + "intermediate": modules / "nested", + "leaf": modules / "nested/module", + "gitfile": root / "module/.git", + "config": modules / "nested/module/config", + "alias": modules / "alias", + } + link = paths[kind] + target = tmp_path / "outside" + if kind == "alias": + target = modules / "nested" + (root / "module/.git").write_text("gitdir: ../.git/modules/alias/module") + else: + link.rename(target) + link.symlink_to(target, target_is_directory=target.is_dir()) + # Relocating metadata changes the base of a relative core.worktree setting. + sm.repo.git.config("--file", str(modules / "nested/module/config"), "core.worktree", str(root / "module")) + assert sm.module_exists() + if operation == "remove": + sm.remove(force=True) + assert not (root / "module").exists() + return + if operation == "update": + sm.update() + elif operation == "move": + sm.move("moved") + else: + sm.rename("renamed") + with sm.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve() + assert Path(sm.abspath, "file").read_text() == "content" + + class TestRootProgress(RootUpdateProgress): """Just prints messages, for now without checking the correctness of the states""" From 2bfd829f37f4bb810e3c4b409baeeb2be34544b8 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 08:46:28 +0200 Subject: [PATCH 06/19] fix: Preserve submodule paths and release checkout handles on Windows A quick rubber-stamp, admittedly. V4 will probably review all tests and make it more proper, if there can be such a thing in python anyway. Both Windows failures came from os.renames() pruning a directory symlink above the source after the rename succeeded. Unlike POSIX, Windows rmdir() can remove a directory symlink even when its target is nonempty. Moving a checkout through a worktree alias therefore deleted the alias and broke configuration updates and rollback. Renaming metadata through a linked .git/modules directory deleted that link and broke config.lock creation. Route checkout moves, rollback, and metadata renames through one helper. It creates destination parents and renames the source, then prunes empty source parents only until it reaches a symlink or a directory it cannot remove. This keeps ordinary empty-directory cleanup while preserving parent links and their targets, including targets that become empty. Leaf symlinks continue to move as links rather than moving their targets. Exercise Windows directory-symlink removal semantics on POSIX in the existing compatibility tests, and use native behavior on Windows. Both reported failures reproduced locally before the fix. Strengthen assertions that worktree and metadata parent aliases survive, their targets remain directories, and leaf metadata symlinks move without moving their targets. A further Windows run exposed a separate sharing violation during the checkout move. Submodule.add() read HEAD through its temporary Repo but left that Repo's persistent cat-file processes open. Those processes can hold the checkout as their current directory and prevent its rename. Close the owned Repo with a context manager when reading HEAD, including on read failure, instead of waiting for garbage collection. Add a regression that observes the real cat-file processes started for the new checkout and requires them to have exited before add() returns. It failed before the fix and now passes, along with the immediate move. The remaining metadata failures also reproduce with Python 3.7's Windows path semantics: ntpath.realpath is an alias of abspath and does not resolve symlinks. Relative core.worktree values were calculated from the metadata alias instead of the repository directory Git actually opens. This broke add/reconnect HEAD reads and made moves and renames point at nonexistent worktrees. The SHA/dubious-ownership message was a secondary read failure. Use pathlib.Path.resolve(), which resolves Windows symlinks on Python 3.7, for both endpoints of gitfile/config rewrites and for metadata removal. Run metadata and worktree-alias tests with native and simulated Windows 3.7 realpath behavior. The simulation reproduced all eight reported failures plus a leaf-symlink removal failure before this change. Reference: https://github.com/python/cpython/blob/3.7/Lib/ntpath.py and https://github.com/python/cpython/blob/3.7/Lib/pathlib.py. Validation: 183 passed, 3 skipped, and 1 expected failure across the submodule and diff suites plus the commit-message hook success test on macOS. Ruff lint and formatting, mypy for the changed module, and git diff --check passed. Native Windows validation remains for CI. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/objects/submodule/base.py | 34 +++++++++++++---- test/test_submodule.py | 72 +++++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 461b3068d..2b21fc145 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -9,6 +9,7 @@ import ntpath import os import os.path as osp +from pathlib import Path import shlex import stat import sys @@ -444,6 +445,20 @@ def _checked_abspath( raise ValueError("Submodule path %r contains a symbolic link" % relative_path) return path + @staticmethod + def _renames(source: PathLike, destination: PathLike) -> None: + os.makedirs(osp.dirname(destination), exist_ok=True) + os.rename(source, destination) + # Match renames() cleanup, but stop before directory symlinks: Windows + # rmdir() removes the link even when its target is nonempty. + parent = osp.dirname(source) + while parent and not osp.islink(parent): + try: + os.rmdir(parent) + except OSError: + break + parent = osp.dirname(parent) + @classmethod def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_abspath: PathLike) -> None: """Write a ``.git`` file containing a (preferably) relative path to the actual @@ -468,8 +483,9 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab Absolute path to the bare repository. """ # Git resolves metadata symlinks before interpreting core.worktree. - module_abspath = osp.realpath(module_abspath) - working_tree_dir = osp.realpath(working_tree_dir) + # Path.resolve() also handles Windows symlinks on Python 3.7. + module_abspath = str(Path(module_abspath).resolve()) + working_tree_dir = str(Path(working_tree_dir).resolve()) git_file = osp.join(working_tree_dir, ".git") module_config = osp.join(module_abspath, "config") rela_path = osp.relpath(module_abspath, start=working_tree_dir) @@ -685,7 +701,9 @@ def add( # We deliberately assume that our head matches our index! if mrepo: - sm.binsha = mrepo.head.commit.binsha + # Release cat-file processes before callers move the checkout on Windows. + with mrepo: + sm.binsha = mrepo.head.commit.binsha index.add([sm], write=True) return sm @@ -1120,7 +1138,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = # Move the module into place if possible. renamed_module = False if module and osp.exists(cur_path): - os.renames(cur_path, module_checkout_abspath) + self._renames(cur_path, module_checkout_abspath) renamed_module = True if osp.isfile(osp.join(module_checkout_abspath, ".git")): @@ -1151,7 +1169,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = # END handle configuration flag except Exception: if renamed_module: - os.renames(module_checkout_abspath, cur_path) + self._renames(module_checkout_abspath, cur_path) # END undo module renaming raise # END handle undo rename @@ -1239,7 +1257,7 @@ def remove( ################################ if module and self.module_exists(): mod = self.module() - git_dir = osp.realpath(mod.git_dir) + git_dir = str(Path(mod.git_dir).resolve()) if force: # Take the fast lane and just delete everything in our module path. # TODO: If we run into permission problems, we have a highly @@ -1504,10 +1522,10 @@ def rename(self, new_name: str) -> "Submodule": # Let's be sure the submodule name is not so obviously tied to a directory. if str(destination_module_abspath).startswith(str(mod.git_dir)): tmp_dir = self._module_abspath(self.repo, self.path, str(uuid.uuid4())) - os.renames(source_dir, tmp_dir) + self._renames(source_dir, tmp_dir) source_dir = tmp_dir # END handle self-containment - os.renames(source_dir, destination_module_abspath) + self._renames(source_dir, destination_module_abspath) if mod.working_tree_dir: self._write_git_file_and_module_config(mod.working_tree_dir, destination_module_abspath) # END move separate git repository diff --git a/test/test_submodule.py b/test/test_submodule.py index 1fcf3ae23..e64416847 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -252,7 +252,53 @@ def test_submodule_rejects_checkout_and_gitmodules_symlinks(movable_submodule, t assert (root / "moved").is_dir() -def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path): +def test_add_closes_checkout_processes(movable_submodule, monkeypatch): + """Adding a submodule must not leave a child process holding its checkout open.""" + sm = movable_submodule + checkout = Path(sm.repo.working_tree_dir, "new") + execute = Git.execute + processes = [] + + def capture_process(self, command, *args, **kwargs): + result = execute(self, command, *args, **kwargs) + if ( + kwargs.get("as_process") + and "cat-file" in command + and Path(self.working_dir).resolve() == checkout.resolve() + ): + processes.append(result.proc) + return result + + monkeypatch.setattr(Git, "execute", capture_process) + try: + added = Submodule.add(sm.repo, "new", "new", sm.url) + assert processes, "The HEAD read must exercise a persistent cat-file process" + assert all(process.poll() is not None for process in processes) + added.move("moved") + finally: + for process in processes: + if process.poll() is None: + process.terminate() + process.wait() + + +@pytest.fixture +def windows_directory_symlink_removal(monkeypatch): + """Exercise Windows rmdir semantics on POSIX, where rmdir rejects symlinks.""" + if sys.platform != "win32": + original_rmdir = os.rmdir + + def rmdir(path, *args, **kwargs): + if osp.islink(path): + return os.unlink(path, *args, **kwargs) + return original_rmdir(path, *args, **kwargs) + + monkeypatch.setattr(os, "rmdir", rmdir) + + +def test_submodule_allows_symlink_above_worktree( + movable_submodule, tmp_path, windows_directory_symlink_removal, metadata_realpath +): """Allow adding and moving submodules when the parent is opened through a symlink.""" sm = movable_submodule alias = tmp_path / "alias" @@ -260,13 +306,25 @@ def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path): with git.Repo(alias) as parent: added = Submodule.add(parent, "new", "new", sm.url) added.move("moved") + assert alias.is_symlink() with added.module() as module: assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(added.abspath).resolve() assert Path(added.abspath, "file").read_text() == "content" +@pytest.fixture(params=[False, True], ids=["native-realpath", "windows37-realpath"]) +def metadata_realpath(request): + """Model Python 3.7 on Windows without altering pathlib's own resolver.""" + if request.param: + with mock.patch("git.objects.submodule.base.osp", wraps=osp) as paths: + paths.realpath.side_effect = osp.abspath + yield + else: + yield + + @pytest.mark.parametrize("operation", ["add", "reconnect", "rename"]) -def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation): +def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation, metadata_realpath): """Allow linked metadata destinations while keeping the checkout correctly connected. Adding, reconnecting after deinit, and renaming may store metadata outside the @@ -296,7 +354,9 @@ def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_p @pytest.mark.parametrize("kind", ["modules", "intermediate", "leaf", "gitfile", "config", "alias"]) @pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"]) -def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path, kind, operation): +def test_submodule_allows_existing_metadata_symlinks( + movable_submodule, tmp_path, kind, operation, windows_directory_symlink_removal, metadata_realpath +): """Keep submodule operations compatible with existing symlinks in Git metadata. Cover linked metadata directories, gitfiles, configs, and internal aliases. @@ -336,6 +396,12 @@ def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path sm.move("moved") else: sm.rename("renamed") + if kind == "modules" or (operation == "rename" and kind in ("intermediate", "alias")): + assert link.is_symlink() + assert link.is_dir() + if operation == "rename" and kind == "leaf": + assert (modules / "renamed").is_symlink() + assert target.is_dir() with sm.module() as module: assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve() assert Path(sm.abspath, "file").read_text() == "content" From 415c211705882fadae6be0a299d86e1e4a3c5b03 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 08:19:17 +0000 Subject: [PATCH 07/19] test: Simplify submodule path simulation and verify metadata removal rubber stamp The Python 3.7 Windows path simulation wrapped the entire os.path module in Mock. On Windows, all six simulated removal cases hit sharing violations that the default error handling converted into skipped tests. Use a SimpleNamespace copy of the path module and replace only realpath with abspath. Patch only the submodule module's osp binding so pathlib keeps its own resolver and other path operations remain ordinary calls. This retains Python 3.7 compatibility and allows the removal cases to run successfully without Windows permission-error suppression. Before removing a submodule, resolve and verify its metadata directory, then assert that removal deletes it as well as the checkout. Checking only the checkout could miss metadata left behind through a directory symlink. Validation: all 57 focused submodule compatibility and process-cleanup tests passed on Windows with Python 3.10 and HIDE_WINDOWS_KNOWN_ERRORS=0. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 was unavailable; its realpath behavior is simulated. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/objects/submodule/base.py | 6 +++++ test/test_submodule.py | 50 ++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 2b21fc145..57dbf81f3 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -1226,6 +1226,12 @@ def remove( Doesn't work atomically, as failure to remove any part of the submodule will leave an inconsistent state. + :note: + Metadata-directory aliases under ``.git/modules`` are retained. A link + directly to the deleted repository becomes dangling; adding or initializing + the submodule again recreates its target. Linked parent directories remain + available to sibling submodules. + :raise git.exc.InvalidGitRepositoryError: Thrown if the repository cannot be deleted. diff --git a/test/test_submodule.py b/test/test_submodule.py index e64416847..5ca855bd4 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -9,6 +9,7 @@ import shutil import sys import tempfile +from types import SimpleNamespace from unittest import mock, skipUnless import pytest @@ -316,8 +317,9 @@ def test_submodule_allows_symlink_above_worktree( def metadata_realpath(request): """Model Python 3.7 on Windows without altering pathlib's own resolver.""" if request.param: - with mock.patch("git.objects.submodule.base.osp", wraps=osp) as paths: - paths.realpath.side_effect = osp.abspath + paths = SimpleNamespace(**vars(osp)) + paths.realpath = osp.abspath + with mock.patch("git.objects.submodule.base.osp", paths): yield else: yield @@ -361,7 +363,7 @@ def test_submodule_allows_existing_metadata_symlinks( Cover linked metadata directories, gitfiles, configs, and internal aliases. Update, move, and rename must retain a usable checkout; forced removal must - still remove it. + still remove both the checkout and the resolved metadata directory. """ sm = movable_submodule sm.rename("nested/module") @@ -387,8 +389,20 @@ def test_submodule_allows_existing_metadata_symlinks( sm.repo.git.config("--file", str(modules / "nested/module/config"), "core.worktree", str(root / "module")) assert sm.module_exists() if operation == "remove": + metadata_dir = (modules / "nested/module").resolve() + assert metadata_dir.is_dir() + url = sm.url sm.remove(force=True) assert not (root / "module").exists() + assert not metadata_dir.exists() + if kind in ("modules", "intermediate", "alias"): + assert link.is_symlink() and link.is_dir() + replacement = Submodule.add(sm.repo, "nested/module", "module", url) + if kind == "leaf": + assert link.is_symlink() and link.is_dir() + assert target.is_dir() + with replacement.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == (root / "module").resolve() return if operation == "update": sm.update() @@ -407,6 +421,36 @@ def test_submodule_allows_existing_metadata_symlinks( assert Path(sm.abspath, "file").read_text() == "content" +@pytest.mark.parametrize("kind", ["modules", "intermediate", "leaf"]) +def test_remove_linked_metadata_keeps_siblings_and_can_reinitialize( + movable_submodule, tmp_path, kind, metadata_realpath +): + sm = movable_submodule + sm.rename("nested/module") + sibling = Submodule.add(sm.repo, "nested/sibling", "sibling", sm.url) + sm.repo.index.commit("Add sibling") + modules = Path(sm.repo.git_dir) / "modules" + link = {"modules": modules, "intermediate": modules / "nested", "leaf": modules / "nested/module"}[kind] + target = tmp_path / "outside" + link.rename(target) + link.symlink_to(target, target_is_directory=True) + for child in (sm, sibling): + sm.repo.git.config("--file", str(modules / child.name / "config"), "core.worktree", str(child.abspath)) + + sm.remove(force=True, configuration=False) + + assert link.is_symlink() + assert link.exists() == (kind != "leaf") + with sibling.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sibling.abspath).resolve() + assert Path(sibling.abspath, "file").read_text() == "content" + sm.update(init=True) + assert link.is_symlink() and link.is_dir() + assert Path(sm.abspath, "file").read_text() == "content" + with sm.module() as module: + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve() + + class TestRootProgress(RootUpdateProgress): """Just prints messages, for now without checking the correctness of the states""" From 958002b4cc06647407d39eb78c65d9dc68d1bee9 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 11:28:46 +0200 Subject: [PATCH 08/19] test: Disable automatic maintenance in remote timeout tests Ubuntu CI reported FileNotFoundError for maintenance.lock while running test_timeout_funcs. The test performs normal pull/fetch calls before its forced timeouts, and Git can launch detached automatic maintenance from those operations. Maintenance can then race with the fixture's recursive removal of the temporary repository, removing a lock file after cleanup has enumerated it. Disable maintenance.auto in this test's temporary repository and set gc.auto to zero for older Git versions that use automatic garbage collection. This removes background housekeeping unrelated to the timeout assertions without weakening repository cleanup or changing library behavior. Use mock.patch.object for the global forced termination status so an assertion failure cannot leak the override into subsequent tests. Validation: Git Trace2 recorded three detached maintenance launches in the original test and none with the fix. The timeout test then passed 20 consecutive runs locally on macOS. Ruff lint and formatting checks and git diff --check passed. The original Ubuntu cleanup exception was not reproduced locally; tracing verified removal of the suspected race source. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- test/test_remote.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index e1793214c..0212ffab8 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -1090,14 +1090,16 @@ def test_fetch_unsafe_branch_name(self, rw_repo, remote_repo): class TestTimeouts(TestBase): @with_rw_repo("HEAD", bare=False) def test_timeout_funcs(self, repo): + # Maintenance may outlive a timed-out fetch and race with fixture cleanup. + with repo.config_writer() as config: + config.set_value("maintenance", "auto", False) + config.set_value("gc", "auto", 0) # Older Git versions use auto-gc. + # Force error code to prevent a race condition if the python thread is slow. - default = Git.AutoInterrupt._status_code_if_terminate - Git.AutoInterrupt._status_code_if_terminate = -15 - for function in ["pull", "fetch"]: # Can't get push to time out. - f = getattr(repo.remotes.origin, function) - assert f is not None # Make sure these functions exist. - _ = f() # Make sure the function runs. - with pytest.raises(GitCommandError, match="kill_after_timeout=0 s"): - f(kill_after_timeout=0) - - Git.AutoInterrupt._status_code_if_terminate = default + with mock.patch.object(Git.AutoInterrupt, "_status_code_if_terminate", -15): + for function in ["pull", "fetch"]: # Can't get push to time out. + f = getattr(repo.remotes.origin, function) + assert f is not None # Make sure these functions exist. + _ = f() # Make sure the function runs. + with pytest.raises(GitCommandError, match="kill_after_timeout=0 s"): + f(kill_after_timeout=0) From e9e271bdc0982c262cd47ca4858533eb8c60e8bd Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 09:56:03 +0000 Subject: [PATCH 09/19] fix: Clone into dangling submodule metadata symlink targets on Windows rubber stamp Removing a submodule retains its metadata alias but deletes the target. Adding the same submodule again then passes a dangling directory symlink to git clone --separate-git-dir. Git for Windows fails while copying its template files through that alias. Both native-realpath and simulated Windows 3.7 remove-leaf cases reproduced this failure locally. When the metadata destination is a leaf symlink, pass its target to Git and leave the alias intact. Resolve relative targets against the link's parent, create missing target parents through the existing clone setup, and let Git create the repository directory itself. Precreating that directory is insufficient because Git rejects an existing separate git repository destination. Read the link explicitly because Python 3.7 on Windows cannot resolve a dangling link with Path.resolve(). Normalize the Windows namespace prefix returned by newer os.readlink implementations, including UNC targets, and use forward slashes before passing the path through Git's URL logic. The shared clone helper covers add() and initialization through update(). Add regression coverage for direct cloning through absolute and relative dangling metadata links with missing target parents. Verify that the link and its stored target are retained, metadata is created at the target, and the resulting checkout works under both realpath modes. Validation: 61 focused tests passed on Windows/Python 3.10 with HIDE_WINDOWS_KNOWN_ERRORS=0, including both reported failures. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 and UNC network shares were not available for execution. A broader run also exposed sharing violations in sibling-reinitialization tests before cloning; a representative case also failed with the unchanged HEAD clone helper loaded in memory. That separate removal issue is not changed here. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/objects/submodule/base.py | 12 ++++++++++++ test/test_submodule.py | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 57dbf81f3..8308e4459 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -377,6 +377,18 @@ def _clone_repo( repo.unsafe_git_clone_options, ) allow_unsafe_options = True + if osp.islink(module_abspath): + # Clone into the target while retaining the metadata alias. Git for + # Windows cannot initialize through a dangling directory symlink. + # Read the link explicitly for Python 3.7, and remove the Windows + # namespace prefix returned by newer Python versions for Git. + target = os.readlink(module_abspath) + if sys.platform == "win32": + if target.startswith("\\\\?\\UNC\\"): + target = "\\\\" + target[8:] + elif target.startswith("\\\\?\\"): + target = target[4:] + module_abspath = to_native_path_linux(osp.join(osp.dirname(module_abspath), target)) kwargs["separate_git_dir"] = module_abspath module_abspath_dir = osp.dirname(module_abspath) if not osp.isdir(module_abspath_dir): diff --git a/test/test_submodule.py b/test/test_submodule.py index 5ca855bd4..da391d5cb 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -451,6 +451,25 @@ def test_remove_linked_metadata_keeps_siblings_and_can_reinitialize( assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve() +@pytest.mark.parametrize("relative_target", [False, True], ids=["absolute-target", "relative-target"]) +def test_add_to_dangling_metadata_symlink(movable_submodule, tmp_path, metadata_realpath, relative_target): + sm = movable_submodule + link = Path(sm.repo.git_dir) / "modules/new" + target = tmp_path / "missing" / "metadata" + link.symlink_to(osp.relpath(target, link.parent) if relative_target else target, target_is_directory=True) + link_target = os.readlink(link) + + added = Submodule.add(sm.repo, "new", "new", sm.url) + + assert link.is_symlink() and link.is_dir() + assert os.readlink(link) == link_target + assert (target / "HEAD").is_file() + with added.module() as module: + assert Path(module.git_dir).resolve() == target.resolve() + assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(added.abspath).resolve() + assert Path(added.abspath, "file").read_text() == "content" + + class TestRootProgress(RootUpdateProgress): """Just prints messages, for now without checking the correctness of the states""" From 2aca1778cd8b884b8dd39c638e366072e99425ee Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 10:24:34 +0000 Subject: [PATCH 10/19] test: Pass strings to readlink for Windows Python 3.7 rubber stamp, just get this through CI OMG The dangling-metadata regression passed pathlib.Path objects to os.readlink() when capturing and checking the symlink target. Windows Python 3.7 requires a string argument, so all four parameter combinations failed with TypeError before exercising the clone fix. Convert the path to str at both calls. The production clone helper already passes a string and needs no change. Keep the target-preservation assertions and the absolute/relative and realpath-mode coverage intact. Validation: reproduced all four TypeErrors on Windows/Python 3.10 with an in-memory readlink wrapper enforcing the Python 3.7 string requirement. After the conversions, all four cases passed with the same wrapper and Windows permission-error suppression disabled. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 was not available locally. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- test/test_submodule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index da391d5cb..f5070c665 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -457,12 +457,12 @@ def test_add_to_dangling_metadata_symlink(movable_submodule, tmp_path, metadata_ link = Path(sm.repo.git_dir) / "modules/new" target = tmp_path / "missing" / "metadata" link.symlink_to(osp.relpath(target, link.parent) if relative_target else target, target_is_directory=True) - link_target = os.readlink(link) + link_target = os.readlink(str(link)) added = Submodule.add(sm.repo, "new", "new", sm.url) assert link.is_symlink() and link.is_dir() - assert os.readlink(link) == link_target + assert os.readlink(str(link)) == link_target assert (target / "HEAD").is_file() with added.module() as module: assert Path(module.git_dir).resolve() == target.resolve() From 8da65080849a56a2fc9f3e7817bbf0524e8397d5 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 08:07:55 +0200 Subject: [PATCH 11/19] fix: validate checkout positional reference options Head.checkout checked keyword options but omitted the serialized reference from its existing unsafe-option validation (GHSA-23mf-xhv8-69c2). Reference names can originate in a cloned repository, so callers could reach behavior that normally requires explicit opt-in without supplying any checkout options themselves. Pass self through the shared option-candidate helper, matching the argument actually sent to Git. This applies the existing policy to direct and cloned references, including abbreviated option spellings, while retaining allow_unsafe_options=True and ordinary checkout semantics. A leading -- separator would instead make the reference a pathspec and break branch switching. Add direct-reference regression coverage and a local clone regression using synthetic file content; the latter also verifies explicit opt-in. Both regression tests failed before the guard change. All 30 tests in test/test_refs.py pass with Python 3.12.14 and Apple Git 2.50.1; git diff --check passes. Git behavior reference: local git/git checkout at 1630431f326e15fcde608827b5ff38422528eb59, builtin/checkout.c checkout_main pathspec_from_file handling, which parses file contents as pathspecs. No Git source was copied. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- doc/source/changes.rst | 1 + git/refs/head.py | 2 +- test/test_refs.py | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 4803790b9..df003c7d3 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -8,6 +8,7 @@ Changelog Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58 +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-23mf-xhv8-69c2 If you can, also try and provide feedback on the upcoming v4 branch https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. diff --git a/git/refs/head.py b/git/refs/head.py index 7f563374e..5dd4a5a0a 100644 --- a/git/refs/head.py +++ b/git/refs/head.py @@ -283,7 +283,7 @@ def checkout( """ if not allow_unsafe_options: Git.check_unsafe_options( - options=Git._option_candidates([], kwargs), + options=Git._option_candidates([self], kwargs), unsafe_options=Git.unsafe_git_pathspec_from_file_options, ) kwargs["f"] = force diff --git a/test/test_refs.py b/test/test_refs.py index 32c8fbe34..ae3687c30 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -289,6 +289,25 @@ def test_head_checkout_rejects_pathspec_from_file(self, rw_repo): pathspec_file_nul=True, **{option_name: str(pathspecs)}, ) + for option_name in ("--pathspec-from-file", "--pathspec-from"): + branch = Head(rw_repo, f"refs/heads/{option_name}={pathspecs}") + with self.assertRaises(UnsafeOptionError): + branch.checkout() + + def test_cloned_head_checkout_rejects_pathspec_from_file(self): + with tempfile.TemporaryDirectory() as tdir: + base_dir = Path(tdir) + with self._repo_with_initial_commit(base_dir) as source: + branch = source.create_head("--pathspec-from-file=pathspecs") + source.head.reference = branch + with Repo.clone_from(source.working_tree_dir, base_dir / "clone") as cloned: + (base_dir / "clone" / "pathspecs").write_text("unmatched-private-content\n", encoding="utf-8") + assert cloned.active_branch.name == branch.name + with self.assertRaises(UnsafeOptionError): + cloned.active_branch.checkout() + with self.assertRaises(GitCommandError) as error: + cloned.active_branch.checkout(allow_unsafe_options=True) + assert "unmatched-private-content" in str(error.exception) @with_rw_repo("HEAD") def test_head_reset_rejects_pathspec_from_file(self, rw_repo): From fa931374b841fd01f11114b42ca0fdb4d87633dc Mon Sep 17 00:00:00 2001 From: Mingyang Wu <129849514+aprylewu@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:02:32 +0800 Subject: [PATCH 12/19] Fix timeout child-process lookup on non-GNU systems Git.execute used ps --ppid to find direct children before enforcing kill_after_timeout. On macOS this option is rejected: the parent is killed, but a child can continue running and hold captured output pipes open. Use pgrep -P for the child lookup, with POSIX ps PID/PPID output as a fallback when pgrep is absent. Filter the fallback by the original parent PID and reap the lookup subprocess in both paths. Keep the existing parent-first SIGKILL order, direct-child scope, and Windows guard, and update the documented command requirements. Systems without either lookup facility and the existing PID-reuse race remain limitations. Add real-process regressions for native pgrep and the ps fallback, plus a test that excludes unrelated processes and grandchildren from the fallback. Both real-process cases failed on the original code on macOS. The command module now passes 105 tests with 1 skip on macOS 27.0 / Python 3.13.5. Ruff check and format, codespell, mypy (45 files), basedpyright, and diff whitespace checks pass. Linux and Cygwin were not run locally; Cygwin's default ps lacks the required options, so the real-process cases skip it. Fixes #1756 Signed-off-by: Mingyang Wu <129849514+aprylewu@users.noreply.github.com> --- git/cmd.py | 30 ++++++++++++++++--------- test/test_git.py | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 193dfd4f6..9de2f6e8a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1303,9 +1303,9 @@ def execute( carefully considered, due to the following limitations: 1. This feature is not supported at all on Windows. - 2. Effectiveness may vary by operating system. ``ps --ppid`` is used to - enumerate child processes, which is available on most GNU/Linux systems - but not most others. + 2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command + supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not + installed. Effectiveness may vary on systems without these commands. 3. Deeper descendants do not receive signals, though they may sometimes terminate as a consequence of their parent processes being killed. 4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side @@ -1465,14 +1465,24 @@ def kill_process(pid: int) -> None: This callback implementation would be ineffective and unsafe on Windows. """ - p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE) child_pids = [] - if p.stdout is not None: - for line in p.stdout: - if len(line.split()) > 0: - local_pid = (line.split())[0] - if local_pid.isdigit(): - child_pids.append(int(local_pid)) + try: + p = Popen(["pgrep", "-P", str(pid)], stdout=PIPE) + except FileNotFoundError: + # POSIX ps does not support selecting by parent PID. + with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p: + if p.stdout is not None: + for line in p.stdout: + fields = line.split() + if len(fields) == 2 and all(field.isdigit() for field in fields): + if int(fields[1]) == pid: + child_pids.append(int(fields[0])) + else: + with p: + if p.stdout is not None: + for line in p.stdout: + if line.strip().isdigit(): + child_pids.append(int(line)) try: os.kill(pid, signal.SIGKILL) for child_pid in child_pids: diff --git a/test/test_git.py b/test/test_git.py index a88d980fb..b19652363 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -14,6 +14,7 @@ import pickle import re import shutil +import signal import subprocess import sys import tempfile @@ -332,6 +333,63 @@ def test_it_honors_kill_after_timeout_with_output_stream(self): self.assertEqual(output_stream.getvalue(), b"started\n") self.assertIn("Timeout: the command", stderr) + @skipUnless( + sys.platform not in ("win32", "cygwin"), + "child process lookup requires pgrep or POSIX ps", + ) + @ddt.data(False, True) + def test_timeout_kills_direct_child(self, without_pgrep): + with tempfile.TemporaryDirectory() as directory: + marker = Path(directory, "child-survived") + child_code = ( + "import pathlib, sys, time; time.sleep(2); " + "pathlib.Path(sys.argv[1]).write_text('survived', encoding='utf-8')" + ) + parent_code = ( + "import subprocess, sys, time; " + "subprocess.Popen([sys.executable, '-c', sys.argv[1], sys.argv[2]]); " + "time.sleep(30)" + ) + popen = cmd.Popen + + def portable_popen(args, **kwargs): + if without_pgrep and args[0] == "pgrep": + raise FileNotFoundError("pgrep is not installed") + return popen(args, **kwargs) + + with mock.patch.object(cmd, "Popen", side_effect=portable_popen): + status, _, stderr = self.git.execute( + [sys.executable, "-c", parent_code, child_code, str(marker)], + kill_after_timeout=1, + with_exceptions=False, + with_extended_output=True, + ) + + self.assertNotEqual(status, 0) + self.assertIn("Timeout: the command", stderr) + self.assertFalse(marker.exists(), "the direct child survived the timeout") + + @skipUnless(sys.platform != "win32", "kill_after_timeout is not supported on Windows") + def test_timeout_ps_fallback_selects_only_direct_children(self): + process = mock.MagicMock() + process.pid = 1234 + process.communicate.return_value = (b"", b"") + process.returncode = -signal.SIGKILL + ps = mock.MagicMock() + ps.__enter__.return_value = ps + ps.stdout = io.BytesIO(b"PID PPID\n 321 1\n 5678 1234\n 9012 5678\n\n") + + with contextlib.ExitStack() as stack: + stack.enter_context(mock.patch.object(cmd, "safer_popen", return_value=process)) + stack.enter_context(mock.patch.object(cmd, "Popen", side_effect=[FileNotFoundError, ps])) + kill = stack.enter_context(mock.patch.object(cmd.os, "kill")) + timer = stack.enter_context(mock.patch.object(cmd.threading, "Timer")) + # Run the timeout callback synchronously, with no real processes or signals. + timer.return_value.start.side_effect = lambda: timer.call_args.args[1](1234) + self.git.execute(["git", "version"], kill_after_timeout=1, with_exceptions=False) + + self.assertEqual(kill.call_args_list, [mock.call(1234, signal.SIGKILL), mock.call(5678, signal.SIGKILL)]) + def test_it_executes_git_without_stdout_redirect(self): returncode, stdout, stderr = self.git.execute( ["git", "version"], From d66edad65ef7c6a42a3040c4c5287e53a16b6b68 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 13 Sep 2026 04:44:16 +0200 Subject: [PATCH 13/19] Correct public API typing and add portable runtime checks Several public annotations rejected supported inputs or lost the relationship between input options and return types. Describe Git.execute process, text, bytes, and extended-output results with overloads, accept stdin file descriptors, and account for absent stdout. Correct remote-removal, object, database, blame, and index-entry types, preserve entry subclasses and the supported tuple shapes, and accept streams with only the required read or write methods. Normalize absent previous stderr before appending process errors in AutoInterrupt.wait. Add runtime and static regressions for these interfaces, include them in mypy and basedpyright, make required imports explicit, and reduce the basedpyright baseline to the remaining diagnostics. Keep mock available to typecheck the Python 3.7 import branches. Make the output-type regression emit its payload without a newline. Its original print call produced CRLF on Windows; Git.execute strips the final LF as documented, leaving a carriage return that failed the test in all 11 Windows jobs. Omitting the newline keeps the same text, bytes, and tuple assertions independent of platform newline translation. Validation on macOS with Python 3.12.14: all four test/test_typing.py tests pass. Forcing the child stdout to translate newlines to CRLF reproduces the old failure and passes with the corrected command. Mypy passes for 46 source files; basedpyright --warnings reports no errors or warnings; Ruff lint and format checks for test/test_typing.py and git diff --check pass. Native Windows validation is delegated to the PR CI matrix. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- .basedpyright/baseline.json | 1464 +++++++++++---------------------- git/cmd.py | 112 ++- git/index/base.py | 2 +- git/index/fun.py | 6 +- git/index/typ.py | 23 +- git/objects/base.py | 4 +- git/objects/commit.py | 2 +- git/objects/fun.py | 6 +- git/objects/submodule/base.py | 4 +- git/objects/tag.py | 4 +- git/refs/reference.py | 2 +- git/remote.py | 16 +- git/repo/base.py | 13 +- git/repo/fun.py | 19 +- git/types.py | 16 + git/util.py | 5 +- pyproject.toml | 4 +- test-requirements.txt | 2 +- test/test_git.py | 26 +- test/test_typing.py | 91 ++ 20 files changed, 772 insertions(+), 1049 deletions(-) create mode 100644 test/test_typing.py diff --git a/.basedpyright/baseline.json b/.basedpyright/baseline.json index 14bf1024e..79d35235e 100644 --- a/.basedpyright/baseline.json +++ b/.basedpyright/baseline.json @@ -1,966 +1,502 @@ { - "files": { - "./git/cmd.py": [ - { - "code": "reportGeneralTypeIssues", - "range": { - "startColumn": 13, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportOptionalOperand", - "range": { - "startColumn": 27, - "endColumn": 35, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 28, - "endColumn": 31, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 28, - "endColumn": 40, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 19, - "endColumn": 68, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 19, - "endColumn": 31, - "lineCount": 1 - } - } - ], - "./git/config.py": [ - { - "code": "reportGeneralTypeIssues", - "range": { - "startColumn": 11, - "endColumn": 26, - "lineCount": 1 - } - }, - { - "code": "reportInvalidTypeVarUse", - "range": { - "startColumn": 43, - "endColumn": 45, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 41, - "endColumn": 48, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 32, - "endColumn": 39, - "lineCount": 1 - } - }, - { - "code": "reportCallIssue", - "range": { - "startColumn": 25, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 30, - "endColumn": 39, - "lineCount": 1 - } - } - ], - "./git/db.py": [ - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 12, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 61, - "endColumn": 79, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 14, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 70, - "endColumn": 88, - "lineCount": 1 - } - } - ], - "./git/index/base.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 30, - "endColumn": 36, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 28, - "endColumn": 34, - "lineCount": 1 - } - }, - { - "code": "reportAssignmentType", - "range": { - "startColumn": 38, - "endColumn": 76, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 45, - "endColumn": 53, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 60, - "endColumn": 63, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 56, - "endColumn": 60, - "lineCount": 1 - } - }, - { - "code": "reportSelfClsParameterName", - "range": { - "startColumn": 30, - "endColumn": 33, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 51, - "endColumn": 57, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 55, - "endColumn": 61, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 12, - "lineCount": 1 - } - }, - { - "code": "reportAssignmentType", - "range": { - "startColumn": 20, - "endColumn": 46, - "lineCount": 1 - } - } - ], - "./git/index/fun.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 8, - "endColumn": 36, - "lineCount": 1 - } - } - ], - "./git/index/typ.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 37, - "endColumn": 46, - "lineCount": 1 - } - } - ], - "./git/objects/base.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 20, - "endColumn": 27, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 29, - "endColumn": 36, - "lineCount": 1 - } - } - ], - "./git/objects/blob.py": [ - { - "code": "reportIncompatibleVariableOverride", - "range": { - "startColumn": 4, - "endColumn": 8, - "lineCount": 1 - } - } - ], - "./git/objects/commit.py": [ - { - "code": "reportIncompatibleVariableOverride", - "range": { - "startColumn": 4, - "endColumn": 8, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 31, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 20, - "endColumn": 33, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 20, - "endColumn": 30, - "lineCount": 1 - } - } - ], - "./git/objects/submodule/base.py": [ - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 19, - "endColumn": 24, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 19, - "endColumn": 24, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 23, - "endColumn": 25, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 23, - "endColumn": 25, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 41, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 52, - "endColumn": 84, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 20, - "endColumn": 40, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 35, - "endColumn": 42, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 20, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 20, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 19, - "endColumn": 33, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 19, - "endColumn": 33, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 27, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 27, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 19, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 19, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 18, - "endColumn": 20, - "lineCount": 1 - } - } - ], - "./git/objects/tag.py": [ - { - "code": "reportIncompatibleVariableOverride", - "range": { - "startColumn": 4, - "endColumn": 8, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 72, - "endColumn": 78, - "lineCount": 1 - } - } - ], - "./git/objects/tree.py": [ - { - "code": "reportIncompatibleVariableOverride", - "range": { - "startColumn": 4, - "endColumn": 8, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 31, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 19, - "endColumn": 83, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 54, - "lineCount": 1 - } - } - ], - "./git/objects/util.py": [ - { - "code": "reportAssignmentType", - "range": { - "startColumn": 23, - "endColumn": 34, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 22, - "endColumn": 26, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 30, - "endColumn": 34, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 54, - "lineCount": 1 - } - } - ], - "./git/refs/log.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 30, - "endColumn": 34, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 17, - "endColumn": 22, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 28, - "endColumn": 30, - "lineCount": 1 - } - } - ], - "./git/refs/reference.py": [ - { - "code": "reportInvalidTypeVarUse", - "range": { - "startColumn": 22, - "endColumn": 34, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleVariableOverride", - "range": { - "startColumn": 13, - "endColumn": 17, - "lineCount": 1 - } - } - ], - "./git/refs/symbolic.py": [ - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 15, - "endColumn": 20, - "lineCount": 1 - } - } - ], - "./git/refs/tag.py": [ - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 14, - "lineCount": 1 - } - }, - { - "code": "reportIncompatibleMethodOverride", - "range": { - "startColumn": 8, - "endColumn": 14, - "lineCount": 1 - } - } - ], - "./git/remote.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 25, - "endColumn": 36, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 29, - "endColumn": 40, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 26, - "endColumn": 38, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 26, - "endColumn": 38, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 26, - "endColumn": 38, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 26, - "endColumn": 38, - "lineCount": 1 - } - } - ], - "./git/repo/base.py": [ - { - "code": "reportRedeclaration", - "range": { - "startColumn": 4, - "endColumn": 15, - "lineCount": 1 - } - }, - { - "code": "reportAttributeAccessIssue", - "range": { - "startColumn": 18, - "endColumn": 22, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 35, - "endColumn": 41, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 51, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 28, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 16, - "endColumn": 31, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 21, - "endColumn": 31, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 34, - "endColumn": 44, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 65, - "endColumn": 79, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 82, - "endColumn": 102, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 50, - "endColumn": 69, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 68, - "endColumn": 85, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 88, - "endColumn": 111, - "lineCount": 1 - } - }, - { - "code": "reportTypedDictNotRequiredAccess", - "range": { - "startColumn": 51, - "endColumn": 73, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 12, - "endColumn": 26, - "lineCount": 1 - } - }, - { - "code": "reportAssignmentType", - "range": { - "startColumn": 22, - "endColumn": 38, - "lineCount": 1 - } - } - ], - "./git/repo/fun.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 25, - "endColumn": 33, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 35, - "endColumn": 43, - "lineCount": 1 - } - }, - { - "code": "reportAssignmentType", - "range": { - "startColumn": 18, - "endColumn": 28, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 11, - "endColumn": 14, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 24, - "endColumn": 27, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 11, - "endColumn": 14, - "lineCount": 1 - } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 11, - "endColumn": 20, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 25, - "endColumn": 28, - "lineCount": 1 - } - }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 24, - "endColumn": 27, - "lineCount": 1 - } - } - ], - "./test/deprecation/test_basic.py": [ - { - "code": "reportUnusedExpression", - "range": { - "startColumn": 12, - "endColumn": 62, - "lineCount": 1 - } - } - ] - } + "files": { + "./git/config.py": [ + { + "code": "reportGeneralTypeIssues", + "range": { + "startColumn": 11, + "endColumn": 26, + "lineCount": 1 + } + }, + { + "code": "reportInvalidTypeVarUse", + "range": { + "startColumn": 43, + "endColumn": 45, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 41, + "endColumn": 48, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 32, + "endColumn": 39, + "lineCount": 1 + } + }, + { + "code": "reportCallIssue", + "range": { + "startColumn": 25, + "endColumn": 46, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 30, + "endColumn": 39, + "lineCount": 1 + } + } + ], + "./git/db.py": [ + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 12, + "lineCount": 1 + } + }, + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 14, + "lineCount": 1 + } + } + ], + "./git/index/base.py": [ + { + "code": "reportArgumentType", + "range": { + "startColumn": 30, + "endColumn": 36, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 28, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportAssignmentType", + "range": { + "startColumn": 38, + "endColumn": 76, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 51, + "endColumn": 57, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 55, + "endColumn": 61, + "lineCount": 1 + } + }, + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 12, + "lineCount": 1 + } + }, + { + "code": "reportAssignmentType", + "range": { + "startColumn": 20, + "endColumn": 46, + "lineCount": 1 + } + } + ], + "./git/objects/blob.py": [ + { + "code": "reportIncompatibleVariableOverride", + "range": { + "startColumn": 4, + "endColumn": 8, + "lineCount": 1 + } + } + ], + "./git/objects/commit.py": [ + { + "code": "reportIncompatibleVariableOverride", + "range": { + "startColumn": 4, + "endColumn": 8, + "lineCount": 1 + } + }, + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 31, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 20, + "endColumn": 33, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 20, + "endColumn": 30, + "lineCount": 1 + } + } + ], + "./git/objects/submodule/base.py": [ + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 19, + "endColumn": 24, + "lineCount": 1 + } + }, + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 19, + "endColumn": 24, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 23, + "endColumn": 25, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 23, + "endColumn": 25, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 52, + "endColumn": 84, + "lineCount": 1 + } + } + ], + "./git/objects/tag.py": [ + { + "code": "reportIncompatibleVariableOverride", + "range": { + "startColumn": 4, + "endColumn": 8, + "lineCount": 1 + } + } + ], + "./git/objects/tree.py": [ + { + "code": "reportIncompatibleVariableOverride", + "range": { + "startColumn": 4, + "endColumn": 8, + "lineCount": 1 + } + }, + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 31, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 19, + "endColumn": 83, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 15, + "endColumn": 54, + "lineCount": 1 + } + } + ], + "./git/objects/util.py": [ + { + "code": "reportAssignmentType", + "range": { + "startColumn": 23, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 22, + "endColumn": 26, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 30, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 15, + "endColumn": 54, + "lineCount": 1 + } + } + ], + "./git/refs/log.py": [ + { + "code": "reportArgumentType", + "range": { + "startColumn": 30, + "endColumn": 34, + "lineCount": 1 + } + }, + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 17, + "endColumn": 22, + "lineCount": 1 + } + }, + { + "code": "reportArgumentType", + "range": { + "startColumn": 28, + "endColumn": 30, + "lineCount": 1 + } + } + ], + "./git/refs/reference.py": [ + { + "code": "reportIncompatibleVariableOverride", + "range": { + "startColumn": 13, + "endColumn": 17, + "lineCount": 1 + } + } + ], + "./git/refs/symbolic.py": [ + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 15, + "endColumn": 20, + "lineCount": 1 + } + } + ], + "./git/refs/tag.py": [ + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 14, + "lineCount": 1 + } + }, + { + "code": "reportIncompatibleMethodOverride", + "range": { + "startColumn": 8, + "endColumn": 14, + "lineCount": 1 + } + } + ], + "./git/remote.py": [ + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 26, + "endColumn": 38, + "lineCount": 1 + } + }, + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 26, + "endColumn": 38, + "lineCount": 1 + } + }, + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 26, + "endColumn": 38, + "lineCount": 1 + } + }, + { + "code": "reportAttributeAccessIssue", + "range": { + "startColumn": 26, + "endColumn": 38, + "lineCount": 1 + } + } + ], + "./git/repo/base.py": [ + { + "code": "reportReturnType", + "range": { + "startColumn": 15, + "endColumn": 46, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 15, + "endColumn": 51, + "lineCount": 1 + } + }, + { + "code": "reportReturnType", + "range": { + "startColumn": 15, + "endColumn": 28, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 21, + "endColumn": 31, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 34, + "endColumn": 44, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 65, + "endColumn": 79, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 82, + "endColumn": 102, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 50, + "endColumn": 69, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 68, + "endColumn": 85, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 88, + "endColumn": 111, + "lineCount": 1 + } + }, + { + "code": "reportTypedDictNotRequiredAccess", + "range": { + "startColumn": 51, + "endColumn": 73, + "lineCount": 1 + } + } + ], + "./git/repo/fun.py": [ + { + "code": "reportReturnType", + "range": { + "startColumn": 11, + "endColumn": 20, + "lineCount": 1 + } + } + ], + "./test/deprecation/test_basic.py": [ + { + "code": "reportUnusedExpression", + "range": { + "startColumn": 12, + "endColumn": 62, + "lineCount": 1 + } + } + ] + } } diff --git a/git/cmd.py b/git/cmd.py index 9de2f6e8a..3d7446906 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -100,7 +100,7 @@ def handle_process_output( - process: "Git.AutoInterrupt" | Popen, + process: Union["Git.AutoInterrupt", Popen], stdout_handler: Union[ None, Callable[[AnyStr], None], @@ -395,9 +395,7 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: :raise git.exc.GitCommandError: If the return status is not 0. """ - if stderr is None: - stderr_b = b"" - stderr_b = force_bytes(data=stderr, encoding="utf-8") + stderr_b = force_bytes(data=stderr, encoding="utf-8") or b"" status: Union[int, None] if self.proc is not None: status = self.proc.wait() @@ -1180,52 +1178,112 @@ def version_info(self) -> Tuple[int, ...]: def execute( self, command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, *, as_process: Literal[True], + **subprocess_kwargs: Any, ) -> "AutoInterrupt": ... @overload def execute( self, command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, *, as_process: Literal[False] = False, - stdout_as_string: Literal[True], - ) -> Union[str, Tuple[int, str, str]]: ... + with_extended_output: Literal[False] = False, + stdout_as_string: Literal[True] = True, + with_stdout: Literal[True] = True, + **subprocess_kwargs: Any, + ) -> str: ... @overload def execute( self, command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, *, as_process: Literal[False] = False, - stdout_as_string: Literal[False] = False, - ) -> Union[bytes, Tuple[int, bytes, str]]: ... + with_extended_output: Literal[False] = False, + stdout_as_string: Literal[False], + universal_newlines: Literal[False] = False, + with_stdout: Literal[True] = True, + **subprocess_kwargs: Any, + ) -> bytes: ... @overload def execute( self, command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, *, - with_extended_output: Literal[False], - as_process: Literal[False], - stdout_as_string: Literal[True], - ) -> str: ... + as_process: Literal[False] = False, + with_extended_output: Literal[True], + stdout_as_string: Literal[True] = True, + with_stdout: Literal[True] = True, + **subprocess_kwargs: Any, + ) -> Tuple[int, str, str]: ... @overload def execute( self, command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, *, - with_extended_output: Literal[False], - as_process: Literal[False], + as_process: Literal[False] = False, + with_extended_output: Literal[True], stdout_as_string: Literal[False], - ) -> bytes: ... + universal_newlines: Literal[False] = False, + with_stdout: Literal[True] = True, + **subprocess_kwargs: Any, + ) -> Tuple[int, bytes, str]: ... + + @overload + def execute( + self, + command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, + *, + as_process: Literal[False] = False, + with_extended_output: Literal[True], + **subprocess_kwargs: Any, + ) -> Tuple[int, Union[str, bytes, None], str]: ... + + @overload + def execute( + self, + command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, + *, + as_process: Literal[False] = False, + with_extended_output: Literal[False] = False, + **subprocess_kwargs: Any, + ) -> Union[str, bytes, None]: ... + + @overload + def execute( + self, + command: Union[str, Sequence[Any]], + istream: Union[None, int, BinaryIO] = None, + with_extended_output: bool = False, + with_exceptions: bool = True, + as_process: bool = False, + output_stream: Union[None, BinaryIO] = None, + stdout_as_string: bool = True, + kill_after_timeout: Union[None, float] = None, + with_stdout: bool = True, + universal_newlines: bool = False, + shell: Union[None, bool] = None, + env: Union[None, Mapping[str, str]] = None, + max_chunk_size: int = io.DEFAULT_BUFFER_SIZE, + strip_newline_in_stdout: bool = True, + **subprocess_kwargs: Any, + ) -> Union[None, str, bytes, Tuple[int, Union[str, bytes, None], str], AutoInterrupt]: ... def execute( self, command: Union[str, Sequence[Any]], - istream: Union[None, BinaryIO] = None, + istream: Union[None, int, BinaryIO] = None, with_extended_output: bool = False, with_exceptions: bool = True, as_process: bool = False, @@ -1239,7 +1297,7 @@ def execute( max_chunk_size: int = io.DEFAULT_BUFFER_SIZE, strip_newline_in_stdout: bool = True, **subprocess_kwargs: Any, - ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], AutoInterrupt]: + ) -> Union[None, str, bytes, Tuple[int, Union[str, bytes, None], str], AutoInterrupt]: R"""Handle executing the command, and consume and return the returned information (stdout). @@ -1503,7 +1561,7 @@ def make_timeout_error() -> Union[str, bytes]: err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.' return err if universal_newlines else err.encode(defenc) - def communicate() -> Tuple[AnyStr, AnyStr]: + def communicate() -> Tuple[Union[str, bytes, None], Union[str, bytes, None]]: assert watchdog is not None assert kill_check is not None watchdog.start() @@ -1523,8 +1581,8 @@ def communicate() -> Tuple[AnyStr, AnyStr]: # Wait for the process to return. status = 0 - stdout_value: Union[str, bytes] = b"" - stderr_value: Union[str, bytes] = b"" + stdout_value: Union[str, bytes, None] = b"" + stderr_value: Union[str, bytes, None] = b"" newline = "\n" if universal_newlines else b"\n" try: if output_stream is None: @@ -1566,7 +1624,7 @@ def communicate() -> Tuple[AnyStr, AnyStr]: if self.GIT_PYTHON_TRACE == "full": cmdstr = " ".join(redacted_command) - def as_text(stdout_value: Union[bytes, str]) -> str: + def as_text(stdout_value: Union[bytes, str, None]) -> str: return not output_stream and safe_decode(stdout_value) or "" # END as_text @@ -1591,6 +1649,8 @@ def as_text(stdout_value: Union[bytes, str]) -> str: if isinstance(stdout_value, bytes) and stdout_as_string: # Could also be output_stream. stdout_value = safe_decode(stdout_value) + # stderr is always captured through PIPE. + assert stderr_value is not None # Allow access to the command's status code. if with_extended_output: return (status, stdout_value, safe_decode(stderr_value)) @@ -1829,7 +1889,7 @@ def _parse_object_header(self, header_line: str) -> Tuple[str, str, int]: raise ValueError("Failed to parse header: %r" % header_line) return (tokens[0], tokens[1], int(tokens[2])) - def _prepare_ref(self, ref: AnyStr) -> bytes: + def _prepare_ref(self, ref: object) -> bytes: # Required for command to separate refs on stdin, as bytes. if isinstance(ref, bytes): # Assume 40 bytes hexsha - bin-to-ascii for some reason returns bytes, not text. @@ -1856,7 +1916,7 @@ def _get_persistent_cmd(self, attr_name: str, cmd_name: str, *args: Any, **kwarg cmd = cast("Git.AutoInterrupt", cmd) return cmd - def __get_object_header(self, cmd: "Git.AutoInterrupt", ref: AnyStr) -> Tuple[str, str, int]: + def __get_object_header(self, cmd: "Git.AutoInterrupt", ref: Union[str, bytes]) -> Tuple[str, str, int]: if cmd.stdin and cmd.stdout: cmd.stdin.write(self._prepare_ref(ref)) cmd.stdin.flush() @@ -1864,7 +1924,7 @@ def __get_object_header(self, cmd: "Git.AutoInterrupt", ref: AnyStr) -> Tuple[st else: raise ValueError("cmd stdin was empty") - def get_object_header(self, ref: str) -> Tuple[str, str, int]: + def get_object_header(self, ref: Union[str, bytes]) -> Tuple[str, str, int]: """Use this method to quickly examine the type and size of the object behind the given ref. @@ -1878,7 +1938,7 @@ def get_object_header(self, ref: str) -> Tuple[str, str, int]: cmd = self._get_persistent_cmd("cat_file_header", "cat_file", batch_check=True) return self.__get_object_header(cmd, ref) - def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]: + def get_object_data(self, ref: Union[str, bytes]) -> Tuple[str, str, int, bytes]: """Similar to :meth:`get_object_header`, but returns object data as well. :return: @@ -1892,7 +1952,7 @@ def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]: del stream return (hexsha, typename, size, data) - def stream_object_data(self, ref: str) -> Tuple[str, str, int, "Git.CatFileContentStream"]: + def stream_object_data(self, ref: Union[str, bytes]) -> Tuple[str, str, int, "Git.CatFileContentStream"]: """Similar to :meth:`get_object_data`, but returns the data as a stream. :return: diff --git a/git/index/base.py b/git/index/base.py index a3c915242..560fc5e2c 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1235,7 +1235,7 @@ def _read_commit_editmsg(self) -> str: def _commit_editmsg_filepath(self) -> str: return osp.join(self.repo.common_dir, "COMMIT_EDITMSG") - def _flush_stdin_and_wait(cls, proc: "Popen[bytes]", ignore_stdout: bool = False) -> bytes: + def _flush_stdin_and_wait(self, proc: "Popen[bytes]", ignore_stdout: bool = False) -> bytes: stdin_IO = proc.stdin if stdin_IO: stdin_IO.flush() diff --git a/git/index/fun.py b/git/index/fun.py index 886e10de9..45c18ace4 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -46,7 +46,7 @@ from git.types import PathLike if TYPE_CHECKING: - from git.db import GitCmdObjectDB + from gitdb.db.base import ObjectDBR, ObjectDBW from git.objects.tree import TreeCacheTup from .base import IndexFile @@ -412,7 +412,7 @@ def read_cache( def write_tree_from_cache( - entries: List[IndexEntry], odb: "GitCmdObjectDB", sl: slice, si: int = 0 + entries: List[IndexEntry], odb: "ObjectDBW", sl: slice, si: int = 0 ) -> Tuple[bytes, List["TreeCacheTup"]]: R"""Create a tree from the given sorted list of entries and put the respective trees into the given object database. @@ -484,7 +484,7 @@ def _tree_entry_to_baseindexentry(tree_entry: "TreeCacheTup", stage: int) -> Bas return BaseIndexEntry((tree_entry[1], tree_entry[0], stage << CE_STAGESHIFT, tree_entry[2])) -def aggressive_tree_merge(odb: "GitCmdObjectDB", tree_shas: Sequence[bytes]) -> List[BaseIndexEntry]: +def aggressive_tree_merge(odb: "ObjectDBR", tree_shas: Sequence[bytes]) -> List[BaseIndexEntry]: R""" :return: List of :class:`~git.index.typ.BaseIndexEntry`\s representing the aggressive diff --git a/git/index/typ.py b/git/index/typ.py index 927633a9f..78f7c8ea5 100644 --- a/git/index/typ.py +++ b/git/index/typ.py @@ -9,12 +9,13 @@ from pathlib import Path from git.objects import Blob +from git.objects.base import IndexObject from .util import pack, unpack # typing ---------------------------------------------------------------------- -from typing import NamedTuple, Sequence, TYPE_CHECKING, Tuple, Union, cast +from typing import NamedTuple, Sequence, TYPE_CHECKING, Tuple, Type, TypeVar, Union, cast from git.types import PathLike @@ -22,6 +23,7 @@ from git.repo import Repo StageType = int +_T_IndexEntry = TypeVar("_T_IndexEntry", bound="BaseIndexEntry") # --------------------------------------------------------------------------------- @@ -104,15 +106,20 @@ class BaseIndexEntry(BaseIndexEntryHelper): """ def __new__( - cls, + cls: Type[_T_IndexEntry], inp_tuple: Union[ Tuple[int, bytes, int, PathLike], + Tuple[int, bytes, int, PathLike, bytes, bytes, int, int, int, int, int], Tuple[int, bytes, int, PathLike, bytes, bytes, int, int, int, int, int, int], ], - ) -> "BaseIndexEntry": + ) -> _T_IndexEntry: """Override ``__new__`` to allow construction from a tuple for backwards compatibility.""" - return super().__new__(cls, *inp_tuple) + if len(inp_tuple) == 4: + return BaseIndexEntryHelper.__new__(cls, *inp_tuple) + if len(inp_tuple) == 11: + return BaseIndexEntryHelper.__new__(cls, *inp_tuple) + return BaseIndexEntryHelper.__new__(cls, *inp_tuple) def __str__(self) -> str: return "%o %s %i\t%s" % (self.mode, self.hexsha, self.stage, self.path) @@ -148,7 +155,7 @@ def intent_to_add(self) -> bool: return (self.extended_flags & CE_EXT_INTENT_TO_ADD) > 0 @classmethod - def from_blob(cls, blob: Blob, stage: int = 0) -> "BaseIndexEntry": + def from_blob(cls, blob: IndexObject, stage: int = 0) -> "BaseIndexEntry": """:return: Fully equipped BaseIndexEntry at the given stage""" return cls((blob.mode, blob.binsha, stage << CE_STAGESHIFT, blob.path)) @@ -192,10 +199,10 @@ def from_base(cls, base: "BaseIndexEntry") -> "IndexEntry": Instance of type :class:`BaseIndexEntry`. """ time = pack(">LL", 0, 0) - return IndexEntry((base.mode, base.binsha, base.flags, base.path, time, time, 0, 0, 0, 0, 0)) # type: ignore[arg-type] + return IndexEntry((base.mode, base.binsha, base.flags, base.path, time, time, 0, 0, 0, 0, 0)) @classmethod - def from_blob(cls, blob: Blob, stage: int = 0) -> "IndexEntry": + def from_blob(cls, blob: IndexObject, stage: int = 0) -> "IndexEntry": """:return: Minimal entry resembling the given blob object""" time = pack(">LL", 0, 0) return IndexEntry( @@ -211,5 +218,5 @@ def from_blob(cls, blob: Blob, stage: int = 0) -> "IndexEntry": 0, 0, blob.size, - ) # type: ignore[arg-type] + ) ) diff --git a/git/objects/base.py b/git/objects/base.py index faf600c6b..1188ec0c9 100644 --- a/git/objects/base.py +++ b/git/objects/base.py @@ -18,7 +18,7 @@ from typing import Any, TYPE_CHECKING, Union -from git.types import AnyGitObject, GitObjectTypeString, PathLike +from git.types import AnyGitObject, GitObjectTypeString, PathLike, SupportsWrite if TYPE_CHECKING: from gitdb.base import OStream @@ -200,7 +200,7 @@ def data_stream(self) -> "OStream": """ return self.repo.odb.stream(self.binsha) - def stream_data(self, ostream: "OStream") -> "Object": + def stream_data(self, ostream: SupportsWrite[bytes]) -> "Object": """Write our data directly to the given output stream. :param ostream: diff --git a/git/objects/commit.py b/git/objects/commit.py index 45843eac2..0348d3299 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -502,7 +502,7 @@ def _interpret_trailers( ) -> str: message_bytes = message if isinstance(message, bytes) else message.encode(encoding, errors="strict") cmd = [repo.git.GIT_PYTHON_GIT_EXECUTABLE, "interpret-trailers", *trailer_args] - proc: Git.AutoInterrupt = repo.git.execute( # type: ignore[call-overload] + proc: Git.AutoInterrupt = repo.git.execute( cmd, as_process=True, istream=PIPE, diff --git a/git/objects/fun.py b/git/objects/fun.py index ad5fbd59b..7ef229990 100644 --- a/git/objects/fun.py +++ b/git/objects/fun.py @@ -30,7 +30,7 @@ if TYPE_CHECKING: from _typeshed import ReadableBuffer - from git import GitCmdObjectDB + from gitdb.db.base import ObjectDBR EntryTup = Tuple[bytes, int, str] # Same as TreeCacheTup in tree.py. EntryTupOrNone = Union[EntryTup, None] @@ -166,7 +166,7 @@ def _to_full_path(item: EntryTupOrNone, path_prefix: str) -> EntryTupOrNone: def traverse_trees_recursive( - odb: "GitCmdObjectDB", tree_shas: Sequence[Union[bytes, None]], path_prefix: str + odb: "ObjectDBR", tree_shas: Sequence[Union[bytes, None]], path_prefix: str ) -> List[Tuple[EntryTupOrNone, ...]]: """ :return: @@ -253,7 +253,7 @@ def traverse_trees_recursive( return out -def traverse_tree_recursive(odb: "GitCmdObjectDB", tree_sha: bytes, path_prefix: str) -> List[EntryTup]: +def traverse_tree_recursive(odb: "ObjectDBR", tree_sha: bytes, path_prefix: str) -> List[EntryTup]: """ :return: List of entries of the tree pointed to by the binary `tree_sha`. diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 8308e4459..ba281c499 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -14,7 +14,7 @@ import stat import sys import uuid -import urllib +import urllib.parse import git from git.cmd import Git @@ -1769,7 +1769,7 @@ def iter_items( # END handle critical error # Make sure we are looking at a submodule object. - if type(sm) is not git.objects.submodule.base.Submodule: + if type(sm) is not Submodule: continue # Fill in remaining info - saves time as it doesn't have to be parsed again. diff --git a/git/objects/tag.py b/git/objects/tag.py index 88671d316..18b4a9ca4 100644 --- a/git/objects/tag.py +++ b/git/objects/tag.py @@ -23,6 +23,8 @@ from typing import List, TYPE_CHECKING, Union +from git.types import AnyGitObject + if sys.version_info >= (3, 8): from typing import Literal else: @@ -61,7 +63,7 @@ def __init__( self, repo: "Repo", binsha: bytes, - object: Union[None, base.Object] = None, + object: Union[None, AnyGitObject] = None, tag: Union[None, str] = None, tagger: Union[None, Actor] = None, tagged_date: Union[int, None] = None, diff --git a/git/refs/reference.py b/git/refs/reference.py index 0c4327225..7d6c62cf5 100644 --- a/git/refs/reference.py +++ b/git/refs/reference.py @@ -26,7 +26,7 @@ def require_remote_ref_path(func: Callable[..., _T]) -> Callable[..., _T]: """A decorator raising :exc:`ValueError` if we are not a valid remote, based on the path.""" - def wrapper(self: T_References, *args: Any) -> _T: + def wrapper(self: SymbolicReference, *args: Any) -> _T: if not self.is_remote(): raise ValueError("ref path does not point to a remote reference: %s" % self.path) return func(self, *args) diff --git a/git/remote.py b/git/remote.py index e2d5cbc1d..2ddf11af0 100644 --- a/git/remote.py +++ b/git/remote.py @@ -38,6 +38,7 @@ Sequence, TYPE_CHECKING, Type, + TypeVar, Union, cast, overload, @@ -50,6 +51,8 @@ from git.objects.submodule.base import UpdateProgress from git.repo.base import Repo +_T_RemoteName = TypeVar("_T_RemoteName", bound=Union[str, "Remote"]) + flagKeyLiteral = Literal[" ", "!", "+", "-", "*", "=", "t", "?"] # ------------------------------------------------------------- @@ -820,19 +823,20 @@ def add(cls, repo: "Repo", name: str, url: str, **kwargs: Any) -> "Remote": return cls.create(repo, name, url, **kwargs) @classmethod - def remove(cls, repo: "Repo", name: str) -> str: + def remove(cls, repo: "Repo", name: _T_RemoteName) -> _T_RemoteName: """Remove the remote with the given name. :return: The passed remote name to remove """ repo.git.remote("rm", name) - if isinstance(name, cls): - name._clear_cache() + remote = name + if isinstance(remote, cls): + remote._clear_cache() return name @classmethod - def rm(cls, repo: "Repo", name: str) -> str: + def rm(cls, repo: "Repo", name: _T_RemoteName) -> _T_RemoteName: """Alias of remove. Remove the remote with the given name. @@ -901,7 +905,7 @@ def _get_fetch_info_from_stderr( kill_after_timeout=kill_after_timeout, ) - stderr_text = progress.error_lines and "\n".join(progress.error_lines) or "" + stderr_text = "\n".join(progress.error_lines) proc.wait(stderr=stderr_text) if stderr_text: _logger.warning("Error lines received while fetching: %s", stderr_text) @@ -973,7 +977,7 @@ def stdout_handler(line: str) -> None: decode_streams=False, kill_after_timeout=kill_after_timeout, ) - stderr_text = progress.error_lines and "\n".join(progress.error_lines) or "" + stderr_text = "\n".join(progress.error_lines) try: proc.wait(stderr=stderr_text) except Exception as e: diff --git a/git/repo/base.py b/git/repo/base.py index f326266d6..29922e118 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -18,6 +18,7 @@ import warnings import gitdb +import gitdb.util from gitdb.db.loose import LooseObjectDB from gitdb.exc import BadObject @@ -33,7 +34,7 @@ from git.index import IndexFile from git.objects import Submodule, RootModule, Commit from git.refs import HEAD, Head, Reference, TagReference -from git.remote import Remote, add_progress, to_progress_instance +from git.remote import Remote, _T_RemoteName, add_progress, to_progress_instance from git.util import ( Actor, cygpath, @@ -95,7 +96,7 @@ class BlameEntry(NamedTuple): - commit: Dict[str, Commit] + commit: Commit linenos: range orig_path: Optional[str] orig_linenos: range @@ -396,7 +397,7 @@ def __init__( self._working_tree_dir = None # END working dir handling - self.working_dir: PathLike = self._working_tree_dir or self.common_dir + self.working_dir = self._working_tree_dir or self.common_dir self.git = self.GitCommandWrapperType(self.working_dir) if common_dir_env is not None: self.git.update_environment(GIT_DIR=os.fspath(self.git_dir), GIT_COMMON_DIR=os.fspath(self.common_dir)) @@ -718,7 +719,7 @@ def create_remote(self, name: str, url: str, **kwargs: Any) -> Remote: """ return Remote.create(self, name, url, **kwargs) - def delete_remote(self, remote: "Remote") -> str: + def delete_remote(self, remote: _T_RemoteName) -> _T_RemoteName: """Delete the given remote.""" return Remote.remove(self, remote) @@ -1504,7 +1505,7 @@ def _clone( git: "Git", url: PathLike, path: PathLike, - odb_default_type: Type[GitCmdObjectDB], + odb_default_type: Type[LooseObjectDB], progress: Union["RemoteProgress", "UpdateProgress", Callable[..., "RemoteProgress"], None] = None, multi_options: Optional[List[str]] = None, allow_unsafe_protocols: bool = False, @@ -1711,7 +1712,7 @@ def clone_from( def archive( self, ostream: Union[TextIO, BinaryIO], - treeish: Optional[str] = None, + treeish: Union[str, Commit, None] = None, prefix: Optional[str] = None, allow_unsafe_options: bool = False, allow_unsafe_protocols: bool = False, diff --git a/git/repo/fun.py b/git/repo/fun.py index eb0d8075a..a565054bc 100644 --- a/git/repo/fun.py +++ b/git/repo/fun.py @@ -40,11 +40,10 @@ from git.types import AnyGitObject, Literal, PathLike if TYPE_CHECKING: - from git.db import GitCmdObjectDB + from gitdb.db import CompoundDB, LooseObjectDB from git.objects import Commit from git.refs.reference import Reference from git.refs.log import RefLog, RefLogEntry - from git.refs.tag import Tag from .base import Repo @@ -158,7 +157,7 @@ def find_submodule_git_dir(d: PathLike) -> Optional[PathLike]: return path if is_git_dir(path) else None -def short_to_long(odb: "GitCmdObjectDB", hexsha: str) -> Optional[bytes]: +def short_to_long(odb: Union["CompoundDB", "LooseObjectDB"], hexsha: str) -> Optional[bytes]: """ :return: Long hexadecimal sha1 from the given less than 40 byte hexsha, or ``None`` if no @@ -261,7 +260,7 @@ def name_to_object(repo: "Repo", name: str, return_ref: bool = False) -> Union[A return Object.new_from_sha(repo, hex_to_bin(hexsha)) -def deref_tag(tag: "Tag") -> AnyGitObject: +def deref_tag(tag: AnyGitObject) -> AnyGitObject: """Recursively dereference a tag and return the resulting object.""" while True: try: @@ -272,7 +271,7 @@ def deref_tag(tag: "Tag") -> AnyGitObject: return tag -def to_commit(obj: Object) -> "Commit": +def to_commit(obj: AnyGitObject) -> "Commit": """Convert the given object to a commit if possible and return it.""" if obj.type == "tag": obj = deref_tag(obj) @@ -521,7 +520,7 @@ def _find_commit_by_message( if rev is None: commits = _all_ref_commits(repo) else: - commits = _reachable_commits([to_commit(cast(Object, rev))]) + commits = _reachable_commits([to_commit(rev)]) # END handle starting point for commit in commits: @@ -541,7 +540,7 @@ def _all_ref_commits(repo: "Repo") -> Iterator["Commit"]: starts = [] for ref in repo.references: try: - starts.append(to_commit(cast(Object, ref.object))) + starts.append(to_commit(ref.object)) except (BadName, ValueError): pass # END skip refs that do not point to commits @@ -589,7 +588,7 @@ def _index_lookup(repo: "Repo", spec: str) -> AnyGitObject: def _tree_lookup(obj: AnyGitObject, path: str) -> AnyGitObject: if obj.type != "tree": - obj = to_commit(cast(Object, obj)).tree + obj = to_commit(obj).tree # END get tree if not path: return obj @@ -604,9 +603,9 @@ def _peel(obj: AnyGitObject, output_type: str, repo: "Repo", rev: str) -> AnyGit if output_type == "object": return obj if output_type == "commit": - return to_commit(cast(Object, obj)) + return to_commit(obj) if output_type == "tree": - return to_commit(cast(Object, obj)).tree if obj.type != "tree" else obj + return to_commit(obj).tree if obj.type != "tree" else obj if output_type == "blob": obj = deref_tag(obj) if obj.type == "tag" else obj if obj.type == output_type: diff --git a/git/types.py b/git/types.py index 100fff43f..31d40bf3b 100644 --- a/git/types.py +++ b/git/types.py @@ -48,6 +48,22 @@ _T = TypeVar("_T") """Type variable used internally in GitPython.""" +_T_Stream_co = TypeVar("_T_Stream_co", str, bytes, covariant=True) +_T_Stream_contra = TypeVar("_T_Stream_contra", str, bytes, contravariant=True) + + +class SupportsRead(Protocol[_T_Stream_co]): + """A stream supporting reads, without requiring the full IO interface.""" + + def read(self, __size: int = -1) -> _T_Stream_co: ... + + +class SupportsWrite(Protocol[_T_Stream_contra]): + """A stream supporting writes, including writers that return None.""" + + def write(self, __data: _T_Stream_contra) -> object: ... + + AnyGitObject = Union["Commit", "Tree", "TagObject", "Blob"] """Union of the :class:`~git.objects.base.Object`-based types that represent actual git object types. diff --git a/git/util.py b/git/util.py index a80e667c7..f72e3d7c1 100644 --- a/git/util.py +++ b/git/util.py @@ -68,7 +68,6 @@ from typing import ( Any, AnyStr, - BinaryIO, Callable, Dict, Generator, @@ -101,6 +100,8 @@ PathLike, Protocol, SupportsIndex, + SupportsRead, + SupportsWrite, Total_TD, runtime_checkable, ) @@ -253,7 +254,7 @@ def rmfile(path: PathLike) -> None: os.remove(path) -def stream_copy(source: BinaryIO, destination: BinaryIO, chunk_size: int = 512 * 1024) -> int: +def stream_copy(source: SupportsRead[AnyStr], destination: SupportsWrite[AnyStr], chunk_size: int = 512 * 1024) -> int: """Copy all data from the `source` stream into the `destination` stream in chunks of size `chunk_size`. diff --git a/pyproject.toml b/pyproject.toml index b7c437bf3..fbcde3611 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -19,7 +19,7 @@ testpaths = "test" # Space separated list of paths from root e.g test tests doc # filterwarnings ignore::WarningType # ignores those warnings [tool.mypy] -files = ["git/", "test/deprecation/"] +files = ["git/", "test/deprecation/", "test/test_typing.py"] disallow_untyped_defs = true no_implicit_optional = true warn_redundant_casts = true @@ -40,6 +40,8 @@ pythonVersion = "3.7" include = [ "git", "test/deprecation", + "test/test_typing.py", + "test/test_git.py", ] extraPaths = [ "gitdb", diff --git a/test-requirements.txt b/test-requirements.txt index e2443825c..0fabac809 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,7 +1,7 @@ coverage[toml] basedpyright==1.39.9 ; python_version >= "3.9" and sys_platform != "cygwin" ddt >= 1.1.1, != 1.4.3 -mock ; python_version < "3.8" +mock # Also needed to typecheck the Python 3.7 import branches. mypy==1.18.2 ; python_version >= "3.9" # pin mypy version to avoid new errors pre-commit pytest >= 7.3.1 diff --git a/test/test_git.py b/test/test_git.py index b19652363..14fc7dfc3 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -137,24 +137,24 @@ def test_it_raises_errors(self): self.assertRaises(GitCommandError, self.git.this_does_not_exist) def test_it_transforms_kwargs_into_git_command_arguments(self): - self.assertEqual(["-s"], self.git.transform_kwargs(**{"s": True})) - self.assertEqual(["-s", "5"], self.git.transform_kwargs(**{"s": 5})) - self.assertEqual([], self.git.transform_kwargs(**{"s": None})) + self.assertEqual(["-s"], self.git.transform_kwargs(s=True)) + self.assertEqual(["-s", "5"], self.git.transform_kwargs(s=5)) + self.assertEqual([], self.git.transform_kwargs(s=None)) - self.assertEqual(["--max-count"], self.git.transform_kwargs(**{"max_count": True})) - self.assertEqual(["--max-count=5"], self.git.transform_kwargs(**{"max_count": 5})) - self.assertEqual(["--max-count=0"], self.git.transform_kwargs(**{"max_count": 0})) - self.assertEqual([], self.git.transform_kwargs(**{"max_count": None})) + self.assertEqual(["--max-count"], self.git.transform_kwargs(max_count=True)) + self.assertEqual(["--max-count=5"], self.git.transform_kwargs(max_count=5)) + self.assertEqual(["--max-count=0"], self.git.transform_kwargs(max_count=0)) + self.assertEqual([], self.git.transform_kwargs(max_count=None)) # Multiple args are supported by using lists/tuples. self.assertEqual( ["-L", "1-3", "-L", "12-18"], - self.git.transform_kwargs(**{"L": ("1-3", "12-18")}), + self.git.transform_kwargs(L=("1-3", "12-18")), ) - self.assertEqual(["-C", "-C"], self.git.transform_kwargs(**{"C": [True, True, None, False]})) + self.assertEqual(["-C", "-C"], self.git.transform_kwargs(C=[True, True, None, False])) # Order is undefined. - res = self.git.transform_kwargs(**{"s": True, "t": True}) + res = self.git.transform_kwargs(s=True, t=True) self.assertEqual({"-s", "-t"}, set(res)) def test_check_unsafe_options_normalizes_kwargs(self): @@ -221,7 +221,9 @@ def test_option_candidates_include_falsey_non_boolean_values(self): candidates = Git._option_candidates(kwargs=kwargs) self.assertEqual(candidates, ["--pathspec-from-file"]) - self.assertEqual(self.git.transform_kwargs(**kwargs), ["--pathspec-from-file=0"]) + self.assertEqual( + self.git.transform_kwargs(split_single_char_options=True, **kwargs), ["--pathspec-from-file=0"] + ) with self.assertRaises(UnsafeOptionError): Git.check_unsafe_options( options=candidates, @@ -634,6 +636,7 @@ def test_initial_refresh_from_bad_git_path_env_warn(self, case): with mock.patch.dict(os.environ, env_vars): with self.assertLogs(cmd.__name__, logging.CRITICAL) as ctx: refresh() + assert ctx is not None self.assertEqual(len(ctx.records), 1) message = ctx.records[0].getMessage() self.assertRegex(message, r"\ABad git executable.\n") @@ -753,6 +756,7 @@ def test_refresh_with_good_absolute_git_path_arg(self): def test_refresh_with_good_relative_git_path_arg(self): """Good relative path arg is resolved to absolute path and set.""" absolute_path = shutil.which("git") + assert absolute_path is not None dirname, basename = osp.split(absolute_path) with cwd(dirname): diff --git a/test/test_typing.py b/test/test_typing.py new file mode 100644 index 000000000..101c05520 --- /dev/null +++ b/test/test_typing.py @@ -0,0 +1,91 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Runtime and static checks for public APIs with related input/output types.""" + +from io import BytesIO, StringIO +import subprocess +import sys +from typing import List, Tuple, TYPE_CHECKING + +import pytest + +from git import Git +from git.exc import GitCommandError +from git.index.typ import BaseIndexEntry, IndexEntry +from git.util import stream_copy + + +def test_index_entry_constructor_shapes() -> None: + class DerivedEntry(IndexEntry): + pass + + short = (0o100644, b"\0" * 20, 0, "file") + full = short + (b"\0" * 8, b"\0" * 8, 1, 2, 3, 4, 5) + entries: List[IndexEntry] = [IndexEntry(short), IndexEntry(full), IndexEntry(full + (0x4000,))] + derived: DerivedEntry = DerivedEntry(short) + + assert all(type(entry) is IndexEntry for entry in entries) + assert [entry.size for entry in entries] == [0, 5, 5] + assert [entry.skip_worktree for entry in entries] == [False, False, True] + assert type(derived) is DerivedEntry + assert IndexEntry.from_base(BaseIndexEntry(short)) == entries[0] + + +def test_stream_copy_minimal_writer() -> None: + class Writer: + def __init__(self) -> None: + self.data = b"" + + def write(self, data: bytes) -> None: + self.data += data + + writer = Writer() + assert stream_copy(BytesIO(b"payload"), writer, chunk_size=3) == 7 + assert writer.data == b"payload" + text = StringIO() + assert stream_copy(StringIO("payload"), text, chunk_size=3) == 7 + assert text.getvalue() == "payload" + + +def test_process_wait_with_no_previous_stderr() -> None: + process = Git().execute( + [sys.executable, "-c", "import sys; sys.stderr.write('failure'); sys.exit(1)"], + as_process=True, + istream=subprocess.DEVNULL, + shell=False, + ) + with pytest.raises(GitCommandError, match="failure"): + process.wait(stderr=None) + + +def test_execute_output_types() -> None: + git = Git() + command = [sys.executable, "-c", "print('payload', end='')"] + text: str = git.execute(command, with_exceptions=False, shell=False) + binary: bytes = git.execute(command, stdout_as_string=False, shell=False) + extended_text: Tuple[int, str, str] = git.execute(command, with_extended_output=True, shell=False) + extended_binary: Tuple[int, bytes, str] = git.execute( + command, with_extended_output=True, stdout_as_string=False, shell=False + ) + assert text == "payload" + assert binary == b"payload" + assert extended_text == (0, text, "") + assert extended_binary == (0, binary, "") + + +if TYPE_CHECKING: + from git import Remote, Repo + from git.repo.base import BlameEntry + from git.objects import Commit, Submodule + + repo = Repo() + remote = Remote(repo, "origin") + removed_names: List[str] = [Remote.remove(repo, "origin"), Remote.rm(repo, "origin"), repo.delete_remote("origin")] + removed_remotes: List[Remote] = [Remote.remove(repo, remote), Remote.rm(repo, remote), repo.delete_remote(remote)] + blame = BlameEntry(repo.head.commit, range(1), "file", range(1)) + commit: Commit = blame.commit + submodule_entry: IndexEntry = IndexEntry.from_blob(Submodule(repo, b"\0" * 20)) + Git().get_object_header(b"HEAD") + Git().get_object_data(b"HEAD") + Git().stream_object_data(b"HEAD") From 22296682d66fb0b2a76e549670d0ec6ef7447ab1 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 13 Sep 2026 11:01:15 +0200 Subject: [PATCH 14/19] fix: preserve implicit boolean config keys (#2237) This was mostly a rubber-stamp, knowing the the whole implementation is quite a hack that is held together with ductape. Ideally, it will just work well enough at some point, to gain time for v4 to be made. GitConfigParser discarded keys written without an assignment. Reading such an option raised NoOptionError, and editing an unrelated setting silently removed it. Git treats a bare key as true but an explicitly empty value as false, so representing both as an empty string would lose their meaning. Store bare entries as None, following RawConfigParser's allow_no_value representation. Raw get/items access preserves the distinction, while get_value/get_values return an empty string as requested. Convert None to true and an empty string to false in getboolean, retaining the standard boolean spellings. Write None entries without an equals sign and exclude them from string validation and include-path expansion. The existing ordered multi-dict preserves repeated bare and assigned entries together. Update the regression that expected bare color.ui to disappear, and add a Git-backed read-modify-write test covering trailing whitespace, EOF without a newline, quoted and unquoted empty values, repeated keys, and a valueless non-path option in an include section. Both regressions failed before the fix. Compare Git's NUL-delimited listing before and after an unrelated edit and check the resulting repeated values with --type=bool --get-all. Git reference: checkout 1630431f326e15fcde608827b5ff38422528eb59, t/t1300-config.sh tests for novalue.variable and emptyvalue.variable, and parse.c:git_parse_maybe_bool_text. Runtime comparison used Git 2.50.1 (Apple Git-155). Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/config.py | 51 ++++++++++++++++++++++++++++++++---------- test/test_config.py | 54 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/git/config.py b/git/config.py index 0da90bab2..d6f2706f1 100644 --- a/git/config.py +++ b/git/config.py @@ -45,7 +45,7 @@ from git.repo.base import Repo T_ConfigParser = TypeVar("T_ConfigParser", bound="GitConfigParser") -T_OMD_value = TypeVar("T_OMD_value", str, bytes, int, float, bool) +T_OMD_value = TypeVar("T_OMD_value", str, bytes, int, float, bool, None) if sys.version_info[:3] < (3, 7, 2): # typing.Ordereddict not added until Python 3.7.2. @@ -291,6 +291,12 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder): :note: If used as a context manager, this will release the locked file. + + :note: + Options without a value are stored as ``None`` and written without ``=``. + :meth:`get_value` and :meth:`get_values` return an empty string for them, + while :meth:`getboolean` returns ``True``. An explicit empty value is + stored as an empty string and reads as ``False`` with :meth:`getboolean`. """ # { Configuration @@ -348,7 +354,7 @@ def __init__( Reference to repository to use if ``[includeIf]`` sections are found in configuration files. """ - cp.RawConfigParser.__init__(self, dict_type=_OMD) + cp.RawConfigParser.__init__(self, dict_type=_OMD, allow_no_value=True) self._dict: Callable[..., _OMD] self._defaults: _OMD self._sections: _OMD @@ -587,8 +593,12 @@ def parse_value(value: str) -> str: # Preserves multiple values for duplicate optnames. cursect.add(optname, optval) else: - # Check if it's an option with no value - it's just ignored by git. - if not self.OPTVALUEONLY.match(line): + # A valueless option is an implicit boolean true, not an empty value. + mo = self.OPTVALUEONLY.match(line) + if mo: + optname = self.optionxform(mo.group("option").rstrip()) + cursect.add(optname, None) + else: if not e: e = cp.ParsingError(fpname) e.append(lineno, repr(line)) @@ -625,6 +635,7 @@ def _all_items(section: str) -> List[Tuple[str, str]]: for key, values in self._sections[section].items_all() if key != "__name__" for value in values + if value is not None ] paths = [] @@ -760,13 +771,16 @@ def _write(self, fp: IO) -> None: def write_section(name: str, section_dict: _OMD) -> None: fp.write(("[%s]\n" % name).encode(defenc)) - values: Sequence[str] # Runtime only gets str in tests, but should be whatever _OMD stores. - v: str + values: List[Any] + v: Any for key, values in section_dict.items_all(): if key == "__name__": continue for v in values: + if v is None: + fp.write(("\t%s\n" % key).encode(defenc)) + continue value = self._value_to_string(v) if any(char in value for char in '\n\t\b\\"#;') or value[:1].isspace() or value[-1:].isspace(): value = value.replace("\\", "\\\\").replace('"', '\\"') @@ -783,11 +797,11 @@ def write_section(name: str, section_dict: _OMD) -> None: for name, value in self._sections.items(): write_section(name, value) - def items(self, section_name: str) -> List[Tuple[str, str]]: # type: ignore[override] + def items(self, section_name: str) -> List[Tuple[str, Union[str, None]]]: # type: ignore[override] """:return: list((option, value), ...) pairs of all items in the given section""" return [(k, v) for k, v in super().items(section_name) if k != "__name__"] - def items_all(self, section_name: str) -> List[Tuple[str, List[str]]]: + def items_all(self, section_name: str) -> List[Tuple[str, List[Union[str, None]]]]: """:return: list((option, [values...]), ...) pairs of all items in the given section""" rv = _OMD(self._defaults) @@ -841,6 +855,8 @@ def write(self) -> None: for key, values in section.items_all(): if key != "__name__": for raw_value in values: + if raw_value is None: + continue if "\r" in self._value_to_string(raw_value) or "\x00" in self._value_to_string(raw_value): raise ValueError("Git config values must not contain CR or NUL") @@ -877,7 +893,6 @@ def read_only(self) -> bool: """:return: ``True`` if this instance may change the configuration file""" return self._read_only - # FIXME: Figure out if default or return type can really include bool. def get_value( self, section: str, @@ -894,7 +909,7 @@ def get_value( did not exist. :return: - A properly typed value, either int, float or string + A properly typed value, either int, float, string or bool :raise TypeError: In case the value could not be understood. @@ -925,7 +940,7 @@ def get_values( in case the option did not exist. :return: - A list of properly typed values, either int, float or string + A list of properly typed values, either int, float, string or bool :raise TypeError: In case the value could not be understood. @@ -941,7 +956,19 @@ def get_values( return [self._string_to_value(valuestr) for valuestr in lst] - def _string_to_value(self, valuestr: str) -> Union[int, float, str, bool]: + def _convert_to_boolean(self, value: Union[str, None]) -> bool: + if value is None: + return True + if value == "": + return False + try: + return self.BOOLEAN_STATES[value.lower()] + except KeyError: + raise ValueError("Not a boolean: %s" % value) from None + + def _string_to_value(self, valuestr: Union[str, None]) -> Union[int, float, str, bool]: + if valuestr is None: + return "" types = (int, float) for numtype in types: try: diff --git a/test/test_config.py b/test/test_config.py index 3031721de..2910f3e6c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -864,8 +864,58 @@ def test_empty_config_value(self): assert cr.get_value("core", "filemode"), "Should read keys with values" - with self.assertRaises(cp.NoOptionError): - cr.get_value("color", "ui") + self.assertTrue(cr.has_option("color", "ui")) + self.assertIsNone(cr.get("color", "ui")) + self.assertEqual(cr.get_value("color", "ui"), "") + self.assertIs(cr.getboolean("color", "ui"), True) + + @with_rw_directory + def test_implicit_boolean_round_trip(self, rw_dir): + config_path = osp.join(rw_dir, "config") + with open(config_path, "wb") as config_file: + config_file.write( + b"[include]\n" + b"\toptional\n" + b"[flag]\n" + b"\timplicit\n" + b"\ttrailing-space \n" + b"\ttrailing-tab\t\n" + b"\tempty =\n" + b'\tquoted = ""\n' + b"\tmultiple = false\n" + b"\tmultiple\n" + b"\tmultiple =\n" + b"\tmultiple" + ) + git_config = ["git", "config", "--file", config_path] + original = subprocess.check_output(git_config + ["--null", "--list"]) + + with GitConfigParser(config_path, read_only=False) as config: + for option in ("implicit", "trailing-space", "trailing-tab"): + self.assertIsNone(config.get("flag", option)) + self.assertEqual(config.get_value("flag", option), "") + self.assertIs(config.getboolean("flag", option), True) + for option in ("empty", "quoted"): + self.assertEqual(config.get("flag", option), "") + self.assertEqual(config.get_value("flag", option), "") + self.assertIs(config.getboolean("flag", option), False) + self.assertEqual(config.get_values("flag", "multiple"), [False, "", "", ""]) + self.assertIsNone(dict(config.items("flag"))["multiple"]) + self.assertEqual(dict(config.items_all("flag"))["multiple"], ["false", None, "", None]) + config.set_value("other", "value", "updated") + + self.assertEqual( + subprocess.check_output(git_config + ["--null", "--list"]), + original + b"other.value\nupdated\0", + ) + self.assertEqual( + subprocess.check_output(git_config + ["--type=bool", "--get-all", "flag.multiple"]), + b"false\ntrue\nfalse\ntrue\n", + ) + with GitConfigParser(config_path) as config: + self.assertIs(config.getboolean("flag", "implicit"), True) + self.assertIs(config.getboolean("flag", "empty"), False) + self.assertEqual(dict(config.items_all("flag"))["multiple"], ["false", None, "", None]) def test_config_with_quotes(self): cr = GitConfigParser(fixture_path("git_config_with_quotes"), read_only=True) From 2d4f0683341cb77e75a8656484d3c09a441f9d22 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 13 Sep 2026 18:04:41 +0200 Subject: [PATCH 15/19] fix: reject comments after implicit boolean config keys Note that this is just a fixup, on a huge hack which is the native git-config parsing. Let's just hope this holds up until v4. GitConfigParser accepted entries such as "enabled # comment" and "enabled ; comment" even though Git rejects them. The comment became part of the option name, and an equals sign or colon inside the comment could make the entry look like an assignment. Silently stripping the comment would also accept configuration that Git considers invalid. Exclude both comment markers from the shared option-name expression and require a full-line match for valueless options. The assignment pattern cannot cross a comment marker, and the valueless fallback cannot accept just the valid-looking prefix. Such lines now raise the existing ParsingError during reading or an attempted edit. Ordinary bare keys retain their implicit true value and round-trip behavior. Add six regression cases covering both markers, spaces, tabs, adjacent comments, and assignment delimiters inside comments. Compare rejection with git config, check both getboolean and an unrelated edit raise ParsingError, and verify that the failed edit leaves the original bytes untouched. All six cases failed before full-line matching was added. Git reference: checkout 1630431f326e15fcde608827b5ff38422528eb59, config.c:get_value. Without an assignment, that parser requires the line to end after the key and optional whitespace. Runtime comparisons used Git 2.50.1 (Apple Git-155), which rejected all six inputs with exit status 128. Validation on Python 3.12.14: 42 configuration tests and six regression subtests passed, with two existing skips. Ruff lint and formatting and git diff --check passed. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/config.py | 4 ++-- test/test_config.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/git/config.py b/git/config.py index d6f2706f1..cb130579a 100644 --- a/git/config.py +++ b/git/config.py @@ -310,7 +310,7 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder): re_comment = re.compile(r"^\s*[#;]") # } END configuration - optvalueonly_source = r"\s*(?P