From 24b6f95d4b90ab1483fb601c736ac6e7c059cd61 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 10 Sep 2026 04:59:45 +0200 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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()