From d702d9a6f598bc5eb701b468940db56afb978059 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Mon, 20 Apr 2026 06:11:16 -0700 Subject: [PATCH 1/9] feat(submodule): add deinit method to Submodule (#2014) Mirrors the pattern of `Submodule.add`, `Submodule.update`, and `Submodule.remove` by exposing `git submodule deinit` as a first-class method, so callers no longer need the `repo.git.submodule('deinit', ...)` workaround noted in the issue. The method delegates to `git submodule deinit [--force] -- ` via `self.repo.git.submodule`, decorated with `@unbare_repo` to match the other mutating Submodule methods. It unregisters the submodule from `.git/config` and clears the working-tree directory while leaving `.gitmodules` and `.git/modules/` intact, so a later `update()` can re-initialize. --- git/objects/submodule/base.py | 31 +++++++++++++++++++++++++++++++ test/test_submodule.py | 15 +++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index d183672db..33b1a29a0 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -49,6 +49,7 @@ Callable, Dict, Iterator, + List, Mapping, Sequence, TYPE_CHECKING, @@ -1267,6 +1268,36 @@ def remove( return self + @unbare_repo + def deinit(self, force: bool = False) -> "Submodule": + """Run ``git submodule deinit`` on this submodule. + + This is a thin wrapper around ``git submodule deinit ``, paralleling + :meth:`add`, :meth:`update`, and :meth:`remove`. It unregisters the + submodule (removes its entry from ``.git/config`` and empties the + working-tree directory) without deleting the submodule from + ``.gitmodules`` or its checked-out repository under ``.git/modules/``. + A subsequent :meth:`update` will re-initialize the submodule from the + retained contents. + + :param force: + If ``True``, pass ``--force`` to ``git submodule deinit``. This + allows deinitialization even when the submodule's working tree has + local modifications that would otherwise block the command. + + :return: + self + + :note: + Doesn't work in bare repositories. + """ + args: List[str] = [] + if force: + args.append("--force") + args.extend(["--", str(self.path)]) + self.repo.git.submodule("deinit", *args) + return self + def set_parent_commit(self, commit: Union[Commit_ish, str, None], check: bool = True) -> "Submodule": """Set this instance to use the given commit whose tree is supposed to contain the ``.gitmodules`` blob. diff --git a/test/test_submodule.py b/test/test_submodule.py index 47647f2a1..a3705f452 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -718,6 +718,21 @@ def test_iter_items_from_invalid_hash(self): next(it) self.assertIsNone(ctx.exception.value) + @with_rw_directory + def test_deinit_calls_git_submodule(self, rwdir): + repo = git.Repo.init(rwdir) + submodule = Submodule(repo, b"\0" * 20, name="module", path="module") + + with mock.patch.object(Git, "submodule", create=True) as git_submodule: + submodule.deinit() + + git_submodule.assert_called_once_with("deinit", "--", submodule.path) + git_submodule.reset_mock() + + submodule.deinit(force=True) + + git_submodule.assert_called_once_with("deinit", "--force", "--", submodule.path) + @with_rw_repo(k_no_subm_tag, bare=False) def test_first_submodule(self, rwrepo): assert len(list(rwrepo.iter_submodules())) == 0 From 0c0cc8acab16733ec294af115aa909c6d8d9738f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 16 Jul 2026 06:44:05 +0200 Subject: [PATCH 2/9] review --- git/objects/submodule/base.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 33b1a29a0..9899b1eac 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -1272,13 +1272,13 @@ def remove( def deinit(self, force: bool = False) -> "Submodule": """Run ``git submodule deinit`` on this submodule. - This is a thin wrapper around ``git submodule deinit ``, paralleling - :meth:`add`, :meth:`update`, and :meth:`remove`. It unregisters the - submodule (removes its entry from ``.git/config`` and empties the - working-tree directory) without deleting the submodule from - ``.gitmodules`` or its checked-out repository under ``.git/modules/``. - A subsequent :meth:`update` will re-initialize the submodule from the - retained contents. + This is a thin wrapper around ``git submodule deinit ``, + which unregisters the submodule (removes its entry from + ``.git/config`` and empties the working-tree directory) + without deleting the submodule from ``.gitmodules`` + or its checked-out repository under ``.git/modules/``. + A subsequent :meth:`update` will re-initialize the + submodule from the retained contents. :param force: If ``True``, pass ``--force`` to ``git submodule deinit``. This From 6d0b6a4609b15bb74d45abec2e3b79bdb4632634 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 16 Jul 2026 05:09:09 +0200 Subject: [PATCH 3/9] typing: introduce sensible basedpyright defaults That way it dosesn't all light up immediately in dev mode. --- git/cmd.py | 2 +- git/util.py | 46 ++++++++++++++++++++++++++++------------------ pyproject.toml | 11 +++++++++++ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 150c9d16f..1868862d3 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1358,7 +1358,7 @@ def execute( # Allow the user to have the command executed in their working dir. try: - cwd = self._working_dir or os.getcwd() # type: Union[None, str] + cwd = self._working_dir or os.getcwd() # type: Optional[PathLike] if not os.access(str(cwd), os.X_OK): cwd = None except FileNotFoundError: diff --git a/git/util.py b/git/util.py index 11c1a0b15..8584b0e7f 100644 --- a/git/util.py +++ b/git/util.py @@ -217,7 +217,7 @@ def rmtree(path: PathLike) -> None: couldn't be deleted are read-only. Windows will not remove them in that case. """ - def handler(function: Callable, path: PathLike, _excinfo: Any) -> None: + def handler(function: Callable[[str], Any], path: str, _excinfo: Any) -> None: """Callback for :func:`shutil.rmtree`. This works as either a ``onexc`` or ``onerror`` style callback. @@ -401,7 +401,7 @@ def _cygexpath(drive: Optional[str], path: str, expand_vars: bool = True) -> str return p_str.replace("\\", "/") -_cygpath_parsers: Tuple[Tuple[Pattern[str], Callable, bool], ...] = ( +_cygpath_parsers: Tuple[Tuple[Pattern[str], Callable[..., str], bool], ...] = ( # See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx # and: https://www.cygwin.com/cygwin-ug-net/using.html#unc-paths ( @@ -508,7 +508,7 @@ def get_user_id() -> str: return "%s@%s" % (getpass.getuser(), platform.node()) -def finalize_process(proc: Union[subprocess.Popen, "Git.AutoInterrupt"], **kwargs: Any) -> None: +def finalize_process(proc: Union["subprocess.Popen[Any]", "Git.AutoInterrupt"], **kwargs: Any) -> None: """Wait for the process (clone, fetch, pull or push) and handle its errors accordingly.""" # TODO: No close proc-streams?? @@ -520,19 +520,21 @@ def expand_path(p: None, expand_vars: bool = ...) -> None: ... @overload -def expand_path(p: PathLike, expand_vars: bool = ...) -> str: +def expand_path(p: PathLike, expand_vars: bool = ...) -> Optional[PathLike]: # TODO: Support for Python 3.5 has been dropped, so these overloads can be improved. ... def expand_path(p: Union[None, PathLike], expand_vars: bool = True) -> Optional[PathLike]: - if isinstance(p, Path): - return p.resolve() + if p is None: + return None try: - p = osp.expanduser(p) # type: ignore[arg-type] + if isinstance(p, Path): + return p.resolve() + expanded_path = osp.expanduser(os.fspath(p)) if expand_vars: - p = osp.expandvars(p) - return osp.normpath(osp.abspath(p)) + expanded_path = osp.expandvars(expanded_path) + return osp.normpath(osp.abspath(expanded_path)) except Exception: return None @@ -767,7 +769,7 @@ class CallableRemoteProgress(RemoteProgress): __slots__ = ("_callable",) - def __init__(self, fn: Callable) -> None: + def __init__(self, fn: Callable[..., Any]) -> None: self._callable = fn super().__init__() @@ -846,7 +848,7 @@ def _main_actor( cls, env_name: str, env_email: str, - config_reader: Union[None, "GitConfigParser", "SectionConstraint"] = None, + config_reader: Union[None, "GitConfigParser", "SectionConstraint[GitConfigParser]"] = None, ) -> "Actor": actor = Actor("", "") user_id = None # We use this to avoid multiple calls to getpass.getuser(). @@ -882,7 +884,9 @@ def default_name() -> str: return actor @classmethod - def committer(cls, config_reader: Union[None, "GitConfigParser", "SectionConstraint"] = None) -> "Actor": + def committer( + cls, config_reader: Union[None, "GitConfigParser", "SectionConstraint[GitConfigParser]"] = None + ) -> "Actor": """ :return: :class:`Actor` instance corresponding to the configured committer. It @@ -897,7 +901,9 @@ def committer(cls, config_reader: Union[None, "GitConfigParser", "SectionConstra return cls._main_actor(cls.env_committer_name, cls.env_committer_email, config_reader) @classmethod - def author(cls, config_reader: Union[None, "GitConfigParser", "SectionConstraint"] = None) -> "Actor": + def author( + cls, config_reader: Union[None, "GitConfigParser", "SectionConstraint[GitConfigParser]"] = None + ) -> "Actor": """Same as :meth:`committer`, but defines the main author. It may be specified in the environment, but defaults to the committer.""" return cls._main_actor(cls.env_author_name, cls.env_author_email, config_reader) @@ -980,11 +986,11 @@ class IndexFileSHA1Writer: __slots__ = ("f", "sha1") - def __init__(self, f: IO) -> None: + def __init__(self, f: IO[bytes]) -> None: self.f = f self.sha1 = make_sha(b"") - def write(self, data: AnyStr) -> int: + def write(self, data: bytes) -> int: self.sha1.update(data) return self.f.write(data) @@ -1181,6 +1187,7 @@ def __new__(cls, id_attr: str, prefix: str = "") -> "IterableList[T_IterableObj] return super().__new__(cls) def __init__(self, id_attr: str, prefix: str = "") -> None: + super().__init__() self._id_attr = id_attr self._prefix = prefix @@ -1210,7 +1217,9 @@ def __getattr__(self, attr: str) -> T_IterableObj: # END for each item return list.__getattribute__(self, attr) - def __getitem__(self, index: Union[SupportsIndex, int, slice, str]) -> T_IterableObj: # type: ignore[override] + def __getitem__( # type: ignore[override] # pyright: ignore[reportIncompatibleMethodOverride] + self, index: Union[SupportsIndex, int, slice, str] + ) -> T_IterableObj: if isinstance(index, int): return list.__getitem__(self, index) elif isinstance(index, slice): @@ -1288,7 +1297,7 @@ def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> IterableList[T_I :return: list(Item,...) list of item instances """ - out_list: IterableList = IterableList(cls._id_attribute_) + out_list: IterableList[T_IterableObj] = IterableList(cls._id_attribute_) out_list.extend(cls.iter_items(repo, *args, **kwargs)) return out_list @@ -1297,7 +1306,8 @@ class IterableClassWatcher(type): """Metaclass that issues :exc:`DeprecationWarning` when :class:`git.util.Iterable` is subclassed.""" - def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None: + def __init__(cls, name: str, bases: Tuple[type, ...], clsdict: Dict[str, Any]) -> None: + super().__init__(name, bases, clsdict) for base in bases: if type(base) is IterableClassWatcher: warnings.warn( diff --git a/pyproject.toml b/pyproject.toml index 149f2dc92..324630002 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,17 @@ exclude = ["^git/ext/gitdb"] module = "gitdb.*" ignore_missing_imports = true +[tool.basedpyright] +typeCheckingMode = "standard" +pythonVersion = "3.7" +extraPaths = [ + "git/ext/gitdb", + "git/ext/gitdb/gitdb/ext/smmap", +] +exclude = [ + "git/ext/gitdb", +] + [tool.coverage.run] source = ["git"] From b9d82c08210d107af9e2bdf7e368fb2f9dfec296 Mon Sep 17 00:00:00 2001 From: "GPT 5.6" Date: Thu, 16 Jul 2026 07:19:56 +0200 Subject: [PATCH 4/9] fix: make `submodule.update()` after `submodule.deinit()` work Git's `submodule deinit` command removes the submodule checkout but retains its repository under the parent repository's `.git/modules` directory. Submodule.update() previously treated the missing checkout as a completely uninitialized submodule and attempted to clone it again. The clone could not reuse the existing module repository, preventing a deinitialized submodule from being initialized again through GitPython. Detect a valid retained repository before entering the clone path. Restore the checkout's `.git` file and the module repository's worktree configuration, reset the retained repository to recreate its index and working tree, and restore the submodule URL in the parent configuration. Continue using the existing clone behavior when no valid retained repository is available. Co-authored-by: Sebastian Thiel --- git/objects/submodule/base.py | 210 ++++++++++++++++++---------------- test/test_submodule.py | 145 +++++++++++++++++++++++ 2 files changed, 255 insertions(+), 100 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 9899b1eac..7984db340 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -739,122 +739,132 @@ def update( mrepo = None # END init mrepo + def fetch_remotes(module_repo: "Repo") -> None: + rmts = module_repo.remotes + len_rmts = len(rmts) + for i, remote in enumerate(rmts): + op = FETCH + if i == 0: + op |= BEGIN + # END handle start + + progress.update( + op, + i, + len_rmts, + prefix + "Fetching remote %s of submodule %r" % (remote, self.name), + ) + # =============================== + if not dry_run: + remote.fetch(progress=progress) + # END handle dry-run + # =============================== + if i == len_rmts - 1: + op |= END + # END handle end + progress.update( + op, + i, + len_rmts, + prefix + "Done fetching remote of submodule %r" % self.name, + ) + # END fetch new data + try: # ENSURE REPO IS PRESENT AND UP-TO-DATE ####################################### try: mrepo = self.module() - rmts = mrepo.remotes - len_rmts = len(rmts) - for i, remote in enumerate(rmts): - op = FETCH - if i == 0: - op |= BEGIN - # END handle start - - progress.update( - op, - i, - len_rmts, - prefix + "Fetching remote %s of submodule %r" % (remote, self.name), - ) - # =============================== - if not dry_run: - remote.fetch(progress=progress) - # END handle dry-run - # =============================== - if i == len_rmts - 1: - op |= END - # END handle end - progress.update( - op, - i, - len_rmts, - prefix + "Done fetching remote of submodule %r" % self.name, - ) - # END fetch new data + fetch_remotes(mrepo) except InvalidGitRepositoryError: mrepo = None if not init: return self # END early abort if init is not allowed - # There is no git-repository yet - but delete empty paths. checkout_module_abspath = self.abspath - if not dry_run and osp.isdir(checkout_module_abspath): + module_abspath = self._module_abspath(self.repo, self.path, self.name) + + # ``git submodule deinit`` leaves the repository in + # ``.git/modules`` and empties the checkout. Reconnect that retained + # repository instead of trying to clone over it. + if not dry_run and osp.isdir(module_abspath): try: - os.rmdir(checkout_module_abspath) - except OSError as e: - raise OSError( - "Module directory at %r does already exist and is non-empty" % checkout_module_abspath - ) from e - # END handle OSError - # END handle directory removal - - # Don't check it out at first - nonetheless it will create a local - # branch according to the remote-HEAD if possible. - progress.update( - BEGIN | CLONE, - 0, - 1, - prefix - + "Cloning url '%s' to '%s' in submodule %r" % (self.url, checkout_module_abspath, self.name), - ) - if not dry_run: - if self.url.startswith("."): - url = urllib.parse.urljoin(self.repo.remotes.origin.url + "/", self.url) + git.Repo(module_abspath) + except InvalidGitRepositoryError: + pass else: - url = self.url - mrepo = self._clone_repo( - self.repo, - url, - self.path, - self.name, - n=True, - env=env, - multi_options=clone_multi_options, - allow_unsafe_options=allow_unsafe_options, - allow_unsafe_protocols=allow_unsafe_protocols, + if osp.lexists(checkout_module_abspath) and ( + osp.islink(checkout_module_abspath) + or not osp.isdir(checkout_module_abspath) + or os.listdir(checkout_module_abspath) + ): + raise OSError( + "Module directory at %r does already exist and is non-empty" % checkout_module_abspath + ) + os.makedirs(checkout_module_abspath, exist_ok=True) + self._write_git_file_and_module_config(checkout_module_abspath, module_abspath) + mrepo = git.Repo(checkout_module_abspath) + mrepo.head.reset(mrepo.head.commit, index=True, working_tree=True) + fetch_remotes(mrepo) + with self.repo.config_writer() as writer: + writer.set_value(sm_section(self.name), "url", self.url) + + if mrepo is None: + # There is no git-repository yet - but delete empty paths. + if not dry_run and osp.isdir(checkout_module_abspath): + try: + os.rmdir(checkout_module_abspath) + except OSError as e: + raise OSError( + "Module directory at %r does already exist and is non-empty" % checkout_module_abspath + ) from e + # END handle directory removal + + # Don't check it out at first - nonetheless it will create a local + # branch according to the remote-HEAD if possible. + progress.update( + BEGIN | CLONE, + 0, + 1, + prefix + + "Cloning url '%s' to '%s' in submodule %r" % (self.url, checkout_module_abspath, self.name), ) - # END handle dry-run - progress.update( - END | CLONE, - 0, - 1, - prefix + "Done cloning to %s" % checkout_module_abspath, - ) - - if not dry_run: - # See whether we have a valid branch to check out. - try: - mrepo = cast("Repo", mrepo) - # Find a remote which has our branch - we try to be flexible. - remote_branch = find_first_remote_branch(mrepo.remotes, self.branch_name) - local_branch = mkhead(mrepo, self.branch_path) - - # Have a valid branch, but no checkout - make sure we can figure - # that out by marking the commit with a null_sha. - local_branch.set_object(Object(mrepo, self.NULL_BIN_SHA)) - # END initial checkout + branch creation - - # Make sure HEAD is not detached. - mrepo.head.set_reference( - local_branch, - logmsg="submodule: attaching head to %s" % local_branch, + if not dry_run: + if self.url.startswith("."): + url = urllib.parse.urljoin(self.repo.remotes.origin.url + "/", self.url) + else: + url = self.url + mrepo = self._clone_repo( + self.repo, + url, + self.path, + self.name, + n=True, + env=env, + multi_options=clone_multi_options, + allow_unsafe_options=allow_unsafe_options, + allow_unsafe_protocols=allow_unsafe_protocols, ) - mrepo.head.reference.set_tracking_branch(remote_branch) - except (IndexError, InvalidGitRepositoryError): - _logger.warning("Failed to checkout tracking branch %s", self.branch_path) - # END handle tracking branch - - # NOTE: Have to write the repo config file as well, otherwise the - # default implementation will be offended and not update the - # repository. Maybe this is a good way to ensure it doesn't get into - # our way, but we want to stay backwards compatible too... It's so - # redundant! - with self.repo.config_writer() as writer: - writer.set_value(sm_section(self.name), "url", self.url) - # END handle dry_run + progress.update(END | CLONE, 0, 1, prefix + "Done cloning to %s" % checkout_module_abspath) + + if not dry_run: + # See whether we have a valid branch to check out. + try: + mrepo = cast("Repo", mrepo) + remote_branch = find_first_remote_branch(mrepo.remotes, self.branch_name) + local_branch = mkhead(mrepo, self.branch_path) + local_branch.set_object(Object(mrepo, self.NULL_BIN_SHA)) + mrepo.head.set_reference( + local_branch, + logmsg="submodule: attaching head to %s" % local_branch, + ) + mrepo.head.reference.set_tracking_branch(remote_branch) + except (IndexError, InvalidGitRepositoryError): + _logger.warning("Failed to checkout tracking branch %s", self.branch_path) + + with self.repo.config_writer() as writer: + writer.set_value(sm_section(self.name), "url", self.url) # END handle initialization # DETERMINE SHAS TO CHECK OUT diff --git a/test/test_submodule.py b/test/test_submodule.py index 0cd7c8e50..265ee2ffb 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -728,6 +728,151 @@ def test_deinit_calls_git_submodule(self, rwdir): git_submodule.assert_called_once_with("deinit", "--force", "--", submodule.path) + @with_rw_directory + def test_update_after_deinit(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + touch(osp.join(source_path, "file")) + source_repo.index.add(["file"]) + source_repo.index.commit("initial commit") + + parent_path = osp.join(rwdir, "parent") + parent_repo = git.Repo.init(parent_path) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + + submodule.deinit() + assert not submodule.module_exists() + assert osp.isdir(osp.join(parent_repo.git_dir, "modules", submodule.name)) + + submodule.update() + + assert submodule.module_exists() + assert submodule.module().head.commit == source_repo.head.commit + assert osp.isfile(osp.join(submodule.abspath, "file")) + + @with_rw_directory + def test_update_to_latest_revision_after_deinit_fetches_remote(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + source_repo.git.commit(m="initial commit", allow_empty=True) + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + submodule.deinit() + + touch(osp.join(source_path, "new-file")) + source_repo.index.add(["new-file"]) + source_repo.index.commit("advance remote") + + submodule.update(to_latest_revision=True) + + assert submodule.module().head.commit == source_repo.head.commit + assert osp.isfile(osp.join(submodule.abspath, "new-file")) + + @with_rw_directory + def test_update_after_deinit_fetches_new_gitlink_commit(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + source_repo.git.commit(m="initial commit", allow_empty=True) + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + submodule.deinit() + + touch(osp.join(source_path, "new-file")) + source_repo.index.add(["new-file"]) + source_repo.index.commit("advance remote") + parent_repo.git.update_index( + "--cacheinfo", + f"160000,{source_repo.head.commit.hexsha},{submodule.path}", + ) + parent_repo.index.commit("advance submodule") + + submodule = parent_repo.submodule(submodule.name) + submodule.update() + + assert submodule.module().head.commit == source_repo.head.commit + assert osp.isfile(osp.join(submodule.abspath, "new-file")) + + @with_rw_directory + def test_update_after_deinit_refuses_non_empty_checkout(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + tracked_file = osp.join(source_path, "file") + with open(tracked_file, "w") as fp: + fp.write("submodule content") + source_repo.index.add(["file"]) + source_repo.index.commit("initial commit") + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + submodule.deinit() + + checkout_file = osp.join(submodule.abspath, "file") + with open(checkout_file, "w") as fp: + fp.write("user content") + + with pytest.raises(OSError, match="does already exist and is non-empty"): + submodule.update() + + with open(checkout_file) as fp: + assert fp.read() == "user content" + assert not osp.exists(osp.join(submodule.abspath, ".git")) + + @with_rw_directory + def test_update_after_deinit_without_init_does_not_restore_checkout(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + source_repo.git.commit(m="initial commit", allow_empty=True) + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + submodule.deinit() + + assert submodule.update(init=False) is submodule + assert not submodule.module_exists() + assert not osp.exists(osp.join(submodule.abspath, ".git")) + + @with_rw_directory + def test_dry_run_update_after_deinit_does_not_restore_checkout(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + source_repo.git.commit(m="initial commit", allow_empty=True) + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("module", "module", source_path) + parent_repo.index.commit("add submodule") + submodule.deinit() + + assert submodule.update(dry_run=True) is submodule + assert not submodule.module_exists() + assert not osp.exists(osp.join(submodule.abspath, ".git")) + + @with_rw_directory + def test_update_after_deinit_restores_nested_checkout(self, rwdir): + source_path = osp.join(rwdir, "source") + source_repo = git.Repo.init(source_path) + touch(osp.join(source_path, "file")) + source_repo.index.add(["file"]) + source_repo.index.commit("initial commit") + + parent_repo = git.Repo.init(osp.join(rwdir, "parent")) + submodule = parent_repo.create_submodule("nested/module", "deps/module", source_path) + parent_repo.index.commit("add nested submodule") + submodule.deinit() + + submodule.update() + + module_repo = submodule.module() + assert module_repo.head.commit == source_repo.head.commit + assert osp.isfile(osp.join(submodule.abspath, "file")) + assert osp.samefile(module_repo.git_dir, osp.join(parent_repo.git_dir, "modules", submodule.name)) + @with_rw_repo(k_no_subm_tag, bare=False) def test_first_submodule(self, rwrepo): assert len(list(rwrepo.iter_submodules())) == 0 From 5bc256060cfaf8f32100780f17e0846f3e69d8b7 Mon Sep 17 00:00:00 2001 From: Byron Date: Mon, 20 Jul 2026 09:06:41 +0200 Subject: [PATCH 5/9] upate contributing guide to avoid agent impersonation --- CONTRIBUTING.md | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 60e34a651..76f276323 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,25 +20,16 @@ A contribution that works only narrowly but lowers the quality of the codebase may be declined. The maintainers may not always be able to provide detailed feedback. -## AI-assisted contributions +## Prevent agent impersonation -If AI edits files for you, disclose it in the pull request description and commit -metadata. Prefer making the agent identity part of the commit, for example by using -an AI author such as `$agent $version ` or a co-author via -a `Co-authored-by: ` trailer. +AI agents communicating through a person's account must identify themselves, for +example in issue or PR descriptions and comments. AI assistance that does not replace +the person as the speaker, such as proofreading or wording polish, does not require +identification. -Agents operating through a person's GitHub account must identify themselves. For -example, comments posted by an agent should say so directly with phrases like -`AI agent on behalf of : ...`. - -Fully AI-generated comments on pull requests or issues must also be disclosed. -Undisclosed AI-generated comments may lead to the pull request or issue being closed. - -AI-assisted proofreading or wording polish does not need disclosure, but it is still -courteous to mention it when the AI materially influenced the final text. - -Automated or "full-auto" AI contributions without a human responsible for reviewing -and standing behind the work may be closed. +Attributing AI assistance in commit metadata, for example with a `Co-authored-by` +trailer, is welcome but not required. Code is reviewed the same way regardless of its +origin. ## Fuzzing Test Specific Documentation From 406b98e1695dfaa706304103b0d524515ace2ebf Mon Sep 17 00:00:00 2001 From: Siesta Date: Sun, 31 May 2026 21:42:58 +0800 Subject: [PATCH 6/9] fix: respect core.hooksPath for commit hooks Use git rev-parse --git-path when resolving commit hook paths so GitPython follows Git's core.hooksPath configuration.\n\nAdd regression coverage for a pre-commit hook stored in a custom hooks path.\n\nFixes #2083 --- git/index/fun.py | 12 +++++++++++- test/test_index.py | 26 ++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/git/index/fun.py b/git/index/fun.py index 629c19b1e..71dd96e7f 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -64,6 +64,16 @@ def hook_path(name: str, git_dir: PathLike) -> str: return osp.join(git_dir, "hooks", name) +def _commit_hook_path(name: str, index: "IndexFile") -> str: + """:return: path to the named commit hook, respecting Git's core.hooksPath.""" + hp = index.repo.git.rev_parse("--git-path", f"hooks/{name}") + if osp.isabs(hp): + return hp + + base_dir = index.repo.working_dir or index.repo.git_dir + return osp.abspath(osp.join(base_dir, hp)) + + def _has_file_extension(path: str) -> str: return osp.splitext(path)[1] @@ -82,7 +92,7 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: :raise git.exc.HookExecutionError: """ - hp = hook_path(name, index.repo.git_dir) + hp = _commit_hook_path(name, index) if not os.access(hp, os.X_OK): return diff --git a/test/test_index.py b/test/test_index.py index 3be750dbb..ea8aa81ca 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -1160,6 +1160,32 @@ def test_pre_commit_hook_success(self, rw_repo): _make_hook(index.repo.git_dir, "pre-commit", "exit 0") index.commit("This should not fail") + @pytest.mark.xfail( + type(_win_bash_status) is WinBashStatus.Absent, + reason="Can't run a hook on Windows without bash.exe.", + raises=HookExecutionError, + ) + @pytest.mark.xfail( + type(_win_bash_status) is WinBashStatus.WslNoDistro, + reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", + raises=HookExecutionError, + ) + @with_rw_repo("HEAD", bare=True) + def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo): + index = rw_repo.index + hooks_dir = Path(index.repo.git_dir, "custom-hooks") + hooks_dir.mkdir() + hp = hooks_dir / "pre-commit" + hp.write_text(HOOKS_SHEBANG + "echo 'ran custom hook' >custom-hook-output.txt", encoding="utf-8") + os.chmod(hp, 0o744) + + with index.repo.config_writer() as writer: + writer.set_value("core", "hooksPath", "custom-hooks") + + index.commit("This should run the custom hook") + output = Path(rw_repo.git_dir, "custom-hook-output.txt").read_text(encoding="utf-8") + self.assertEqual(output, "ran custom hook\n") + @pytest.mark.xfail( type(_win_bash_status) is WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", From 9bc287a2b1eb331b6051d2ba704a603c3e0ddc6f Mon Sep 17 00:00:00 2001 From: Codex GPT-5 Date: Mon, 20 Jul 2026 10:24:12 +0200 Subject: [PATCH 7/9] Address review feedback about hook resolution - Review feedback: resolve core.hooksPath only when configured, avoid an unconditional git rev-parse dependency, and cover relative paths in a non-bare repository. - Read the effective Git configuration directly so unconfigured repositories retain the legacy .git/hooks lookup and configured relative paths resolve from the directory where hooks execute. Exercise both guarantees in the hook tests. - Review feedback: core.hooksPath can point outside index.repo.working_dir, where Path.relative_to() raises on Windows and prevents a valid hook from running. - Build the Bash argument with os.path.relpath so hooks in parent or other absolute locations remain executable. Fall back to the absolute POSIX-form path when Windows cannot form a relative path across drives, and add a focused command-construction regression test. --- git/index/fun.py | 21 ++++++++++++++------- test/test_index.py | 38 ++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/git/index/fun.py b/git/index/fun.py index 71dd96e7f..5d52486f9 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -66,12 +66,13 @@ def hook_path(name: str, git_dir: PathLike) -> str: def _commit_hook_path(name: str, index: "IndexFile") -> str: """:return: path to the named commit hook, respecting Git's core.hooksPath.""" - hp = index.repo.git.rev_parse("--git-path", f"hooks/{name}") - if osp.isabs(hp): - return hp + with index.repo.config_reader() as config: + hooks_dir = config.get("core", "hooksPath", fallback="") - base_dir = index.repo.working_dir or index.repo.git_dir - return osp.abspath(osp.join(base_dir, hp)) + if not hooks_dir: + return hook_path(name, index.repo.git_dir) + + return osp.abspath(osp.join(index.repo.working_dir, osp.expanduser(hooks_dir), name)) def _has_file_extension(path: str) -> str: @@ -104,8 +105,14 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: if sys.platform == "win32" and not _has_file_extension(hp): # Windows only uses extensions to determine how to open files # (doesn't understand shebangs). Try using bash to run the hook. - relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix() - cmd = ["bash.exe", relative_hp] + try: + bash_hp = osp.relpath(hp, index.repo.working_dir) + except ValueError: + # Different drives have no relative path on Windows. Git Bash accepts + # an absolute path in this form, although a relative path is preferable + # because it also works with the Windows Subsystem for Linux wrapper. + bash_hp = hp + cmd = ["bash.exe", Path(bash_hp).as_posix()] process = safer_popen( cmd + list(args), diff --git a/test/test_index.py b/test/test_index.py index ea8aa81ca..881dec19f 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -1099,9 +1099,35 @@ class Mocked: def test_run_commit_hook(self, rw_repo): index = rw_repo.index _make_hook(index.repo.git_dir, "fake-hook", "echo 'ran fake hook' >output.txt") - run_commit_hook("fake-hook", index) - output = Path(rw_repo.git_dir, "output.txt").read_text(encoding="utf-8") - self.assertEqual(output, "ran fake hook\n") + output = Path(rw_repo.git_dir, "output.txt") + with mock.patch.object(Repo, "config_level", ("repository",)): + with mock.patch.object(Git, "execute", side_effect=AssertionError("hook lookup must not run git")): + run_commit_hook("fake-hook", index) + self.assertEqual(output.read_text(encoding="utf-8"), "ran fake hook\n") + + output.unlink() + with index.repo.config_writer() as writer: + writer.set_value("core", "hooksPath", "") + run_commit_hook("fake-hook", index) + + self.assertEqual(output.read_text(encoding="utf-8"), "ran fake hook\n") + + @with_rw_directory + def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir): + root = Path(rw_dir).resolve() + repo = Repo.init(root / "repo") + hooks_dir = root / "hooks" + _make_hook(root, "fake-hook", "exit 0") + with repo.config_writer() as writer: + writer.set_value("core", "hooksPath", str(hooks_dir)) + + with mock.patch("git.index.fun.sys.platform", "win32"): + with mock.patch("git.index.fun.safer_popen") as popen, mock.patch("git.index.fun.handle_process_output"): + popen.return_value.returncode = 0 + run_commit_hook("fake-hook", repo.index) + + command = popen.call_args[0][0] + self.assertEqual(command, ["bash.exe", "../hooks/fake-hook"]) @ddt.data((False,), (True,)) @with_rw_directory @@ -1170,10 +1196,10 @@ def test_pre_commit_hook_success(self, rw_repo): reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", raises=HookExecutionError, ) - @with_rw_repo("HEAD", bare=True) + @with_rw_repo("HEAD") def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo): index = rw_repo.index - hooks_dir = Path(index.repo.git_dir, "custom-hooks") + hooks_dir = Path(index.repo.working_dir, "custom-hooks") hooks_dir.mkdir() hp = hooks_dir / "pre-commit" hp.write_text(HOOKS_SHEBANG + "echo 'ran custom hook' >custom-hook-output.txt", encoding="utf-8") @@ -1183,7 +1209,7 @@ def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo): writer.set_value("core", "hooksPath", "custom-hooks") index.commit("This should run the custom hook") - output = Path(rw_repo.git_dir, "custom-hook-output.txt").read_text(encoding="utf-8") + output = Path(rw_repo.working_dir, "custom-hook-output.txt").read_text(encoding="utf-8") self.assertEqual(output, "ran custom hook\n") @pytest.mark.xfail( From 1ed1b924f4e2d2ee7bab296df77b978af21853f1 Mon Sep 17 00:00:00 2001 From: "GPT 5.6" Date: Mon, 20 Jul 2026 14:13:22 +0200 Subject: [PATCH 8/9] fix: validate config section delimiters GHSA-3rp5-jjmw-4wv2 identified that configuration section names could alter the structure of serialized config despite the existing control-character checks. Reject unquoted closing section delimiters across all writer entry points while preserving valid delimiters inside quoted subsections and the existing option-name behavior. Add regression coverage for unsafe plain and quoted-subsection-shaped names as well as a valid quoted subsection. Git baseline: a23bace963d508bd96983cc637131392d3face18. Git config parsing treats an unquoted closing bracket as the end of a basic section header, while its extended form permits brackets inside a quoted subsection and requires a final bracket after the closing quote. Co-authored-by: Sebastian Thiel --- git/config.py | 16 +++++++++++++++ test/test_config.py | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/git/config.py b/git/config.py index 64f501424..821710ea3 100644 --- a/git/config.py +++ b/git/config.py @@ -897,6 +897,22 @@ def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> s def _assure_config_name_safe(self, name: "cp._SectionName", label: str) -> None: if isinstance(name, str) and UNSAFE_CONFIG_CHARS_RE.search(name): raise ValueError("Git config %s names must not contain CR, LF, or NUL" % label) + if label == "section" and isinstance(name, str): + in_quotes = False + escaped = False + for index, char in enumerate(name): + if escaped: + escaped = False + elif in_quotes and char == "\\": + escaped = True + elif char == '"': + if not in_quotes and (index == 0 or name[index - 1] not in " \t"): + raise ValueError("Git config quoted subsection names must begin after whitespace") + in_quotes = not in_quotes + elif char == "]" and not in_quotes: + raise ValueError("Git config section names must not contain an unquoted closing bracket") + if in_quotes: + raise ValueError("Git config section names must not contain an unterminated quote") @needs_values @set_dirty_and_flush_changes diff --git a/test/test_config.py b/test/test_config.py index c4e39db48..361a51fa9 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -194,6 +194,53 @@ def test_set_value_rejects_unsafe_section_and_option_names(self, rw_dir): self.assertEqual(git_config.get_value("user", "name"), "safe") self.assertFalse(git_config.has_section("core")) + @with_rw_directory + def test_writer_rejects_unquoted_section_terminators(self, rw_dir): + config_path = osp.join(rw_dir, "config") + bad_sections = ( + "user] [other", + 'user"] [other"', + 'submodule "docs"] [other', + 'submodule "docs] [other', + 'submodule "docs] [other\\', + ) + safe_section = 'submodule "docs]archive"' + + with GitConfigParser(config_path, read_only=False) as git_config: + git_config.add_section("user") + for bad_section in bad_sections: + with pytest.raises(ValueError, match="section name"): + git_config.add_section(bad_section) + with pytest.raises(ValueError, match="section name"): + git_config.set(bad_section, "name", "value") + with pytest.raises(ValueError, match="section name"): + git_config.set_value(bad_section, "name", "value") + with pytest.raises(ValueError, match="section name"): + git_config.add_value(bad_section, "name", "value") + with pytest.raises(ValueError, match="section name"): + git_config.rename_section("user", bad_section) + + git_config.set_value("user", "name", "safe") + git_config.set_value(safe_section, "name", "safe") + self.assertEqual(git_config.get_value(safe_section, "name"), "safe") + + # A closing bracket inside a quoted subsection name is data, not a section terminator. + with open(config_path, "rb") as config_file: + self.assertIn( + b'[submodule "docs]archive"]\n', + config_file.read(), + "a closing bracket within a quoted subsection name should be preserved", + ) + + # Reparse the file to verify that rejected names did not inject an [other] section. + with GitConfigParser(config_path, read_only=True) as git_config: + self.assertEqual( + git_config.get_value("user", "name"), + "safe", + "rejected section names corrupted the existing section", + ) + self.assertFalse(git_config.has_section("other"), "an unsafe section name injected an [other] section") + @with_rw_directory def test_set_and_add_value_reject_unsafe_value_characters(self, rw_dir): config_path = osp.join(rw_dir, "config") From faf3c09038b03bc2bdd8545ef34bbf6d7f1cd11f Mon Sep 17 00:00:00 2001 From: Byron Date: Mon, 20 Jul 2026 15:39:33 +0200 Subject: [PATCH 9/9] prepare for security fix --- VERSION | 2 +- doc/source/changes.rst | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 2c5a9fd4a..406837cb8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.1.52 +3.1.53 diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 2bfe6dd17..138826a3c 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,18 @@ Changelog ========= +3.1.53 +====== + +A security fix for +https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-3rp5-jjmw-4wv2 + +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.53 + 3.1.52 ======