From fbbecc6ab9a40dcab680b87b5eded51ffef0a5dc Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Mon, 5 Oct 2026 17:39:29 +0530 Subject: [PATCH 01/25] fix(commit): reject identity fields that alter commit headers `Commit._serialize` wrote `Actor.name` and `Actor.email` into the `author` and `committer` headers exactly as given. Each identity is a single line of the form `name date`, so a line feed or an angle bracket inside either field moves those boundaries. A line feed in the author name ends the `author` line and the remainder is read as further headers. `author` is written before `committer`, so a `committer` line supplied this way is the one Git and `Commit._deserialize` use, and an empty line ends the headers so the rest becomes the message. With the committer pinned by a service and only the display name taken from its user, the name Eve 1700000000 +0000 committer Release Manager 1700000000 +0000 Approved-by: Release Manager produced a commit that `git log` attributes to committer `Release Manager ` with the subject `Approved-by: Release Manager`, and `git fsck` reported nothing for it. Without a line feed, `<` in a name or `>` in an email still presents another email: the name `Release Manager x` with the email `eve@user.example` reads back as `release@corp.example` in both Git and GitPython. Git removes these three characters when it formats an identity (`strbuf_addstr_without_crud()` in `ident.c`), so `git commit-tree` given the same `GIT_AUTHOR_NAME` cannot write such a commit. Check the name and email of both identities at the start of `_serialize` and raise `ValueError` if one contains `<`, `>` or a line feed. `create_from_tree`, `IndexFile.commit` and `replace` all store the commit through this method, and the check runs before the object is stored and before `HEAD` or its reflog are updated. It raises instead of removing the characters, like the tree and index serializers that reject invalid entries, so a caller is not left with an identity it did not ask for. The `back-to-the-roots` branch (#2262) already refuses these characters before calling `git commit-tree`; this covers the native serializer until then. Identities without these characters are written as before. `_deserialize` can return such a field only from a commit that `git fsck` already reports as `badName` or `badEmail`, for example `A> B ` or `A <>`. Serializing a commit like that again, for example through `replace()`, now raises too. None of the 5466 commits in this repository is affected and `test_serialization` still reproduces every object ID it checks. NUL and carriage return are left alone: neither moves a field boundary, and Git keeps a carriage return inside a name. `test_identity_cannot_alter_headers` covers both fields of both identities through `create_from_tree` and `replace`. It fails on the previous code and passes here. The full suite has no new failures on macOS with Python 3.11 (the six `nul\x00name` cases of `test_submodule_rejects_unsafe_checkout_before_mutation` fail there with and without this change), and `ruff`, `mypy`, `basedpyright --warnings` and the documentation build are clean. --- git/objects/commit.py | 14 ++++++++++++++ test/test_commit.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/git/objects/commit.py b/git/objects/commit.py index 307b8c59b..690a296a5 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -662,6 +662,10 @@ def create_from_tree( :return: :class:`Commit` object representing the new commit. + :raise ValueError: + If the name or email of the author or committer contains ``<``, ``>`` or a + line feed, as these would change the identity headers of the commit. + :note: Additional information about the committer and author are taken from the environment or from the git configuration. See :manpage:`git-commit-tree(1)` @@ -786,6 +790,16 @@ def create_from_tree( # { Serializable Implementation def _serialize(self, stream: BytesIO) -> "Commit": + # An identity is written as "name date" on a single header line, so a + # line feed or an angle bracket inside a name or email moves those boundaries: + # it can add header lines, end the headers early, or present another email. + # Git drops these three characters when it writes an identity; refuse them + # here before anything is written. + for actor in (self.author, self.committer): + for value in (actor.name, actor.email): + if value and any(char in value for char in "<>\n"): + raise ValueError("Commit identity %r must not contain '<', '>' or a line feed" % value) + write = stream.write write(("tree %s\n" % self.tree).encode("ascii")) for p in self.parents: diff --git a/test/test_commit.py b/test/test_commit.py index b5baf0e33..08ba4a634 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -424,6 +424,49 @@ def test_invalid_commit(self): self.assertEqual(cmt.author.name, "E.Azer Ko�o�o�oculu", cmt.author.name) self.assertEqual(cmt.author.email, "azer@kodfabrik.com", cmt.author.email) + @with_rw_directory + def test_identity_cannot_alter_headers(self, rw_dir): + """A name or email must not add header lines or present another identity.""" + rw_repo = Repo.init(osp.join(rw_dir, "test_identity_headers")) + path = osp.join(str(rw_repo.working_tree_dir), "hello.txt") + touch(path) + rw_repo.index.add([path]) + tree = rw_repo.index.write_tree() + service = Actor("Service", "service@example.com") + forged = "committer Forged 0 +0000" + + for name, email in ( + # A line feed ends the header line, so the remainder would become headers + # of its own, which Git reads before the committer written after them. + ("User 0 +0000\n" + forged, "user@example.com"), + ("User", "user@example.com> 0 +0000\n" + forged), + # Angle brackets delimit the email, so these would present another one. + ("Forged ", "user@example.com"), + ("User", "forged@example.com> ", "user@example.com"), + ("User", " Date: Thu, 8 Oct 2026 16:22:52 +0530 Subject: [PATCH 02/25] fix(util): reject symbolic links that alias `.gitmodules` `_validate_repo_path` mirrors Git's `verify_path` for tree and index entry paths, but it only ever saw the path. Git's check there is mode dependent: an entry that makes `.gitmodules` a symbolic link is refused, because the submodule configuration would then be read through the link, from outside the repository. That is why `git update-index --add --cacheinfo 120000,,.gitmodules` fails with `Invalid path`, `git read-tree` fails with `invalid path`, and `git fsck --strict` reports `gitmodulesSymlink`. GitPython accepted such an entry in both directions. `IndexFile.add` with a `BaseIndexEntry` of mode `120000` and path `.gitmodules` was stored, `write_tree` serialized the tree, and `IndexFile.commit` wrote a commit Git refuses to read back and a server with `transfer.fsckObjects` set rejects. Coming the other way, `read_cache` and `tree_entries_from_data` accepted the same entry out of an untrusted repository's index or tree. `_validate_repo_path` now takes the entry mode and, for a symbolic link, rejects every spelling Git recognizes: `.gitmodules` with trailing spaces or periods, the HFS form with ignorable code points removed, and the NTFS short names `gitmod~1` through `gitmod~4` and `gi7eba~1` through `gi7eba~9`. The mode is passed at the boundaries that have one: `write_cache`, `read_cache`, `write_tree_from_cache`, `_tree_entry_to_baseindexentry`, `IndexFile._preprocess_add_items`, `IndexFile.add`, `tree_to_stream`, `tree_entries_from_data` and `TreeModifier.add`. Paths reached without a mode, such as the directories walked by `IndexFile._iter_expand_paths`, keep their previous behavior, and a regular file named `.gitmodules` stays valid. Checked against `git update-index --add --cacheinfo` on git 2.52.0 for modes `100644`, `120000`, `160000` and `40000` over the alias corpus: no path is left that Git rejects and GitPython accepts. Like the existing `.git` rule the name is tested per component, so a link below a directory spelled like one of those aliases is refused as well, which Git happens to allow. Adds regression tests in `test/test_index.py` and `test/test_tree.py`; `mypy`, `basedpyright --warnings` and `ruff` are clean. --- git/index/base.py | 4 ++-- git/index/fun.py | 8 ++++---- git/objects/fun.py | 8 ++++---- git/objects/tree.py | 2 +- git/util.py | 20 +++++++++++++++++++- test/test_index.py | 31 +++++++++++++++++++++++++++++-- test/test_tree.py | 22 ++++++++++++++++++++++ 7 files changed, 81 insertions(+), 14 deletions(-) diff --git a/git/index/base.py b/git/index/base.py index 4ecd0c5b1..9a6a1a562 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -729,7 +729,7 @@ def _preprocess_add_items( raise TypeError("Invalid Type: %r" % item) # END for each item for entry in entries: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) return paths, entries def _store_path(self, filepath: PathLike, fprogress: Callable) -> BaseIndexEntry: @@ -1026,7 +1026,7 @@ def handle_null_entries(self: "IndexFile") -> None: # FINALIZE # Add the new entries to this instance. for entry in entries_added: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) for entry in entries_added: self.entries[(entry.path, 0)] = IndexEntry.from_base(entry) diff --git a/git/index/fun.py b/git/index/fun.py index 929038f1f..2491f55ae 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -279,7 +279,7 @@ def write_cache( # Body for entry in entries: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) beginoffset = tell() write(entry.ctime_bytes) # ctime write(entry.mtime_bytes) # mtime @@ -394,7 +394,7 @@ def read_cache( if terminator != b"\0": raise ValueError("Unterminated index entry path") path = path_bytes.decode(defenc) - _validate_repo_path(path) + _validate_repo_path(path, mode) real_size = (tell() - beginoffset + 7) & ~7 padding_size = beginoffset + real_size - tell() @@ -462,7 +462,7 @@ def write_tree_from_cache( """ if si == 0: for entry in entries[sl]: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) tree_items: List["TreeCacheTup"] = [] ci = sl.start @@ -510,7 +510,7 @@ def write_tree_from_cache( def _tree_entry_to_baseindexentry(tree_entry: "TreeCacheTup", stage: int) -> BaseIndexEntry: - _validate_repo_path(tree_entry[2]) + _validate_repo_path(tree_entry[2], tree_entry[1]) return BaseIndexEntry((tree_entry[1], tree_entry[0], stage << CE_STAGESHIFT, tree_entry[2])) diff --git a/git/objects/fun.py b/git/objects/fun.py index ff1bc8483..41ddecafd 100644 --- a/git/objects/fun.py +++ b/git/objects/fun.py @@ -39,11 +39,11 @@ # --------------------------------------------------- -def _validate_tree_entry_name(name: str) -> None: +def _validate_tree_entry_name(name: str, mode: Union[int, None] = None) -> None: if "/" in name: raise ValueError("Tree entry names must not contain '/' characters") # A tree name is a component, not a rooted path; a colon cannot select a drive. - _validate_repo_path("tree/" + name) + _validate_repo_path("tree/" + name, mode) def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer"], Union[int, None]]) -> None: @@ -82,7 +82,7 @@ def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer name_bytes = name.encode(defenc) else: name_bytes = name # type: ignore[unreachable] # check runtime types - is always str? - _validate_tree_entry_name(safe_decode(name_bytes)) + _validate_tree_entry_name(safe_decode(name_bytes), mode) write(b"".join((mode_str, b" ", name_bytes, b"\0", binsha))) # END for each item @@ -112,7 +112,7 @@ def tree_entries_from_data(data: bytes) -> List[EntryTup]: if name_end < 0 or name_end + 21 > len(data): raise ValueError("Truncated tree entry") name = safe_decode(bytes(data[mode_end + 1 : name_end])) - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) offset = name_end + 21 out.append((bytes(data[name_end + 1 : offset]), mode, name)) return out diff --git a/git/objects/tree.py b/git/objects/tree.py index d2a5859df..96e263530 100644 --- a/git/objects/tree.py +++ b/git/objects/tree.py @@ -110,7 +110,7 @@ def add(self, sha: bytes, mode: int, name: str, force: bool = False) -> "TreeMod :return: self """ - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) if (mode >> 12) not in Tree._map_id_to_type: raise ValueError("Invalid object type according to mode %o" % mode) diff --git a/git/util.py b/git/util.py index 75677b7d3..369376713 100644 --- a/git/util.py +++ b/git/util.py @@ -385,12 +385,25 @@ def _to_relative_path(root: PathLike, path: PathLike) -> str: "", "", "\u200c\u200d\u200e\u200f\u202a\u202b\u202c\u202d\u202e\u206a\u206b\u206c\u206d\u206e\u206f\ufeff" ) +# Windows derives an 8.3 short name for ".gitmodules" from its first six +# characters followed by "~1" through "~4", and from a hashed stem once those +# are taken. Git recognizes both spellings (is_ntfs_dotgitmodules in path.c). +_NTFS_DOTGITMODULES_SHORT_NAMES = frozenset( + ["gitmod~%d" % index for index in range(1, 5)] + ["gi7eba~%d" % index for index in range(1, 10)] +) + -def _validate_repo_path(path: PathLike) -> None: +def _validate_repo_path(path: PathLike, mode: Union[int, None] = None) -> None: """Reject unsafe tree/index paths without normalizing away their components. Protect Git metadata aliases on NTFS and HFS even when writing on another platform. Other POSIX filename characters, including newlines, remain valid. + + :param mode: + Mode of the index or tree entry the path belongs to, where one is known. + Git refuses a symbolic link that aliases ``.gitmodules``, since the + submodule configuration would then be read through the link, so that + name is only rejected once the mode says the entry is a link. """ name = os.fspath(path) if not name or "\0" in name or ntpath.splitdrive(name)[0] or name.startswith("/"): @@ -406,6 +419,11 @@ def _validate_repo_path(path: PathLike) -> None: hfs_name = part.translate(_HFS_IGNORABLES).lower() if ntfs_name in (".git", "git~1") or hfs_name == ".git": raise ValueError("Repository path aliases Git metadata: %r" % name) + aliases_gitmodules = ( + ntfs_name == ".gitmodules" or hfs_name == ".gitmodules" or ntfs_name in _NTFS_DOTGITMODULES_SHORT_NAMES + ) + if aliases_gitmodules and mode is not None and stat.S_ISLNK(mode): + raise ValueError("Symbolic link aliases the submodule configuration: %r" % name) def assure_directory_exists(path: PathLike, is_file: bool = False) -> bool: diff --git a/test/test_index.py b/test/test_index.py index 30cb54565..a46a3d424 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -189,10 +189,10 @@ def _make_hook(git_dir, name, content, make_exec=True): return hp -def _raw_index(path): +def _raw_index(path, mode=0o100644): """Build an index without using the writer under test.""" name = path.encode("utf-8") - entry = struct.pack(">10L20sH", 0, 0, 0, 0, 0, 0, 0o100644, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name + entry = struct.pack(">10L20sH", 0, 0, 0, 0, 0, 0, mode, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name entry += b"\0" * (8 - len(entry) % 8) data = b"DIRC" + struct.pack(">LL", 2, 1) + entry return data + sha1(data).digest() @@ -340,6 +340,33 @@ def test_index_reader_and_writer_reject_unsafe_paths(self, path): with pytest.raises(ValueError): write_cache([entry], BytesIO()) + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules.", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~4", + "gi7eba~1", + "gi7eba~9", + "sub/.gitmodules", + ) + def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path): + """An entry that turns .gitmodules into a symbolic link is rejected, while the + same name stays valid for a regular file. The spellings are those + `git update-index --add --cacheinfo 120000,,` refuses on git 2.52.0.""" + with pytest.raises(ValueError): + read_cache(BytesIO(_raw_index(path, mode=0o120000))) + with pytest.raises(ValueError): + write_cache([IndexEntry((0o120000, b"a" * 20, 0, path))], BytesIO()) + + assert next(iter(read_cache(BytesIO(_raw_index(path)))[1])) == (path, 0) + stream = BytesIO() + write_cache([IndexEntry((0o100644, b"a" * 20, 0, path))], stream) + stream.seek(0) + assert next(iter(read_cache(stream)[1])) == (path, 0) + def test_valid_unusual_index_names_round_trip(self): names = ["a b", "a\nb", "a\tb", "name:value", "dir/.gitignore", "café"] if os.name != "nt": diff --git a/test/test_tree.py b/test/test_tree.py index 803f1f381..d12dbcf71 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -138,6 +138,28 @@ def test_tree_names_are_checked_at_construction_and_serialization(self, name): with pytest.raises(ValueError): tree_to_stream([(b"a" * 20, 0o100644, name)], BytesIO().write) + @ddt.data(".gitmodules", ".GITMODULES", ".gitmodules ", ".gi\u200ctmodules", "gitmod~1", "gi7eba~1") + def test_gitmodules_symlink_entries_are_rejected(self, name): + """A symbolic link named like the submodule configuration would make Git read + it from outside the repository, so such an entry is refused in both + directions. A regular file with the same name is the normal case.""" + symlink_mode = 0o120000 + cache = [] + with pytest.raises(ValueError): + TreeModifier(cache).add(b"a" * 20, symlink_mode, name) + assert not cache + with pytest.raises(ValueError): + tree_to_stream([(b"a" * 20, symlink_mode, name)], BytesIO().write) + raw = b"120000 " + name.encode() + b"\0" + b"a" * 20 + with pytest.raises(ValueError): + tree_entries_from_data(raw) + + TreeModifier(cache).add(b"a" * 20, 0o100644, name) + assert cache == [(b"a" * 20, 0o100644, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + assert tree_entries_from_data(data.getvalue()) == cache + def test_traverse(self): root = self.rorepo.tree("0.1.6") num_recursive = 0 From 9f56080a08a5bcc88ee8670721152f8ab35f4d34 Mon Sep 17 00:00:00 2001 From: Byron Date: Fri, 9 Oct 2026 05:22:10 +0200 Subject: [PATCH 03/25] review - speedup additional .gitmodules check --- git/index/base.py | 4 +- git/util.py | 73 ++++++++++++++------------- test/test_index.py | 123 +++++++++++++++++++++++++++++++++++++++------ test/test_tree.py | 87 +++++++++++++++++++++++++++++++- 4 files changed, 236 insertions(+), 51 deletions(-) diff --git a/git/index/base.py b/git/index/base.py index 9a6a1a562..3d32d7d81 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -728,8 +728,10 @@ def _preprocess_add_items( else: raise TypeError("Invalid Type: %r" % item) # END for each item + # Source paths must be safe to read, but their recorded names may be rewritten. + # Apply mode-dependent restrictions to the final entries in add(). for entry in entries: - _validate_repo_path(entry.path, entry.mode) + _validate_repo_path(entry.path) return paths, entries def _store_path(self, filepath: PathLike, fprogress: Callable) -> BaseIndexEntry: diff --git a/git/util.py b/git/util.py index 369376713..61ddab861 100644 --- a/git/util.py +++ b/git/util.py @@ -29,63 +29,62 @@ if sys.platform == "win32": __all__.append("to_native_path_windows") -from abc import abstractmethod import contextlib -from functools import wraps import getpass import logging import ntpath import os import os.path as osp -from pathlib import Path import platform import re import shutil import stat import subprocess import time -from urllib.parse import urlsplit, urlunsplit import warnings - -# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of -# these be used indirectly through other GitPython modules, which avoids having to write -# gitdb all the time in their imports. They are not in __all__, at least currently, -# because they could be removed or changed at any time, and so should not be considered -# conceptually public to code outside GitPython. Linters of course do not like it. -from gitdb.util import ( - LazyMixin, # noqa: F401 - LockedFD, # noqa: F401 - bin_to_hex, # noqa: F401 - file_contents_ro, # noqa: F401 - file_contents_ro_filepath, # noqa: F401 - hex_to_bin, # noqa: F401 - make_sha, - to_bin_sha, # noqa: F401 - to_hex_sha, # noqa: F401 -) +from abc import abstractmethod +from functools import wraps +from pathlib import Path # typing --------------------------------------------------------- - from typing import ( + IO, + TYPE_CHECKING, Any, AnyStr, Callable, Dict, Generator, - IO, Iterator, List, Optional, Pattern, Sequence, Tuple, - TYPE_CHECKING, Type, TypeVar, Union, cast, overload, ) +from urllib.parse import urlsplit, urlunsplit + +# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of +# these be used indirectly through other GitPython modules, which avoids having to write +# gitdb all the time in their imports. They are not in __all__, at least currently, +# because they could be removed or changed at any time, and so should not be considered +# conceptually public to code outside GitPython. Linters of course do not like it. +from gitdb.util import ( + LazyMixin, # noqa: F401 + LockedFD, # noqa: F401 + bin_to_hex, # noqa: F401 + file_contents_ro, # noqa: F401 + file_contents_ro_filepath, # noqa: F401 + hex_to_bin, # noqa: F401 + make_sha, + to_bin_sha, # noqa: F401 + to_hex_sha, # noqa: F401 +) if TYPE_CHECKING: from git.cmd import Git @@ -94,9 +93,9 @@ from git.repo.base import Repo from git.types import ( + HSH_TD, Files_TD, Has_id_attribute, - HSH_TD, Literal, PathLike, Protocol, @@ -385,11 +384,13 @@ def _to_relative_path(root: PathLike, path: PathLike) -> str: "", "", "\u200c\u200d\u200e\u200f\u202a\u202b\u202c\u202d\u202e\u206a\u206b\u206c\u206d\u206e\u206f\ufeff" ) -# Windows derives an 8.3 short name for ".gitmodules" from its first six -# characters followed by "~1" through "~4", and from a hashed stem once those -# are taken. Git recognizes both spellings (is_ntfs_dotgitmodules in path.c). -_NTFS_DOTGITMODULES_SHORT_NAMES = frozenset( - ["gitmod~%d" % index for index in range(1, 5)] + ["gi7eba~%d" % index for index in range(1, 10)] +# Match Git's is_ntfs_dotgitmodules in path.c on a lowercased, trimmed name. +# Besides gitmod~1..4, fallback aliases have exactly eight ASCII characters: +# a shrinking prefix of "gi7eba", "~", and digits with no leading zero. +# Explicit digit counts avoid accepting shorter/longer names or Unicode digits. +_NTFS_DOTGITMODULES_SHORT_NAME = re.compile( + r"(?:gitmod~[1-4]|gi7eba~[1-9]|gi7eb~[1-9][0-9]|gi7e~[1-9][0-9]{2}|" + r"gi7~[1-9][0-9]{3}|gi~[1-9][0-9]{4}|g~[1-9][0-9]{5}|~[1-9][0-9]{6})" ) @@ -419,11 +420,13 @@ def _validate_repo_path(path: PathLike, mode: Union[int, None] = None) -> None: hfs_name = part.translate(_HFS_IGNORABLES).lower() if ntfs_name in (".git", "git~1") or hfs_name == ".git": raise ValueError("Repository path aliases Git metadata: %r" % name) - aliases_gitmodules = ( - ntfs_name == ".gitmodules" or hfs_name == ".gitmodules" or ntfs_name in _NTFS_DOTGITMODULES_SHORT_NAMES - ) - if aliases_gitmodules and mode is not None and stat.S_ISLNK(mode): - raise ValueError("Symbolic link aliases the submodule configuration: %r" % name) + if mode is not None and stat.S_ISLNK(mode): + if ( + ntfs_name == ".gitmodules" + or hfs_name == ".gitmodules" + or _NTFS_DOTGITMODULES_SHORT_NAME.fullmatch(ntfs_name) is not None + ): + raise ValueError("Symbolic link aliases the submodule configuration: %r" % name) def assure_directory_exists(path: PathLike, is_file: bool = False) -> bool: diff --git a/test/test_index.py b/test/test_index.py index a46a3d424..150b39466 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -350,6 +350,21 @@ def test_index_reader_and_writer_reject_unsafe_paths(self, path): "gitmod~4", "gi7eba~1", "gi7eba~9", + "GI7EB~10", + "GI7EB~99", + "GI7E~100", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "GI7EB~10. ", + "GI7E~100:$DATA", + "sub/~1000000", "sub/.gitmodules", ) def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path): @@ -367,6 +382,26 @@ def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path): stream.seek(0) assert next(iter(read_cache(stream)[1])) == (path, 0) + @ddt.data("GI7EB~10", "GI7E~100", "GI7~1000", "GI~10000", "G~100000", "~1000000", "~9999999") + @with_rw_directory + def test_index_add_and_write_tree_reject_gitmodules_fallback_symlinks(self, rw_dir, path): + with Repo.init(rw_dir) as repo: + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + index = repo.index + for item in (Blob(repo, binsha, 0o120000, path), BaseIndexEntry((0o120000, binsha, 0, path))): + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], write=False) + assert not index.entries + + index.entries[(path, 0)] = IndexEntry((0o120000, binsha, 0, path)) + with pytest.raises(ValueError, match="submodule configuration"): + index.write_tree() + assert not Path(index.path).exists() + + index.add([BaseIndexEntry((0o100644, binsha, 0, path))]) + assert repo.index.entries[(path, 0)].mode == 0o100644 + assert index.write_tree()[path].mode == 0o100644 + def test_valid_unusual_index_names_round_trip(self): names = ["a b", "a\nb", "a\tb", "name:value", "dir/.gitignore", "café"] if os.name != "nt": @@ -408,20 +443,26 @@ def _cmp_tree_index(self, tree, index): @with_rw_repo("0.1.6") def test_index_lock_handling(self, rw_repo): - def add_bad_blob(): - rw_repo.index.add([Blob(rw_repo, b"f" * 20, "bad-permissions", "foo")]) - - try: - ## First, fail on purpose adding into index. - add_bad_blob() - except Exception as ex: - assert "required argument is not an integer" in str(ex) - - ## The second time should not fail due to stray lock file. - try: - add_bad_blob() - except Exception as ex: - assert "index.lock' could not be obtained" not in str(ex) + index = rw_repo.index + index_path = Path(index.path) + lock_path = Path(str(index_path) + ".lock") + before = index_path.read_bytes() + + def fail_serialize(stream, ignore_extension_data): + assert lock_path.exists() + stream.write(b"partial index") + raise OSError("simulated index write failure") + + with mock.patch.object(IndexFile, "_serialize", side_effect=fail_serialize): + for _ in range(2): + with pytest.raises(OSError, match="simulated index write failure"): + index.write() + assert not lock_path.exists() + assert index_path.read_bytes() == before + + index.add([Blob(rw_repo, b"f" * 20, 0o100644, "foo")]) + assert not lock_path.exists() + assert rw_repo.index.entries[("foo", 0)].mode == 0o100644 @with_rw_repo("0.1.6") def test_read_tree_methods_reject_index_output(self, rw_repo): @@ -1248,6 +1289,60 @@ def test_staging_rejects_unsafe_object_paths_even_without_writing(self, rw_dir, index.add([item], write=False, **kwargs) assert not index.entries + @ddt.data(*product(("path", "blob", "entry", "stored-blob", "stored-entry"), (False, True), (False, True))) + @ddt.unpack + @with_rw_directory + def test_staging_gitmodules_symlink_check_uses_rewritten_path(self, rw_dir, kind, unsafe_destination, write): + with Repo.init(rw_dir) as repo: + source, destination = ("safe-link", ".gitmodules") if unsafe_destination else (".gitmodules", "safe-link") + binsha = Blob.NULL_BIN_SHA + if kind.startswith("stored-"): + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + else: + try: + (Path(rw_dir) / source).symlink_to("target") + except OSError: + pytest.skip("Symlinks unavailable") + if kind == "path": + item = source + elif kind.endswith("blob"): + item = Blob(repo, binsha, 0o120000, source) + else: + item = BaseIndexEntry((0o120000, binsha, 0, source)) + index = repo.index + rewriter = mock.Mock(return_value=destination) + if unsafe_destination: + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], path_rewriter=rewriter, write=write) + assert not index.entries + assert not Path(index.path).exists() + else: + added = index.add([item], path_rewriter=rewriter, write=write) + assert [(entry.path, entry.mode) for entry in added] == [(destination, 0o120000)] + assert set(index.entries) == {(destination, 0)} + assert index.write_tree()[destination].mode == 0o120000 + if write: + assert repo.index.entries[(destination, 0)].mode == 0o120000 + rewriter.assert_called_once() + assert rewriter.call_args[0][0].path == source + + @ddt.data(*product(("blob", "entry"), ("../outside", ".git/config"))) + @ddt.unpack + @with_rw_directory + def test_staging_rewriter_cannot_sanitize_unsafe_source_paths(self, rw_dir, kind, path): + with Repo.init(rw_dir) as repo: + item = ( + Blob(repo, b"a" * 20, 0o120000, path) + if kind == "blob" + else BaseIndexEntry((0o120000, b"a" * 20, 0, path)) + ) + index = repo.index + rewriter = mock.Mock(return_value="safe-link") + with pytest.raises(ValueError): + index.add([item], path_rewriter=rewriter, write=False) + rewriter.assert_not_called() + assert not index.entries + @with_rw_directory def test_staging_root_preserves_symlinks_and_skips_git_metadata(self, rw_dir): tmp_path = Path(rw_dir) diff --git a/test/test_tree.py b/test/test_tree.py index d12dbcf71..3a85405bd 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -138,7 +138,38 @@ def test_tree_names_are_checked_at_construction_and_serialization(self, name): with pytest.raises(ValueError): tree_to_stream([(b"a" * 20, 0o100644, name)], BytesIO().write) - @ddt.data(".gitmodules", ".GITMODULES", ".gitmodules ", ".gi\u200ctmodules", "gitmod~1", "gi7eba~1") + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~2", + "gitmod~3", + "GITMOD~4", + "gi7eba~1", + "GI7EBA~9", + "GI7EB~10", + "GI7EB~11", + "GI7EB~99", + "GI7E~100", + "GI7E~101", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "Gi7Eb~42", + "gi7e~120", + "GITMOD~4 . ", + "GI7EB~10. ", + "GI7E~100:$DATA", + "~1000000 . :$DATA", + ) def test_gitmodules_symlink_entries_are_rejected(self, name): """A symbolic link named like the submodule configuration would make Git read it from outside the repository, so such an entry is refused in both @@ -160,6 +191,60 @@ def test_gitmodules_symlink_entries_are_rejected(self, name): tree_to_stream(cache, data.write) assert tree_entries_from_data(data.getvalue()) == cache + @ddt.data( + "gitmod~0", + "gitmod~5", + "gitmod~10", + "GI7EBA~", + "GI7EBA~0", + "GI7EBA~~1", + "GI7EBA~X", + "GI7EBA~10", + "Gx7EBA~1", + "GI7EBX~1", + "GI7EB~1", + "GI7EB~01", + "GI7EB~1X", + "GI7EB~100", + "GI7E~10", + "GI7E~010", + "GI7E~1000", + "GI7~100", + "GI7~0100", + "GI7~10000", + "GI~1000", + "GI~01000", + "GI~100000", + "G~10000", + "G~010000", + "G~1000000", + "~100000", + "~0100000", + "~10000000", + "GI7EBA~\u0661", + "GI7EB~1\uff10", + "GI7EB~10x", + "GI7EB~10.x", + " GI7EB~10", + "GI7EB~10\n", + "GI7EB~10\t", + "GI7EB~10x:$DATA", + ".gitmodules x", + ".gitmodules .x", + ".gitmodules,:$DATA", + ) + def test_gitmodules_short_name_near_misses_round_trip(self, name): + """Only exact aliases are forbidden, and only for symbolic links.""" + for mode in (0o100644, 0o120000): + cache = [] + TreeModifier(cache).add(b"a" * 20, mode, name) + assert cache == [(b"a" * 20, mode, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + raw = ("%o " % mode).encode() + name.encode() + b"\0" + b"a" * 20 + assert data.getvalue() == raw + assert tree_entries_from_data(raw) == cache + def test_traverse(self): root = self.rorepo.tree("0.1.6") num_recursive = 0 From 6c99e0439971749d75375beca74acb15b2ad8e04 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Sat, 10 Oct 2026 01:38:04 +0530 Subject: [PATCH 04/25] fix: send one object name or path per git stdin record `Git._prepare_ref` terminates an object name with a line feed and writes it to the persistent `git cat-file --batch-check` or `--batch` process, and `__get_object_header` reads exactly one response line back. A name that itself contains a line feed is therefore two requests against one response read, and the stream is left one response behind for the lifetime of the `Git` instance, so every later lookup is answered with the header of an object it did not ask for. `Repo.is_valid_object` hands its argument straight to `partial_to_complete_sha_hex`, so a single call with an untrusted revision string is enough to get there. After `repo.is_valid_object("HEAD\nHEAD")`, `repo.commit()` returns a different commit than the one named and `Repo.odb.info` reports another object's type and size, with nothing to indicate it. On the `--batch` stream the size from the mismatched header also misaligns the content reads, so object data is read from the wrong offsets and a tree read right afterwards fails with `Invalid tree entry mode`. Git has no way to express an object name containing a line feed on that stdin, so `_prepare_ref` now refuses one and keeps the single trailing line feed that is the request terminator. `Repo.is_valid_object` reports such a name as invalid, which it is, and `short_to_long` keeps turning it into `BadName`. `IndexFile._write_path_to_stdin` writes paths to `git checkout-index --stdin` the same way, and unlike an object name a path can legitimately contain a line feed. With `dir/x\nkeep.txt` in the index, `index.checkout(paths=["dir"], force=True)` had Git read two paths, so `keep.txt` was checked out over the local copy while the requested file was never written. Git also unquotes a line-feed separated path that begins with a double quote, which made a file named `"q"` impossible to check out. That call site now runs with `-z` and NUL-terminated paths, which Git does not unquote. Adds regression tests in `test/test_git.py` and `test/test_index.py`, both of which fail before this change. The rest of the suite is unaffected on Python 3.11 on macOS, apart from eight failures that predate the change in `test_commit.py` and `test_submodule.py`; `ruff`, `mypy`, `basedpyright --warnings` and the docs build are clean. --- git/cmd.py | 12 +++++++++--- git/index/base.py | 11 ++++++----- test/test_git.py | 13 +++++++++++++ test/test_index.py | 21 +++++++++++++++++++++ 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 3ddb9ae7f..1d1ca909a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1940,9 +1940,15 @@ def _prepare_ref(self, ref: object) -> bytes: else: refstr = ref - if not refstr.endswith("\n"): - refstr += "\n" - return refstr.encode(defenc) + # A line feed terminates a request, so one object name must be one line. An + # embedded one would queue a second request on the persistent command while + # only one response line is read back, leaving every later call one response + # behind, answered with the header of an object it did not ask for. + if refstr.endswith("\n"): + refstr = refstr[:-1] + if "\n" in refstr: + raise ValueError("Object name %r contains a line feed" % refstr) + return (refstr + "\n").encode(defenc) def _get_persistent_cmd(self, attr_name: str, cmd_name: str, *args: Any, **kwargs: Any) -> "Git.AutoInterrupt": cur_val = getattr(self, attr_name) diff --git a/git/index/base.py b/git/index/base.py index 3d32d7d81..17a722c21 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -525,17 +525,17 @@ def _write_path_to_stdin( the piped-in files are processed anyway and just in time. :note: - Newlines are essential here, git's behaviour is somewhat inconsistent on - this depending on the version, hence we try our best to deal with newlines - carefully. Usually the last newline will not be sent, instead we will close - stdin to break the pipe. + Paths are NUL-terminated, so the command has to run with ``-z``. A path can + contain a line feed, and with line-feed separation git would read such a + path as two paths and act on files that were never passed. git also unquotes + a line-feed separated path that begins with a double quote. """ fprogress(filepath, False, item) rval: Union[None, str] = None if proc.stdin is not None: try: - proc.stdin.write(("%s\n" % filepath).encode(defenc)) + proc.stdin.write(("%s\0" % filepath).encode(defenc)) except OSError as e: # Pipe broke, usually because some error happened. raise fmakeexc() from e @@ -1452,6 +1452,7 @@ def handle_stderr(proc: "Popen[bytes]", iter_checked_out_files: Iterable[PathLik # initialization. self.entries # noqa: B018 + args.append("-z") args.append("--stdin") kwargs["as_process"] = True kwargs["istream"] = subprocess.PIPE diff --git a/test/test_git.py b/test/test_git.py index 26ff4e44e..5224842ee 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -593,6 +593,19 @@ def test_persistent_cat_file_command(self): self.assertEqual(typename, typename_two) self.assertEqual(size, size_two) + def test_object_header_rejects_an_embedded_line_feed(self): + hexsha = "b2339455342180c7cc1e9bba3e9f181f7baa5167" + git = Git(self.rorepo.working_dir) + header = git.get_object_header(hexsha) + + # A single trailing line feed is the request terminator, not a second request. + self.assertEqual(git.get_object_header(hexsha + "\n"), header) + + self.assertRaises(ValueError, git.get_object_header, "HEAD\nHEAD") + + # The persistent command is still in step, so this is not HEAD's header. + self.assertEqual(git.get_object_header(hexsha), header) + def test_version_info(self): """The version_info attribute is a tuple of up to four ints.""" v = self.git.version_info diff --git a/test/test_index.py b/test/test_index.py index 150b39466..23e59cc04 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -1878,6 +1878,27 @@ def test_checkout_pathlike(self, tmp_path, path_type, absolute, directory, conta for name, data in files.items(): assert (tmp_path / name).read_bytes() == data + @pytest.mark.skipif(os.name == "nt", reason="Line feeds and quotes are not valid Windows filenames") + def test_checkout_sends_each_path_as_one_record(self, tmp_path): + with Repo.init(tmp_path) as repo: + nested = tmp_path / "nested" + nested.mkdir() + (nested / "first\noutside").write_bytes(b"nested") + (tmp_path / "outside").write_bytes(b"committed") + (tmp_path / '"quoted"').write_bytes(b"quoted") + repo.index.add(["nested", "outside", '"quoted"']) + + (nested / "first\noutside").unlink() + (tmp_path / '"quoted"').unlink() + (tmp_path / "outside").write_bytes(b"local") + + checked_out = {"nested/first\noutside", '"quoted"'} + assert set(repo.index.checkout(["nested", '"quoted"'], force=True)) == checked_out + assert (nested / "first\noutside").read_bytes() == b"nested" + assert (tmp_path / '"quoted"').read_bytes() == b"quoted" + # Neither "nested/first" nor "outside" was requested. + assert (tmp_path / "outside").read_bytes() == b"local" + class TestIndexUtils: @pytest.mark.parametrize("file_path_type", [str, Path]) From 3a35778b6dcfc355d4b9a0c9edaf6b532cc6cc2a Mon Sep 17 00:00:00 2001 From: Byron Date: Sat, 10 Oct 2026 04:26:55 +0200 Subject: [PATCH 05/25] review - also update agents.md for clearer separation of speech, as an experiment really. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- AGENTS.md | 3 ++- CONTRIBUTING.md | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 6b2963cf4..cdffa4764 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,8 @@ Before starting work, read and follow [CONTRIBUTING.md](CONTRIBUTING.md), including the [Prevent agent impersonation](CONTRIBUTING.md#prevent-agent-impersonation) -section governing identification when communicating through a person's account. +section governing agent identification and separation of unaltered user statements +when communicating through a person's account. # Commit messages diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 76f276323..1a64a701d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,6 +27,13 @@ example in issue or PR descriptions and comments. AI assistance that does not re the person as the speaker, such as proofreading or wording polish, does not require identification. +Even when identifying themselves, agents must not assert on a person's behalf that +that person performed an action, such as reviewing or approving a PR. The person +must make any such statement themselves. If it is included alongside agent-authored +content, it must be supplied by the person and preserved verbatim in a clearly +labeled, separate user-authored section. Agents must not draft, paraphrase, or embed +such statements in their own narration. + 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. From 3e9611b9689597d86814d087d1b9b37be92e0451 Mon Sep 17 00:00:00 2001 From: v1shay Date: Sat, 10 Oct 2026 23:38:46 -0700 Subject: [PATCH 06/25] fix(stats): preserve whitespace in filenames Commit.stats strips leading and trailing whitespace from filenames after splitting the diff output into records. This merges distinct paths such as `name`, ` name`, and `name `, overwrites their per-file statistics, and turns an all-space filename into an empty key. Keep the filename field intact. Add an integration regression covering root commits, later modifications, empty commits, and deletions. Store blobs directly to avoid relying on trailing-space worktree filenames. The regression fails before this change and passes afterward. On Linux with Python 3.12, the full suite passes: 1690 passed, 89 skipped, 1 xfailed, and 64 subtests passed. Pre-commit, mypy, and basedpyright also pass. Prepared by OpenAI Codex (dot); human review is pending. --- git/util.py | 2 +- test/test_commit.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index 61ddab861..7fd061c7c 100644 --- a/git/util.py +++ b/git/util.py @@ -1107,7 +1107,7 @@ def _list_from_string(cls, repo: "Repo", text: str) -> "Stats": "lines": insertions + deletions, "change_type": change_type, } - hsh["files"][filename.strip()] = files_dict + hsh["files"][filename] = files_dict return Stats(hsh["total"], hsh["files"]) diff --git a/test/test_commit.py b/test/test_commit.py index 08ba4a634..ac155e0df 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -17,6 +17,7 @@ from git import Actor, Commit, Repo from git.exc import UnsafeOptionError +from git.index.typ import BaseIndexEntry from git.objects.util import tzoffset, utc from git.repo.fun import touch @@ -166,6 +167,36 @@ def check_entries(d, has_change_type=False): self.assertEqual(commit.committer_tz_offset, 14400, commit.committer_tz_offset) self.assertEqual(commit.message, "initial project\n") + @with_rw_directory + def test_stats_preserve_filename_spaces(self, rw_dir): + repo = Repo.init(rw_dir) + actor = Actor("Test", "test@example.com") + paths = ("name", " name", "name ", " name ", " ") + # Store blobs directly so trailing spaces work on Windows as well. + for multiplier, change_type in ((1, "A"), (2, "M")): + for count, path in enumerate(paths, 1): + data = b"line\n" * count * multiplier + blob = repo.odb.store(IStream("blob", len(data), BytesIO(data))) + repo.index.add([BaseIndexEntry((0o100644, blob.binsha, 0, path))]) + commit = repo.index.commit("Update files", author=actor, committer=actor) + stats = commit.stats + assert stats.total == {"insertions": 15, "deletions": 0, "lines": 15, "files": 5} + assert stats.files == { + path: {"insertions": count, "deletions": 0, "lines": count, "change_type": change_type} + for count, path in enumerate(paths, 1) + } + empty_stats = repo.index.commit("No changes", author=actor, committer=actor).stats + assert empty_stats.files == {} + assert empty_stats.total == {"insertions": 0, "deletions": 0, "lines": 0, "files": 0} + + repo.index.remove(list(paths)) + removed_stats = repo.index.commit("Remove files", author=actor, committer=actor).stats + assert removed_stats.files == { + path: {"insertions": 0, "deletions": 2 * count, "lines": 2 * count, "change_type": "D"} + for count, path in enumerate(paths, 1) + } + assert removed_stats.total == {"insertions": 0, "deletions": 30, "lines": 30, "files": 5} + def test_renames(self): commit = self.rorepo.commit("185d847ec7647fd2642a82d9205fb3d07ea71715") files = commit.stats.files From 963ca1c7d3bccdfad66293d5417f065ce5829b15 Mon Sep 17 00:00:00 2001 From: v1shay Date: Sun, 11 Oct 2026 00:13:14 -0700 Subject: [PATCH 07/25] test(stats): avoid normalizing fixture paths on Windows The stats regression's root and modification assertions pass on Windows, but IndexFile.remove normalizes the all-space test path to `.` and makes `git rm` fail before testing deletion statistics. Clear and write the fixture's in-memory index instead, just as the test already stores its initial blobs directly. The resulting empty-tree commit still exercises deletion statistics for every original filename. The full Linux suite again passes: 1690 passed, 89 skipped, 1 xfailed, and 64 subtests. Focused commit/util/diff tests, pre-commit, mypy, and basedpyright pass. Windows CI will verify the portability correction. Prepared by OpenAI Codex (dot); human review is pending. --- test/test_commit.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/test_commit.py b/test/test_commit.py index ac155e0df..379bc831b 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -189,7 +189,10 @@ def test_stats_preserve_filename_spaces(self, rw_dir): assert empty_stats.files == {} assert empty_stats.total == {"insertions": 0, "deletions": 0, "lines": 0, "files": 0} - repo.index.remove(list(paths)) + # Avoid filesystem path normalization of space-only names on Windows. + index = repo.index + index.entries.clear() + index.write() removed_stats = repo.index.commit("Remove files", author=actor, committer=actor).stats assert removed_stats.files == { path: {"insertions": 0, "deletions": 2 * count, "lines": 2 * count, "change_type": "D"} From 04fa56f761db83e256ee9882adb255604887a169 Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:26:20 +0300 Subject: [PATCH 08/25] fix(config): preserve exact large integer values `GitConfigParser.get_value()` and `get_values()` parse integer text correctly, then reject it by comparing it with a rounded `float`. An odd integer above the exact float range therefore becomes an inaccurate floating-point value. Restrict the comparison to floating values. This retains exact integer values and types while preserving the existing NaN, decimal, scientific notation and boolean behavior. Three integer regression subtests fail on the baseline. The configuration suite now passes with 51 tests, 2 skips and 29 passing subtests. Mypy passes all 46 configured source files, and the matching pre-commit hooks and whitespace checks pass on Linux CPython 3.12.15. Agent: OpenAI Codex (GPT-6.1-sol). --- git/config.py | 4 ++-- test/test_config.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/git/config.py b/git/config.py index a3437d525..7109c09c3 100644 --- a/git/config.py +++ b/git/config.py @@ -1018,8 +1018,8 @@ def _string_to_value(self, valuestr: Union[str, None]) -> Union[int, float, str, for numtype in types: try: val = numtype(valuestr) - # truncated value ? - if val != float(valuestr): + # Reject NaN without coercing exact integer values to float. + if numtype is float and val != float(valuestr): continue return val except (ValueError, TypeError): diff --git a/test/test_config.py b/test/test_config.py index 274738075..8d526457b 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -37,6 +37,38 @@ def tearDown(self): if osp.isfile(lfp): raise AssertionError("Previous TC left hanging git-lock file: {}".format(lfp)) + def test_integer_config_values_keep_precision(self): + for text, expected in ( + ("9007199254740993", 9007199254740993), + ("9223372036854775807", 9223372036854775807), + ("-9007199254740993", -9007199254740993), + ): + with self.subTest(text=text): + config_file = io.BytesIO(("[limits]\nvalue = %s\n" % text).encode("ascii")) + config_file.name = "integer_precision.config" + with GitConfigParser(config_file) as config: + value = config.get_value("limits", "value") + self.assertEqual(value, expected) + self.assertIs(type(value), int) + self.assertEqual(config.get_values("limits", "value"), [expected]) + + def test_numeric_config_conversion_preserves_other_values(self): + for text, expected in ( + ("11.11", 11.11), + ("1e3", 1000.0), + ("nan", "nan"), + ("NaN", "NaN"), + ("true", True), + ("false", False), + ): + with self.subTest(text=text): + config_file = io.BytesIO(("[values]\nvalue = %s\n" % text).encode("ascii")) + config_file.name = "numeric_conversion.config" + with GitConfigParser(config_file) as config: + value = config.get_value("values", "value") + self.assertEqual(value, expected) + self.assertIs(type(value), type(expected)) + def _to_memcache(self, file_path): with open(file_path, "rb") as fp: sio = io.BytesIO(fp.read()) From fa45526e9d87ddb71de7dc0fbde24a916f2051b2 Mon Sep 17 00:00:00 2001 From: fan wu <336798967+FanWu-ai@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:19:24 -0700 Subject: [PATCH 09/25] fix: resolve IterableList names independently of list attributes Normal attribute lookup makes string indexing return list methods for item names such as `count`, and reports absent `append` identifiers as present. Use `__getattr__` explicitly for identifier lookup and remove its fallback to list attributes. Ordinary attribute access still exposes list methods. Add twelve present/absent collision cases with and without prefixes. The full suite passes 1701 tests, 89 conditional skips, one existing xfail, and 64 subtests; pinned hooks, type checks, docs and packaging also pass. --- git/util.py | 6 +++--- test/test_util.py | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/git/util.py b/git/util.py index 61ddab861..c007c93ed 100644 --- a/git/util.py +++ b/git/util.py @@ -1382,7 +1382,7 @@ def __contains__(self, attr: object) -> bool: # Otherwise make a full name search. try: - getattr(self, cast(str, attr)) # Use cast to silence mypy. + self.__getattr__(cast(str, attr)) # Use cast to silence mypy. return True except (AttributeError, TypeError): return False @@ -1394,7 +1394,7 @@ def __getattr__(self, attr: str) -> T_IterableObj: if getattr(item, self._id_attr) == attr: return item # END for each item - return list.__getattribute__(self, attr) + raise AttributeError(f"No item found with id {attr}") def __getitem__( # type: ignore[override] # pyright: ignore[reportIncompatibleMethodOverride] self, index: Union[SupportsIndex, int, slice, str] @@ -1405,7 +1405,7 @@ def __getitem__( # type: ignore[override] # pyright: ignore[reportIncompatible raise ValueError("Index should be an int or str") else: try: - return getattr(self, cast(str, index)) + return self.__getattr__(cast(str, index)) except AttributeError as e: raise IndexError(f"No item found with id {self._prefix}{index}") from e # END handle getattr diff --git a/test/test_util.py b/test/test_util.py index ca8e006f3..d759343f0 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -862,3 +862,23 @@ def test_mode_str_to_int_accepts_bytes(): assert mode_str_to_int(b"100644") == 0o100644 assert mode_str_to_int("644") == 0o644 assert mode_str_to_int(b"120000") == 0o120000 + + +@pytest.mark.parametrize("name", ["sort", "count", "append", "_prefix", "__class__", "ordinary"]) +@pytest.mark.parametrize("prefix", ["", "refs/"]) +def test_iterable_list_name_lookup_ignores_list_attributes(name, prefix): + values = IterableList("name", prefix) + member = _Member(prefix + name) + values.append(member) + + assert values[name] is member + assert name in values + assert member in values + assert values[0] is member + assert callable(values.append) + assert callable(values.sort) + + del values[name] + assert name not in values + with pytest.raises(IndexError, match="No item found with id"): + values[name] From 76150aae9810e0ab8c9d99d0391639a2c73cb97c Mon Sep 17 00:00:00 2001 From: fan wu <336798967+FanWu-ai@users.noreply.github.com> Date: Sun, 11 Oct 2026 11:08:58 -0700 Subject: [PATCH 10/25] fix(util): wait for contended locks with relative filenames `BlockingLockFile('resource')` incorrectly treats the empty `dirname` of `resource.lock` as a missing directory when another holder owns the lock. Use `os.curdir` for that case so the retry and timeout paths work normally. Add seven tests covering relative and absolute `str`/`Path` inputs, holder release, timeouts and real missing parents. The complete Linux/Python 3.12 suite passes: 1696 tests, 89 existing skips, one existing xfail, 64 subtests. Pinned hooks, mypy, basedpyright, docs, builds and strict Twine also pass. Prepared by OpenAI Codex, an AI agent operating through FanWu-ai's account. --- git/util.py | 2 +- test/test_util.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index a694f4fe9..17a32c01a 100644 --- a/git/util.py +++ b/git/util.py @@ -1310,7 +1310,7 @@ def _obtain_lock(self) -> None: # synity check: if the directory leading to the lockfile is not # readable anymore, raise an exception curtime = time.time() - if not osp.isdir(osp.dirname(self._lock_file_path())): + if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir): msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % ( self._lock_file_path(), curtime - starttime, diff --git a/test/test_util.py b/test/test_util.py index d759343f0..7fe507b8f 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -882,3 +882,40 @@ def test_iterable_list_name_lookup_ignores_list_attributes(name, prefix): assert name not in values with pytest.raises(IndexError, match="No item found with id"): values[name] + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_waits_for_existing_holder(tmp_path, monkeypatch, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + owner = LockFile(path) + waiter = BlockingLockFile(path, check_interval_s=0.01, max_block_time_s=1) + owner._obtain_lock() + try: + with mock.patch.object(time, "sleep", side_effect=lambda _: owner._release_lock()) as sleep: + waiter._obtain_lock() + sleep.assert_called_once_with(0.01) + assert waiter._has_lock() + finally: + owner._release_lock() + waiter._release_lock() + + +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + owner = LockFile(path_type("resource")) + waiter = BlockingLockFile(path_type("resource"), max_block_time_s=0) + owner._obtain_lock() + try: + with pytest.raises(OSError, match="Waited .* seconds for lock"): + waiter._obtain_lock() + finally: + owner._release_lock() + + +def test_blocking_lock_still_rejects_missing_directory(tmp_path): + waiter = BlockingLockFile(tmp_path / "missing" / "resource") + with pytest.raises(OSError, match="Directory containing the lockfile"): + waiter._obtain_lock() From 3b9bb6df3c3caca808e93f77e3eac7b97d24c571 Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 14 Oct 2026 09:47:31 +0200 Subject: [PATCH 11/25] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P2] Propagate non-contention errors before retrying relative locks — git/util.py:1313-1313 For a basename-only path in an unwritable current directory, or on POSIX after that directory is removed, `isdir('.')` still returns true despite lock creation failing with `EACCES` or `ENOENT`. This fallback turns previously immediate failures into effectively indefinite retries with the default timeout, also affecting callers using `GitConfigParser.t_lock = BlockingLockFile`. Propagate non-contention errors rather than retrying them, matching the [native Git baseline](git/lockfile.c#L234-L235), which retries only `EEXIST`. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- git/util.py | 14 +++++---- test/test_config.py | 20 ++++++++++++- test/test_util.py | 72 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 95 insertions(+), 11 deletions(-) diff --git a/git/util.py b/git/util.py index 17a32c01a..874b764d5 100644 --- a/git/util.py +++ b/git/util.py @@ -1270,8 +1270,8 @@ class BlockingLockFile(LockFile): specified timeout. :note: - If the directory containing the lock was removed, an exception will be raised - during the blocking period, preventing hangs as the lock can never be obtained. + Only an existing lock causes retries. Other errors, including a removed or + unwritable parent directory, are raised immediately. """ __slots__ = ("_check_interval", "_max_block_time") @@ -1297,7 +1297,7 @@ def __init__( def _obtain_lock(self) -> None: """This method blocks until it obtained the lock, or raises :exc:`IOError` if it - ran out of time or if the parent directory was not available anymore. + ran out of time or lock creation failed for a reason other than an existing lock. If this method returns, you are guaranteed to own the lock. """ @@ -1307,8 +1307,12 @@ def _obtain_lock(self) -> None: try: super()._obtain_lock() except OSError as e: - # synity check: if the directory leading to the lockfile is not - # readable anymore, raise an exception + # LockFile retains the creation error as the cause of its OSError. + # Only an existing lock is contention; other errors must not be retried. + if not isinstance(e.__cause__, FileExistsError): + raise + + # Abort if the directory containing the existing lock has disappeared. curtime = time.time() if not osp.isdir(osp.dirname(self._lock_file_path()) or osp.curdir): msg = "Directory containing the lockfile %r was not readable anymore after waiting %g seconds" % ( diff --git a/test/test_config.py b/test/test_config.py index 8d526457b..e3c30421c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -3,6 +3,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import errno import glob import io import os @@ -17,7 +18,7 @@ from git import GitConfigParser, Repo from git.compat import defenc from git.config import _OMD, cp -from git.util import cwd, rmfile +from git.util import BlockingLockFile, cwd, rmfile from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory _tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock") @@ -28,6 +29,23 @@ def _rm_lock_files(): rmfile(lfp) +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize("error_number", [errno.EACCES, errno.ENOENT]) +def test_blocking_config_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number): + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(GitConfigParser, "t_lock", BlockingLockFile) + error = OSError(error_number, os.strerror(error_number), "config.lock") + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + GitConfigParser("config", read_only=False) + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not (tmp_path / "config.lock").exists() + + class TestBase(TestCase): def setUp(self): _rm_lock_files() diff --git a/test/test_util.py b/test/test_util.py index 7fe507b8f..ca592aaf7 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -4,7 +4,7 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ import ast -from datetime import datetime +import errno import os import pathlib import pickle @@ -14,6 +14,7 @@ import tempfile import threading import time +from datetime import datetime from unittest import SkipTest, mock import ddt @@ -37,12 +38,11 @@ LockFile, cygpath, decygpath, - is_cygwin_git, get_user_id, + is_cygwin_git, remove_password_if_present, rmtree, ) - from test.lib import TestBase, requires_symlinks, with_rw_repo @@ -917,5 +917,67 @@ def test_blocking_relative_lock_reports_timeout(tmp_path, monkeypatch, path_type def test_blocking_lock_still_rejects_missing_directory(tmp_path): waiter = BlockingLockFile(tmp_path / "missing" / "resource") - with pytest.raises(OSError, match="Directory containing the lockfile"): - waiter._obtain_lock() + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock() + + +@pytest.mark.skipif(sys.platform == "win32", reason="Lock creation uses CreateFileW on Windows") +@pytest.mark.parametrize( + "error_number", [errno.EACCES, errno.ENOENT, errno.ENOTDIR, errno.EROFS, errno.ENOSPC, errno.EIO] +) +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +@pytest.mark.parametrize("relative", [True, False]) +def test_blocking_lock_propagates_non_contention_errors(tmp_path, monkeypatch, error_number, path_type, relative): + monkeypatch.chdir(tmp_path) + path = path_type("resource" if relative else tmp_path / "resource") + waiter = BlockingLockFile(path) + error = OSError(error_number, os.strerror(error_number), waiter._lock_file_path()) + with mock.patch.object(os, "open", side_effect=error) as open_lock: + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + open_lock.assert_called_once() + assert exc_info.value.__cause__ is error + assert str(exc_info.value) == str(error) + assert not waiter._has_lock() + assert not os.path.lexists(waiter._lock_file_path()) + + +@pytest.mark.skipif(os.name != "posix", reason="Directory write permissions require POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_unwritable_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + original_mode = tmp_path.stat().st_mode + tmp_path.chmod(stat.S_IRUSR | stat.S_IXUSR) + try: + if os.access(".", os.W_OK): + pytest.skip("Current user can write to read-only directories") + assert os.path.isdir(".") + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, PermissionError) + assert not waiter._has_lock() + finally: + tmp_path.chmod(original_mode) + + +@pytest.mark.skipif(os.name != "posix", reason="Removing the current directory requires POSIX") +@pytest.mark.parametrize("path_type", [str, pathlib.Path]) +def test_blocking_relative_lock_rejects_removed_cwd(tmp_path, monkeypatch, path_type): + monkeypatch.chdir(tmp_path) + tmp_path.rmdir() + waiter = BlockingLockFile(path_type("resource")) + with mock.patch.object(time, "sleep", side_effect=AssertionError("Must not retry")) as sleep: + with pytest.raises(OSError) as exc_info: + waiter._obtain_lock() + sleep.assert_not_called() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + assert not waiter._has_lock() From c179357b38a8962f1c8e33153ac58b5ac0e6e96a Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 14 Oct 2026 14:19:03 +0200 Subject: [PATCH 12/25] prepare changelog prior to release --- VERSION | 2 +- doc/source/changes.rst | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/VERSION b/VERSION index 944880fa1..e4604e3af 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.2.0 +3.2.1 diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 8447228db..95178088d 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -10,7 +10,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-83vg-56qc-22m7 If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.2.1 @@ -35,7 +35,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-fx3j-rwgx-fr94 If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.2.0 @@ -48,7 +48,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-59cr-6r3x-644w If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.62 @@ -60,7 +60,7 @@ A fixup release to avoid accidental removal of public class regex on Actor. It's now deprecated instead. If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.61 @@ -75,7 +75,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-239g-whfq-7xj9 If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.60 @@ -92,7 +92,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-7833-fr7j-v32q If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.59 @@ -110,7 +110,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-hmq2-w58f-27jc If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.58 @@ -124,7 +124,7 @@ A security fix for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-539m-9xh6-q6rr If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.57 @@ -137,7 +137,7 @@ A security fix for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-p538-c434-8v24 If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.56 @@ -150,7 +150,7 @@ A security fix for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-94p4-4cq8-9g67 If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.55 @@ -165,7 +165,7 @@ A security fix for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-fjr4-x663-mwxc If you can, also try and provide feedback on the upcoming v4 branch -https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.54 @@ -177,7 +177,7 @@ 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. +https://github.com/gitpython-developers/GitPython/pull/2274 - patches welcome. See the following for all changes. https://github.com/gitpython-developers/GitPython/releases/tag/3.1.53 From b1454596dda0561610f0d5438962caf501b4cd97 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Wed, 14 Oct 2026 20:14:58 +0530 Subject: [PATCH 13/25] fix(commit): parse the `encoding` header when deserializing `Commit._deserialize` compared a 10-byte slice of each stripped header line against the 9-byte literal `b"encoding "`, so the comparison never matched and the branch reading a commit's declared encoding was unreachable. Every commit parsed from the object database was decoded with the default UTF-8: `Commit.encoding` reported `UTF-8` regardless of the header, and the author, committer, and message of a commit written under `i18n.commitencoding` came back with U+FFFD replacement characters while `git log` shows them correctly. Re-serializing such a commit also dropped its `encoding` header and stored the garbled text. Compare the right slice length so the header is read. Adopt the declared encoding only when Python has a text codec for it: the identity lines are decoded with `errors="replace"`, which does not guard against an unknown codec name, so a commit declaring one would otherwise raise `LookupError` on attribute access. An unknown name keeps the previous behavior (the default encoding) and logs the error. Validated against a repository committed under `i18n.commitencoding = ISO-8859-1`: `commit.encoding`, `commit.author.name`, and `commit.message` now match what `git log` shows. Adds regression tests in `test/test_commit.py` that fail before the change. --- git/objects/commit.py | 13 +++++++++++-- test/test_commit.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/git/objects/commit.py b/git/objects/commit.py index 690a296a5..0c034d231 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -896,8 +896,17 @@ def _deserialize(self, stream: BytesIO) -> "Commit": enc = next_line buf = enc.strip() while buf: - if buf[0:10] == b"encoding ": - self.encoding = buf[buf.find(b" ") + 1 :].decode(self.encoding, "ignore") + if buf[0:9] == b"encoding ": + encoding = buf[buf.find(b" ") + 1 :].decode(self.encoding, "ignore") + try: + # Probe one byte: decoding b"" skips the codec lookup entirely. + b" ".decode(encoding, "replace") + except (LookupError, ValueError): + # The encoding is not a text encoding Python knows; keep the + # default so the identity and message lines still decode below. + _logger.error("Ignoring unknown commit encoding %r", encoding) + else: + self.encoding = encoding elif buf[0:7] == b"gpgsig ": sig_lines = [buf[buf.find(b" ") + 1 :] + b"\n"] is_next_header = False diff --git a/test/test_commit.py b/test/test_commit.py index 379bc831b..da8b49325 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -458,6 +458,41 @@ def test_invalid_commit(self): self.assertEqual(cmt.author.name, "E.Azer Ko�o�o�oculu", cmt.author.name) self.assertEqual(cmt.author.email, "azer@kodfabrik.com", cmt.author.email) + def test_deserialization_declared_encoding(self): + # The "encoding" header names the encoding of the identity and message bytes. + data = ( + b"tree 3385f7b1e5f91930a3bd3e07105e113fca29981d\n" + b"author Andr\xe9 1112591163 +0200\n" + b"committer Andr\xe9 1112591163 +0200\n" + b"encoding ISO-8859-1\n" + b"\n" + b"R\xe9sum\xe9\n" + ) + cmt = self.rorepo.commit() + cmt._deserialize(BytesIO(data)) + + self.assertEqual(cmt.encoding, "ISO-8859-1") + self.assertEqual(cmt.author.name, "André") + self.assertEqual(cmt.committer.name, "André") + self.assertEqual(cmt.message, "Résumé\n") + + def test_deserialization_unknown_declared_encoding(self): + # An encoding Python has no text codec for falls back to the default encoding. + data = ( + b"tree 3385f7b1e5f91930a3bd3e07105e113fca29981d\n" + b"author Andr\xe9 1112591163 +0200\n" + b"committer Andr\xe9 1112591163 +0200\n" + b"encoding pony-speak\n" + b"\n" + b"R\xe9sum\xe9\n" + ) + cmt = self.rorepo.commit() + cmt._deserialize(BytesIO(data)) + + self.assertEqual(cmt.encoding, cmt.default_encoding) + self.assertEqual(cmt.author.name, "Andr�") + self.assertEqual(cmt.message, "R�sum�\n") + @with_rw_directory def test_identity_cannot_alter_headers(self, rw_dir): """A name or email must not add header lines or present another identity.""" From 2c9b9a9e0703dc1ca878f4313df83d9418e108ed Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 15 Oct 2026 05:01:58 +0200 Subject: [PATCH 14/25] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I didn't try to fix all the issues with this, let's just make it a little bit better hoping the code won't be around much longer. # Known limitations Commit.encoding still conflates the declared encoding with the codec used for best-effort decoding. Unsupported or malformed declarations leave the default encoding—or an earlier accepted declaration—in place, so the resulting attribute does not faithfully represent the commit’s original metadata. The rejected header bytes appear in the diagnostic but are not retained on the object. Fallback decoding with "replace" can irreversibly alter identity and message content. Reserializing such a commit can therefore change both its encoding declaration and its content; this change does not provide lossless round-tripping. Codec validation also checks only that Python accepts the codec, not that it can decode the actual commit content. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- git/objects/commit.py | 10 ++++++---- test/test_commit.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/git/objects/commit.py b/git/objects/commit.py index 0c034d231..3239079c1 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -897,14 +897,16 @@ def _deserialize(self, stream: BytesIO) -> "Commit": buf = enc.strip() while buf: if buf[0:9] == b"encoding ": - encoding = buf[buf.find(b" ") + 1 :].decode(self.encoding, "ignore") + raw_encoding = buf[9:] try: + # Codec names are ASCII, independent of the content encoding. + encoding = raw_encoding.decode("ascii") # Probe one byte: decoding b"" skips the codec lookup entirely. b" ".decode(encoding, "replace") except (LookupError, ValueError): - # The encoding is not a text encoding Python knows; keep the - # default so the identity and message lines still decode below. - _logger.error("Ignoring unknown commit encoding %r", encoding) + # Reject malformed names and non-text codecs without changing + # the current encoding (the default unless a valid header was read). + _logger.error("Ignoring invalid commit encoding %r", raw_encoding) else: self.encoding = encoding elif buf[0:7] == b"gpgsig ": diff --git a/test/test_commit.py b/test/test_commit.py index da8b49325..a22daa87a 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -493,6 +493,26 @@ def test_deserialization_unknown_declared_encoding(self): self.assertEqual(cmt.author.name, "Andr�") self.assertEqual(cmt.message, "R�sum�\n") + def test_deserialization_multiple_encoding_headers(self): + for headers in ( + b"encoding utf-16-be\nencoding ISO-8859-1\n", + b"encoding ISO-8859-1\nencoding utf-\xff8\n", + ): + with self.subTest(headers=headers): + data = ( + b"tree 3385f7b1e5f91930a3bd3e07105e113fca29981d\n" + b"author Andr\xe9 1112591163 +0200\n" + b"committer Andr\xe9 1112591163 +0200\n" + headers + b"\n" + b"R\xe9sum\xe9\n" + ) + cmt = self.rorepo.commit() + cmt._deserialize(BytesIO(data)) + + self.assertEqual(cmt.encoding, "ISO-8859-1") + self.assertEqual(cmt.author.name, "André") + self.assertEqual(cmt.committer.name, "André") + self.assertEqual(cmt.message, "Résumé\n") + @with_rw_directory def test_identity_cannot_alter_headers(self, rw_dir): """A name or email must not add header lines or present another identity.""" From e264eaf7ec2bfdd474012ac45b83e7c2901f868f Mon Sep 17 00:00:00 2001 From: 6801318d8d <144167388+6801318d8d@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:59:29 +0200 Subject: [PATCH 15/25] fix(index): pass a Windows hook outside the work tree by absolute path On Windows, `run_commit_hook` runs an extensionless hook through Bash and passes the hook's path relative to the working tree (`os.path.relpath`). Bash resolves that path in its own view of the filesystem, which differs from the Windows one across a mount point. Git for Windows mounts the user's temporary directory as `/tmp` (`none /tmp usertemp` in its `/etc/fstab`). For a repository under `%TEMP%` and a `core.hooksPath` outside it, such as a global `~/.config/git/hooks`, Bash starts in `/tmp/tmpXXXX/repo`, and `../../../../../.config/git/hooks/pre-commit`, correct on the Windows side, leads to `/.config/git/hooks/pre-commit`. Every commit then fails with exit code 127 and `No such file or directory`. A relative path that stays inside the working tree cannot cross such a mount, so it is kept. A path that leaves the working tree (one starting with `..`) is now passed as the absolute path in POSIX form (`C:/...`), as is already done for a hook on another drive, when the Bash is Git for Windows' own or one found on `PATH`: both accept it. The bare `bash.exe` fallback may be the WSL launcher, which cannot open a Windows absolute path, so it keeps the relative path as before. `test_run_commit_hook_outside_worktree_on_windows` now checks the three cases (outside the working tree with a found Bash, inside it, outside it with the bare fallback), and `test_pre_commit_hook_outside_worktree` runs a real hook through a `core.hooksPath` outside the working tree. Validated on Windows 11 with Git for Windows 2.50.1 and Python 3.14: `test/test_index.py` passes with an empty global Git configuration (183 passed, 3 skipped, 1 xpassed), and the new command-construction assertion fails without the change; `ruff` 0.16.9 and `mypy` report no issues on the changed files; a commit in a repository under `%TEMP%`, with a global `core.hooksPath` holding a `pre-commit` hook, succeeds instead of failing with exit code 127. Co-Authored-By: Claude Opus 5.5 --- git/index/fun.py | 21 +++++++++++----- test/test_index.py | 62 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/git/index/fun.py b/git/index/fun.py index 2491f55ae..70c5a7e50 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -198,6 +198,11 @@ 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. + # Prefer Bash associated with GitPython's selected Git installation. If + # that layout is not recognized, use an explicitly configured non-system + # PATH entry. Preserve the bare fallback for installations that previously + # relied on WSL or another CreateProcess-resolved Bash. + found_bash = _git_for_windows_bash() or _which_from_path("bash.exe") try: bash_hp = osp.relpath(hp, index.repo.working_dir) except ValueError: @@ -205,12 +210,16 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: # 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 - # Prefer Bash associated with GitPython's selected Git installation. If - # that layout is not recognized, use an explicitly configured non-system - # PATH entry. Preserve the bare fallback for installations that previously - # relied on WSL or another CreateProcess-resolved Bash. - bash_executable = _git_for_windows_bash() or _which_from_path("bash.exe") or "bash.exe" - cmd = [bash_executable, Path(bash_hp).as_posix()] + else: + # Bash resolves a relative path in its own view of the filesystem, + # which differs from Windows' across a mount point: Git Bash mounts + # the user's temporary directory as /tmp, so from a working tree + # there, ".." leads elsewhere. A path leaving the working tree is + # thus passed whole to a Bash found above, which accepts it; the + # bare fallback may be WSL's, which does not, and keeps it relative. + if found_bash and bash_hp.startswith(os.pardir + os.sep): + bash_hp = hp + cmd = [found_bash or "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 23e59cc04..457a64ec2 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -1513,6 +1513,7 @@ def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir): repo = Repo.init(root / "repo") hooks_dir = root / "hooks" _make_hook(root, "fake-hook", "exit 0") + _make_hook(repo.working_dir, "fake-hook", "exit 0") system_root = root / "Windows" system_bash = system_root / "System32" / "bash.exe" git_executable = root / "Git" / "cmd" / "git.exe" @@ -1521,24 +1522,37 @@ def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir): executable.parent.mkdir(parents=True) executable.touch() executable.chmod(0o755) - with repo.config_writer() as writer: - writer.set_value("core", "hooksPath", str(hooks_dir)) + + def command(hooks_path, exec_path): + with repo.config_writer() as writer: + writer.set_value("core", "hooksPath", hooks_path) + with mock.patch("git.index.fun.sys.platform", "win32"), mock.patch.object( + Git, "GIT_PYTHON_GIT_EXECUTABLE", "git" + ), mock.patch.dict(os.environ, {"SystemRoot": str(system_root)}), mock.patch( + "git.index.fun.os.get_exec_path", return_value=exec_path + ): + 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) + return popen.call_args[0][0] # Model a normal Windows PATH: System32 (containing the WSL launcher) comes # before Git's cmd directory, while Git's Bash is not itself on PATH. This # exercises both Git-installation discovery and shell selection without # mocking either resolver's answer. - with mock.patch("git.index.fun.sys.platform", "win32"), mock.patch.object( - Git, "GIT_PYTHON_GIT_EXECUTABLE", "git" - ), mock.patch.dict(os.environ, {"SystemRoot": str(system_root)}), mock.patch( - "git.index.fun.os.get_exec_path", return_value=["", str(system_bash.parent), str(git_executable.parent)] - ): - 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, [str(git_bash), "../hooks/fake-hook"]) + with_git = ["", str(system_bash.parent), str(git_executable.parent)] + # A hook outside the working tree is passed by its absolute path: a relative + # one is resolved in Bash's own view of the filesystem, where a mount point + # (Git Bash's /tmp) can make ".." lead elsewhere. + self.assertEqual(command(str(hooks_dir), with_git), [str(git_bash), (hooks_dir / "fake-hook").as_posix()]) + # A hook inside the working tree keeps its relative path. + self.assertEqual(command("hooks", with_git), [str(git_bash), "hooks/fake-hook"]) + # So does one outside it for the bare fallback, which may be WSL's bash.exe, + # unable to open a Windows absolute path. + without_git = ["", str(system_bash.parent)] + self.assertEqual(command(str(hooks_dir), without_git), ["bash.exe", "../hooks/fake-hook"]) @with_rw_directory def test_windows_bash_lookup_respects_explicit_current_directory_in_path(self, rw_dir): @@ -1671,6 +1685,28 @@ def test_pre_commit_hook_respects_core_hooks_path(self, rw_repo): 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( + 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_directory + def test_pre_commit_hook_outside_worktree(self, rw_dir): + repo = Repo.init(osp.join(rw_dir, "repo")) + hp = _make_hook(rw_dir, "pre-commit", "echo 'ran outside hook' >outside-hook-output.txt") + + with repo.config_writer() as writer: + writer.set_value("core", "hooksPath", osp.dirname(hp)) + + repo.index.commit("This should run the hook outside the working tree") + output = Path(repo.working_dir, "outside-hook-output.txt").read_text(encoding="utf-8") + self.assertEqual(output, "ran outside 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 833c8d8d3e5347e95f782cf1cc0b9fab60569110 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Fri, 16 Oct 2026 14:03:21 +0530 Subject: [PATCH 16/25] fix(submodule): resolve relative submodule URLs the way git does `Submodule.update` resolved a `.gitmodules` URL starting with `./` or `../` with `urllib.parse.urljoin` against the superproject's `origin` URL, and `Submodule._clone_repo` appended a `../` URL to the tracking remote's URL with `os.path.join`. Neither follows git's `relative_url`, and `urljoin` only resolves against the schemes it knows: for an `ssh://` or `git://` origin it returns the relative URL unchanged, and for an scp-like `host:path` origin more than one `../` collapses to a bare path. `git clone` then received `./planted`, a path inside the superproject's checkout, where git resolves the same entry to `ssh://host/org/super.git/planted`. A submodule was therefore cloned from a directory committed by whoever authored the superproject, through git's local clone path: `git upload-pack` ran inside that directory and its objects were copied without the `transfer.fsckObjects` checks a transport fetch applies (an object that `git clone file://...` rejects as `badDate` is accepted this way). Relative submodule URLs are common in multi-repository setups, and the superproject is untrusted input to any service that updates submodules of repositories it is given. Add `resolve_relative_url`, a port of git's `relative_url`: each leading `../` strips one component of the remote URL, a path component first, then the scp-like host before the colon, and running out of components is an error; `./` prefixes are dropped and the rest is appended. It agrees with what `git submodule init` records over 272 remote and URL pairs, including the error cases. Use it in `Submodule.update`, `Submodule._clone_repo` (which now also resolves `./` URLs, previously passed to `git clone` verbatim), and the URL change path of `RootModule.update`, which created a remote from the raw relative URL and fetched it as a path relative to the module's working tree. The remote used for resolution is unchanged: `origin` in `update`, the tracking branch's remote in `_clone_repo`. Adds regression tests in `test/test_submodule.py`; the two behavioral ones fail before the change with `./planted` reaching the clone unchanged. --- git/objects/submodule/base.py | 20 +++++--- git/objects/submodule/root.py | 12 +++-- git/objects/submodule/util.py | 94 +++++++++++++++++++++++++++++++++++ test/test_submodule.py | 63 +++++++++++++++++++++++ 4 files changed, 177 insertions(+), 12 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 74fbd39fe..d17404ea3 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -12,7 +12,6 @@ import shlex import stat import sys -import urllib.parse import uuid from io import BytesIO from pathlib import Path @@ -58,7 +57,9 @@ from .util import ( SubmoduleConfigParser, find_first_remote_branch, + is_relative_url, mkhead, + resolve_relative_url, sm_name, sm_section, ) @@ -428,10 +429,10 @@ def _clone_repo( if not osp.isdir(module_abspath_dir): os.makedirs(module_abspath_dir) - if url.startswith("../"): + if is_relative_url(url): remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name repo_remote_url = repo.remote(remote_name).url - url = os.path.join(repo_remote_url, url) + url = resolve_relative_url(repo_remote_url, url) clone = git.Repo.clone_from( url, @@ -982,13 +983,9 @@ def fetch_remotes(module_repo: "Repo") -> None: if not dry_run: if no_fetch: raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None - 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._resolved_url(), self.path, self.name, n=True, @@ -1738,6 +1735,13 @@ def url(self) -> str: """:return: The url to the repository our submodule's repository refers to""" return self._url + def _resolved_url(self) -> str: + """:return: :attr:`url`, with a ``./`` or ``../`` prefix resolved against the + url of the superproject's ``origin`` remote the way git does""" + if not is_relative_url(self.url): + return self.url + return resolve_relative_url(self.repo.remotes.origin.url, self.url) + @property def parent_commit(self) -> "Commit": """ diff --git a/git/objects/submodule/root.py b/git/objects/submodule/root.py index a41c50009..047f15727 100644 --- a/git/objects/submodule/root.py +++ b/git/objects/submodule/root.py @@ -264,10 +264,14 @@ def update( # type: ignore[override] nn = "__new_origin__" smm = sm.module() rmts = smm.remotes + # Relative urls are resolved against the superproject's remote, + # which is what the module's remotes hold. + sm_url = sm._resolved_url() + psm_url = psm._resolved_url() # Don't do anything if we already have the url we search in # place. - if len([r for r in rmts if r.url == sm.url]) == 0: + if len([r for r in rmts if r.url == sm_url]) == 0: progress.update( BEGIN | URLCHANGE, i, @@ -278,7 +282,7 @@ def update( # type: ignore[override] if not dry_run: previous_remote = None for remote in rmts: - if remote.url == psm.url: + if remote.url == psm_url: previous_remote = remote break # END if urls match @@ -305,10 +309,10 @@ def update( # type: ignore[override] # A new remote would have no cached refs. Preserve # the existing refs and tracking configuration for # offline updates instead of replacing the remote. - previous_remote.set_url(git.Git.polish_url(sm.url, expand_vars=False)) + previous_remote.set_url(git.Git.polish_url(sm_url, expand_vars=False)) else: assert nn not in [r.name for r in rmts] - smr = smm.create_remote(nn, sm.url) + smr = smm.create_remote(nn, sm_url) smr.fetch(progress=progress) # If we have a tracking branch, it should be available diff --git a/git/objects/submodule/util.py b/git/objects/submodule/util.py index c021510d8..9856e39ce 100644 --- a/git/objects/submodule/util.py +++ b/git/objects/submodule/util.py @@ -6,10 +6,13 @@ "sm_name", "mkhead", "find_first_remote_branch", + "is_relative_url", + "resolve_relative_url", "SubmoduleConfigParser", ] from io import BytesIO +import sys import weakref import git @@ -63,6 +66,97 @@ def find_first_remote_branch(remotes: Sequence["Remote"], branch_name: str) -> " raise InvalidGitRepositoryError("Didn't find remote branch '%r' in any of the given remotes" % branch_name) +def _is_dir_sep(char: str) -> bool: + return char == "/" or (sys.platform == "win32" and char == "\\") + + +def _starts_with_dot_slash(url: str) -> bool: + return len(url) >= 2 and url[0] == "." and _is_dir_sep(url[1]) + + +def _starts_with_dot_dot_slash(url: str) -> bool: + return len(url) >= 3 and url[0:2] == ".." and _is_dir_sep(url[2]) + + +def _has_dos_drive_prefix(url: str) -> bool: + return sys.platform == "win32" and len(url) >= 2 and url[0].isalpha() and url[1] == ":" + + +def _is_absolute_path(url: str) -> bool: + return (len(url) >= 1 and _is_dir_sep(url[0])) or _has_dos_drive_prefix(url) + + +def _url_is_local_not_ssh(url: str) -> bool: + colon = url.find(":") + slash = url.find("/") + return colon == -1 or (slash != -1 and slash < colon) or _has_dos_drive_prefix(url) + + +def _find_last_dir_sep(url: str) -> int: + for index in range(len(url) - 1, -1, -1): + if _is_dir_sep(url[index]): + return index + return -1 + + +def is_relative_url(url: str) -> bool: + """:return: ``True`` if `url` starts with ``./`` or ``../``, which git resolves + against the superproject's remote rather than treating as a path""" + return _starts_with_dot_slash(url) or _starts_with_dot_dot_slash(url) + + +def resolve_relative_url(remote_url: str, url: str) -> str: + """Resolve a submodule URL that starts with ``./`` or ``../`` against the + superproject's remote URL, the way git's ``relative_url`` does. + + Git treats a relative submodule URL like a relative directory: each leading + ``../`` removes the last component of `remote_url`, whether that is a path + component, the host of a URL, or the host of an scp-like ``host:path`` address. + The remainder is appended with ``/``, or with ``:`` once an scp-like host was + reached. A URL that is not relative is returned unchanged. + + :raise ValueError: + If more components are stripped than `remote_url` has, which git rejects with + "cannot strip one component off url". + """ + if not is_relative_url(url): + return url + + remote = remote_url + if remote and _is_dir_sep(remote[-1]): + remote = remote[:-1] + + is_relative = _url_is_local_not_ssh(remote) and not _is_absolute_path(remote) + if is_relative and not _starts_with_dot_slash(remote) and not _starts_with_dot_dot_slash(remote): + remote = "./" + remote + + colonsep = False + while url: + if _starts_with_dot_dot_slash(url): + url = url[3:] + separator = _find_last_dir_sep(remote) + if separator != -1: + remote = remote[:separator] + elif ":" in remote: + remote = remote[: remote.rfind(":")] + colonsep = True + elif is_relative or remote == ".": + raise ValueError("cannot strip one component off url %r" % remote) + else: + remote = "." + elif _starts_with_dot_slash(url): + url = url[2:] + else: + break + + resolved = remote + (":" if colonsep else "/") + url + if url.endswith("/"): + resolved = resolved[:-1] + if _starts_with_dot_slash(resolved): + resolved = resolved[2:] + return resolved + + # } END utilities # { Classes diff --git a/test/test_submodule.py b/test/test_submodule.py index 45b285c55..97d51d6a0 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -26,6 +26,7 @@ ) from git.objects.submodule.base import Submodule from git.objects.submodule.root import RootModule, RootUpdateProgress +from git.objects.submodule.util import resolve_relative_url from git.repo.fun import find_submodule_git_dir, touch from git.util import HIDE_WINDOWS_KNOWN_ERRORS, cwd, join_path_native, to_native_path_linux @@ -2416,3 +2417,65 @@ def test_submodule_update_relative_url(self, rwdir): has_module = any(sm.name == "module" for sm in cloned_repo.submodules) assert has_module, "Relative submodule was not updated properly" + + def test_resolve_relative_url_like_git(self): + # Expected values are what `git submodule init` records for the same pair. + cases = [ + ("ssh://host/org/super.git", "./sub", "ssh://host/org/super.git/sub"), + ("ssh://host/org/super.git", "../sub", "ssh://host/org/sub"), + ("ssh://host:22/org/super.git", "../sub", "ssh://host:22/org/sub"), + ("git@host:org/super.git", "./sub", "git@host:org/super.git/sub"), + ("git@host:org/super.git", "../sub", "git@host:org/sub"), + ("git@host:org/super.git", "../../sub", "git@host:sub"), + ("https://host/org/super.git", "./sub/", "https://host/org/super.git/sub"), + ("https://host/org/super.git/", "././sub", "https://host/org/super.git/sub"), + ("https://host/org/super.git", ".././sub", "https://host/org/sub"), + ("https://host/org/super.git", "../../../sub", "https://sub"), + ("file:///srv/git/super.git", "../sub", "file:///srv/git/sub"), + ("/srv/git/super", "../sub", "/srv/git/sub"), + ("/srv/git/super", "../../../../sub", "sub"), + ("//share/super", "../../sub", "//sub"), + ("foo/bar", "../sub", "foo/sub"), + ("./foo", "../sub", "sub"), + ("../foo/bar", "../sub", "../foo/sub"), + ("ssh://host/org/super.git", ".sub", ".sub"), + ("ssh://host/org/super.git", "https://other/sub", "https://other/sub"), + ] + for remote_url, url, expected in cases: + assert resolve_relative_url(remote_url, url) == expected, (remote_url, url) + + for remote_url, url in [("foo", "../../sub"), ("git@host:super.git", "../../../sub"), ("../foo", "../../sub")]: + with self.assertRaisesRegex(ValueError, "cannot strip one component"): + resolve_relative_url(remote_url, url) + + @with_rw_directory + def test_update_resolves_relative_url_against_remote(self, rwdir): + parent = git.Repo.init(osp.join(rwdir, "parent")) + parent.create_remote("origin", "ssh://git@host/org/super.git") + os.mkdir(osp.join(parent.working_tree_dir, "planted")) + + for url, expected in [ + ("./planted", "ssh://git@host/org/super.git/planted"), + ("../planted", "ssh://git@host/org/planted"), + ]: + submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url) + with mock.patch.object(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo: + with pytest.raises(AssertionError, match="stop"): + submodule.update(init=True) + assert clone_repo.call_args.args[1] == expected + + @with_rw_directory + def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir): + parent = git.Repo.init(osp.join(rwdir, "parent")) + parent.index.commit("initial") + parent.create_remote("origin", "git@host:org/super.git") + parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/main")) + + for url, expected in [ + ("./planted", "git@host:org/super.git/planted"), + ("../planted", "git@host:org/planted"), + ]: + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: + with pytest.raises(AssertionError, match="stop"): + Submodule._clone_repo(parent, url, "module", "module") + assert clone_from.call_args.args[0] == expected From add9920ad62af88db369c6451a1dc9400eebb814 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Fri, 16 Oct 2026 21:24:11 +0530 Subject: [PATCH 17/25] fix(tag): split tag object headers at line feeds only `TagObject._set_cache_` split the decoded tag object with `str.splitlines()` and then read the headers by position, taking `lines[3]` as the tagger. `str.splitlines()` also breaks at `\r`, `\v`, `\f`, `\x1c`-`\x1e`, `\x85`, U+2028 and U+2029, but git ends a header only at a line feed. A U+2028 inside the `tag` header of an untrusted tag object therefore shifts every later header by one line in GitPython only. With `tag v1tagger Linus Torvalds 1700000000 +0000` followed by a real `tagger Mallory ...` line, `git cat-file tag`, `git show` and `git for-each-ref --format=%(taggername)` report Mallory, while GitPython reported `tag == "v1"` and Linus Torvalds as the tagger. The object passes `git fsck --strict` and a clone with `transfer.fsckObjects=true`, because git only warns `badTagName`. The fix splits at `"\n"` and drops the trailing empty element, so tags that only use line feeds parse exactly as before. A `\r` or U+2028 inside the message is now kept, as git shows it. The new test in `test/test_refs.py` fails before the change and passes after it. `ruff`, `ruff format --check` and `mypy` are clean. On my machine the full suite has 3 failures (`test_empty_repo`, `test_rebasing`, `test_diff_with_staged_file`), and all 3 also fail without this change. --- git/objects/tag.py | 11 ++++++++++- test/test_refs.py | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/git/objects/tag.py b/git/objects/tag.py index e08d6ef10..2dbf79f1b 100644 --- a/git/objects/tag.py +++ b/git/objects/tag.py @@ -106,7 +106,16 @@ def _set_cache_(self, attr: str) -> None: """Cache all our attributes at once.""" if attr in TagObject.__slots__: ostream = self.repo.odb.stream(self.binsha) - lines: List[str] = ostream.read().decode(defenc, "replace").splitlines() + # Git ends a header only at a line feed, but str.splitlines() also + # breaks at "\r", "\v", "\f", U+2028 and similar. Splitting a tag + # object that way lets an untrusted "tag" header containing such a + # character shift every later header by a line. Split at "\n" only, + # and drop the trailing empty element so tags that use line feeds + # parse exactly as before. + data = ostream.read().decode(defenc, "replace") + lines: List[str] = data.split("\n") + if lines and lines[-1] == "": + lines.pop() _obj, hexsha = lines[0].split(" ") _type_token, type_name = lines[1].split(" ") diff --git a/test/test_refs.py b/test/test_refs.py index 2edb7b766..42bf5200e 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -7,10 +7,12 @@ import os.path as osp import tempfile import unittest +from io import BytesIO from itertools import chain from pathlib import Path from unittest.mock import patch +from gitdb import IStream from gitdb.exc import BadName import git.refs as refs @@ -139,6 +141,31 @@ def test_tags_author(self): tagger_name = tagobj.tagger.name assert tagger_name == "Michael Trier" + def test_tag_headers_split_at_line_feeds_only(self): + # A "tag" header value carrying a U+2028 (or any separator that + # str.splitlines() honors but git does not) must not shift the later + # headers. Git ends a header only at "\n", so GitPython must read the + # whole value as the tag name and the following line as the tagger, + # exactly as "git cat-file tag" does. See the TagObject parsing fix. + with tempfile.TemporaryDirectory() as tmp_dir: + with self._repo_with_initial_commit(Path(tmp_dir)) as repo: + commit_hexsha = repo.head.commit.hexsha + payload = ( + f"object {commit_hexsha}\n" + "type commit\n" + "tag v1
tagger Linus Torvalds 1700000000 +0000\n" + "tagger Mallory 1700000001 +0000\n" + "\n" + "release\n" + ).encode("utf-8") + istream = repo.odb.store(IStream("tag", len(payload), BytesIO(payload))) + tagobj = TagObject(repo, istream.binsha) + + assert tagobj.tag == "v1
tagger Linus Torvalds 1700000000 +0000" + assert tagobj.tagger.name == "Mallory" + assert tagobj.tagger.email == "m@evil.example" + assert tagobj.message == "release" + def test_tags(self): # Tag refs can point to tag objects or to commits. s = set() From 0a5938dc55c569dbf5f5c60244106ef2011cefdb Mon Sep 17 00:00:00 2001 From: Byron Date: Fri, 16 Oct 2026 11:18:29 +0200 Subject: [PATCH 18/25] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's basically a rubber-stamp of the review comments, hoping this code will be nothing more than interesting tests for the `gix-backend` branch, so the implementation here won't matter. - [P2] Fall back when a relative URL has no tracking remote — git/objects/submodule/base.py:432-435 For a locally initialized superproject without a tracking branch, `create_submodule(..., url='./source')` now enters this block and dereferences `None`, raising `AttributeError` even when `./source` is a valid repository. This addition succeeded before the patch; detached checkouts also fail at `active_branch`. Handle missing tracking branches by falling back to `origin` or the superproject directory, as Git does. - [P2] Resolve relative submodule URLs only once — git/objects/submodule/base.py:986-988 When `origin` is a local relative URL such as `../repos/super.git` and the branch tracks that remote, `_resolved_url()` turns `../child.git` into `../repos/child.git`. Because that result still starts with `../`, `_clone_repo()` resolves it again, producing `../repos/repos/child.git`. This can fail cloning or select the wrong repository. Ensure URLs already resolved by `update()` bypass the second resolution. - [P2] Defer old-URL resolution until a remote change is needed — git/objects/submodule/root.py:269-270 For an initialized superproject without `origin`, changing `.gitmodules` from `../source` to the absolute URL already used by the submodule's remote now raises `AttributeError` in `psm._resolved_url()`. Previously, the existing-remote check skipped URL handling and the update completed. Resolve the previous URL only after checking whether the new URL already matches a remote, so updates requiring no remote adjustment do not fail. --- git/objects/submodule/base.py | 46 ++++++---- git/objects/submodule/root.py | 34 +++---- test/test_submodule.py | 165 +++++++++++++++++++++++++++++++--- 3 files changed, 196 insertions(+), 49 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index d17404ea3..00bb7e226 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -56,6 +56,8 @@ from .util import ( SubmoduleConfigParser, + _is_absolute_path, + _url_is_local_not_ssh, find_first_remote_branch, is_relative_url, mkhead, @@ -69,7 +71,7 @@ if TYPE_CHECKING: from git.index import IndexFile from git.objects.commit import Commit - from git.refs import Head, RemoteReference + from git.refs import Head from git.repo import Repo # ----------------------------------------------------------------------------- @@ -429,13 +431,8 @@ def _clone_repo( if not osp.isdir(module_abspath_dir): os.makedirs(module_abspath_dir) - if is_relative_url(url): - remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name - repo_remote_url = repo.remote(remote_name).url - url = resolve_relative_url(repo_remote_url, url) - clone = git.Repo.clone_from( - url, + cls._resolve_url(repo, url), module_checkout_path, allow_unsafe_options=allow_unsafe_options, allow_unsafe_protocols=allow_unsafe_protocols, @@ -747,8 +744,9 @@ def add( ) # END verify url - ## See #525 for ensuring git URLs in config-files are valid under Windows. - url = Git.polish_url(url, expand_vars=False) + # Relative submodule URLs must keep their prefix to resolve against the parent remote. + if not is_relative_url(url): + url = Git.polish_url(url, expand_vars=False) # It's important to add the URL to the parent config, to let `git submodule` know. # Otherwise there is a '-' character in front of the submodule listing: @@ -985,7 +983,7 @@ def fetch_remotes(module_repo: "Repo") -> None: raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None mrepo = self._clone_repo( self.repo, - self._resolved_url(), + self.url, self.path, self.name, n=True, @@ -1735,12 +1733,30 @@ def url(self) -> str: """:return: The url to the repository our submodule's repository refers to""" return self._url + @staticmethod + def _resolve_url(repo: "Repo", url: str) -> str: + """Resolve relative URLs against the tracking remote, then ``origin``, + falling back to the superproject's directory the way git does.""" + if not is_relative_url(url): + return url + remote_name = "origin" + if not repo.head.is_detached: + with repo.active_branch.config_reader() as reader: + remote_name = reader.get("remote", fallback="origin") + try: + remote_url = repo.remote(remote_name).url + except ValueError: + remote_url = os.fspath(repo.working_dir) + if remote_url == ".": + remote_url = os.fspath(repo.working_dir) + resolved = resolve_relative_url(remote_url, url) + if resolved and _url_is_local_not_ssh(resolved) and not _is_absolute_path(resolved): + resolved = os.path.join(repo.working_dir, resolved) + return resolved + def _resolved_url(self) -> str: - """:return: :attr:`url`, with a ``./`` or ``../`` prefix resolved against the - url of the superproject's ``origin`` remote the way git does""" - if not is_relative_url(self.url): - return self.url - return resolve_relative_url(self.repo.remotes.origin.url, self.url) + """:return: :attr:`url` resolved against the superproject's default remote""" + return self._resolve_url(self.repo, self.url) @property def parent_commit(self) -> "Commit": diff --git a/git/objects/submodule/root.py b/git/objects/submodule/root.py index 047f15727..c1fb4d537 100644 --- a/git/objects/submodule/root.py +++ b/git/objects/submodule/root.py @@ -267,7 +267,6 @@ def update( # type: ignore[override] # Relative urls are resolved against the superproject's remote, # which is what the module's remotes hold. sm_url = sm._resolved_url() - psm_url = psm._resolved_url() # Don't do anything if we already have the url we search in # place. @@ -281,28 +280,21 @@ def update( # type: ignore[override] if not dry_run: previous_remote = None - for remote in rmts: - if remote.url == psm_url: - previous_remote = remote - break - # END if urls match - # END for each remote + if len(rmts) == 1: + previous_remote = rmts[0] + else: + psm_url = psm._resolved_url() + for remote in rmts: + if remote.url == psm_url: + previous_remote = remote + break + # END if urls match + # END for each remote - # If we didn't find a matching remote, but have exactly - # one, we can safely use this one. if previous_remote is None: - if len(rmts) == 1: - previous_remote = rmts[0] - else: - # If we have not found any remote with the - # original URL we may not have a name. This is a - # special case, and its okay to fail here. - # Alternatively we could just generate a unique - # name and leave all existing ones in place. - raise InvalidGitRepositoryError( - "Couldn't find original remote-repo at url %r" % psm.url - ) - # END handle one single remote + raise InvalidGitRepositoryError( + "Couldn't find original remote-repo at url %r" % psm.url + ) # END handle check we found a remote if no_fetch: diff --git a/test/test_submodule.py b/test/test_submodule.py index 97d51d6a0..c09f0a8e1 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -53,6 +53,138 @@ def _patch_git_config(name, value): yield +@pytest.mark.parametrize("state", ["unborn", "untracked", "detached"]) +@pytest.mark.parametrize("remote", [None, ".", "origin"], ids=["working-tree", "local-origin", "origin"]) +def test_add_relative_submodule_without_tracking_remote(tmp_path, state, remote): + with git.Repo.init(tmp_path / "parent") as parent: + if state != "unborn": + parent.index.commit("Initial commit") + if state == "detached": + parent.head.reference = parent.head.commit + base = tmp_path / "remote" if remote == "origin" else tmp_path / "parent" + if remote is not None: + parent.create_remote("origin", str(base) if remote == "origin" else ".") + with git.Repo.init(base / "source") as source: + source.git.symbolic_ref("HEAD", "refs/heads/master") + source.index.commit("Initial commit") + submodule = parent.create_submodule("module", "module", "./source") + assert submodule.url == "./source" + with submodule.module() as module: + assert module.head.commit == source.head.commit + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) + parent.index.commit("Add submodule") + submodule.remove(configuration=False) + submodule.update(init=True) + with submodule.module() as module: + assert module.head.commit == source.head.commit + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) + + +@pytest.mark.parametrize("url", ["./source", "../source", "./../source"]) +def test_add_preserves_relative_url_on_cygwin(tmp_path, url): + with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "source") as source: + source.index.commit("Initial commit") + with mock.patch.object(Submodule, "_clone_repo", return_value=source): + with mock.patch.object(Git, "is_cygwin", return_value=True): + submodule = parent.create_submodule("module", "module", url) + assert submodule.url == url + with submodule.config_reader() as reader: + assert reader.get_value("url") == url + with parent.config_reader() as reader: + assert reader.get_value('submodule "module"', "url") == url + + +def test_update_resolves_local_relative_remote_once(tmp_path): + with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "repos" / "child.git") as source: + source.git.symbolic_ref("HEAD", "refs/heads/master") + source.index.commit("Initial commit") + parent.create_remote("origin", "../repos/super.git") + parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/master")) + submodule = Submodule( + parent, + source.head.commit.binsha, + name="module", + path="module", + url="../child.git", + branch_path="refs/heads/master", + ) + submodule.update(init=True) + with submodule.module() as module: + assert module.head.commit == source.head.commit + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) + module.remotes.origin.fetch() + + +@pytest.mark.parametrize("no_fetch", [False, True]) +def test_root_update_anchors_relative_remote_url(tmp_path, no_fetch): + with git.Repo.init(tmp_path / "repos" / "source.git") as source, git.Repo.init(tmp_path / "parent") as parent: + source.git.symbolic_ref("HEAD", "refs/heads/master") + source.index.commit("Initial commit") + with git.Repo.clone_from(source.working_tree_dir, tmp_path / "repos" / "replacement.git") as replacement: + parent.create_remote("origin", "../repos/super.git") + submodule = parent.create_submodule("module", "module", source.working_tree_dir) + with submodule.config_writer() as writer: + writer.set_value("url", "../source.git") + previous = parent.index.commit("Record relative URL") + with submodule.config_writer() as writer: + writer.set_value("url", "../replacement.git") + parent.index.commit("Change relative URL") + + RootModule(parent).update(previous_commit=previous, no_fetch=no_fetch) + + with submodule.module() as module: + assert Path(module.remotes.origin.url).samefile(replacement.working_tree_dir) + assert module.head.commit == replacement.head.commit + module.remotes.origin.fetch() + + +@pytest.mark.parametrize("no_fetch", [False, True]) +def test_root_update_uses_single_remote_without_resolving_old_url(tmp_path, no_fetch): + with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: + source.git.symbolic_ref("HEAD", "refs/heads/master") + source.index.commit("Initial commit") + with git.Repo.clone_from(source.working_tree_dir, tmp_path / "replacement") as replacement: + parent.create_remote("origin", "../super.git") + submodule = parent.create_submodule("module", "module", source.working_tree_dir) + with submodule.config_writer() as writer: + writer.set_value("url", "../../lib.git") + previous = parent.index.commit("Record historical URL") + with submodule.config_writer() as writer: + writer.set_value("url", replacement.working_tree_dir) + parent.index.commit("Replace historical URL") + + RootModule(parent).update(previous_commit=previous, no_fetch=no_fetch) + + with submodule.module() as module: + assert Path(module.remotes.origin.url).samefile(replacement.working_tree_dir) + assert module.head.commit == replacement.head.commit + + +@pytest.mark.parametrize("dry_run", [False, True]) +def test_root_update_skips_old_url_resolution_when_remote_matches(tmp_path, dry_run): + with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: + source.index.commit("Initial commit") + submodule = parent.create_submodule("module", "module", source.working_tree_dir) + with submodule.config_writer() as writer: + writer.set_value("url", "../source") + previous = parent.index.commit("Record relative submodule URL") + with submodule.config_writer() as writer: + writer.set_value("url", Git.polish_url(source.working_tree_dir)) + parent.index.commit("Use existing remote URL") + + resolve_url = Submodule._resolved_url + + def resolve_current_url(sm): + assert sm.url != "../source", "Previous URL does not need resolving" + return resolve_url(sm) + + with mock.patch.object(Submodule, "_resolved_url", autospec=True, side_effect=resolve_current_url): + RootModule(parent).update(previous_commit=previous, no_fetch=True, dry_run=dry_run) + with submodule.module() as module: + assert module.remotes.origin.url == Git.polish_url(source.working_tree_dir) + assert module.head.commit == source.head.commit + + @pytest.mark.parametrize( "name", ["module", "$GITPYTHON_TEST_SECRET", "prefix-${GITPYTHON_TEST_SECRET}-suffix", "%GITPYTHON_TEST_SECRET%"] ) @@ -2437,6 +2569,9 @@ def test_resolve_relative_url_like_git(self): ("//share/super", "../../sub", "//sub"), ("foo/bar", "../sub", "foo/sub"), ("./foo", "../sub", "sub"), + ("./super.git", "../", ""), + ("ssh://host/org/super.git", "./", "ssh://host/org/super.git/"), + ("ssh://host/org/super.git", "../", "ssh://host/org/"), ("../foo/bar", "../sub", "../foo/sub"), ("ssh://host/org/super.git", ".sub", ".sub"), ("ssh://host/org/super.git", "https://other/sub", "https://other/sub"), @@ -2459,23 +2594,27 @@ def test_update_resolves_relative_url_against_remote(self, rwdir): ("../planted", "ssh://git@host/org/planted"), ]: submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url) - with mock.patch.object(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo: + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: with pytest.raises(AssertionError, match="stop"): submodule.update(init=True) - assert clone_repo.call_args.args[1] == expected + assert clone_from.call_args.args[0] == expected @with_rw_directory def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir): parent = git.Repo.init(osp.join(rwdir, "parent")) parent.index.commit("initial") - parent.create_remote("origin", "git@host:org/super.git") - parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/main")) - - for url, expected in [ - ("./planted", "git@host:org/super.git/planted"), - ("../planted", "git@host:org/planted"), - ]: - with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: - with pytest.raises(AssertionError, match="stop"): - Submodule._clone_repo(parent, url, "module", "module") - assert clone_from.call_args.args[0] == expected + parent.create_remote("origin", "git@host:other/super.git") + for remote_name in ("upstream", "team/upstream"): + parent.create_remote(remote_name, "git@host:org/super.git") + with parent.head.reference.config_writer() as writer: + writer.set_value("remote", remote_name) + writer.set_value("merge", "refs/heads/main") + + for url, expected in [ + ("./planted", "git@host:org/super.git/planted"), + ("../planted", "git@host:org/planted"), + ]: + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: + with pytest.raises(AssertionError, match="stop"): + Submodule._clone_repo(parent, url, "module", "module") + assert clone_from.call_args.args[0] == expected From 7582674e201a82ad519cf37c09c16a08e64ed49e Mon Sep 17 00:00:00 2001 From: Emmanuel Arias Date: Sat, 17 Oct 2026 15:37:39 -0300 Subject: [PATCH 19/25] Make de api reference reproducible The API reference uses `:special-members:`, so autodoc also documents the `__dict__` of the `TypedDict` classes in `git.types`. Its repr is a `mappingproxy` containing `frozenset` objects, whose iteration order depends on string hash randomization. As a result `reference.html` differs from one build to another. Exclude `__dict__` and `__weakref__` by default. They only hold the class namespace and carry no useful documentation. The `__required_keys__` and similar attributes are still documented, since Sphinx sorts sets when it renders them on their own. The original issue was reported https://bugs.debian.org/1100149 --- doc/source/conf.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/source/conf.py b/doc/source/conf.py index 809762483..edf488e72 100644 --- a/doc/source/conf.py +++ b/doc/source/conf.py @@ -111,6 +111,11 @@ # This file should be a Windows icon file (.ico) being 16x16 or 32x32 pixels large. # html_favicon = None +# Do not documento the attributes holidng a class's namespace. Their reprs embed ++# frozensets whose order depends on string hashing, which makes the generated ++# HTML vary between builds. ++autodoc_default_options = {"exclude-members": "__dict__, __weakref__"} + # Add any paths that contain custom static files (such as style sheets) here, relative # to this directory. They are copied after the builtin static files, so a file named # "default.css" will overwrite the builtin "default.css". From a3241fa7d48ea1a422d969d54101e813689014f1 Mon Sep 17 00:00:00 2001 From: Emmanuel Arias Date: Sat, 17 Oct 2026 15:48:30 -0300 Subject: [PATCH 20/25] Fix autodoc_default_options in conf.py Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- doc/source/conf.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/source/conf.py b/doc/source/conf.py index edf488e72..a8a5427ba 100644 --- a/doc/source/conf.py +++ b/doc/source/conf.py @@ -112,9 +112,9 @@ # html_favicon = None # Do not documento the attributes holidng a class's namespace. Their reprs embed -+# frozensets whose order depends on string hashing, which makes the generated -+# HTML vary between builds. -+autodoc_default_options = {"exclude-members": "__dict__, __weakref__"} +# frozensets whose order depends on string hashing, which makes the generated +# HTML vary between builds. +autodoc_default_options = {"exclude-members": "__dict__, __weakref__"} # Add any paths that contain custom static files (such as style sheets) here, relative # to this directory. They are copied after the builtin static files, so a file named From d8c0e7bdcd6bd2932c16c8c073833a9f88d78f3c Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Sat, 17 Oct 2026 16:35:36 +0530 Subject: [PATCH 21/25] fix(diff): read raw diff records field by field `Diff._handle_diff_line` parses `git diff --raw -z` output by splitting it at every NUL followed by a colon, and `Diff._index_from_raw_format` called it once per line of stdout. With `-z`, git writes paths verbatim, so a path may start with `:` or contain a line feed, and either one breaks the record framing: - A path starting with `:` is taken as the start of a new record. Its remainder is parsed as metadata, the resulting `ValueError` is raised inside the stdout pump thread, and the caller gets back a `DiffIndex` with a pathless entry followed by nothing: every later entry is dropped, and only a log message records the failure. For a commit adding `:planted`, `docs/b` and `setup.py`, `commit.diff(NULL_TREE)` returned a single entry with `a_path == None`. - A path containing a line feed is truncated at the line feed. If the rest of the path contains a colon, it is parsed as a separate record: `docs/a\n:100644 100644 111... 222... M` produced an entry `docs/a` plus a fabricated `M` entry whose blob ids came from the file name. Paths are chosen by whoever authored the commit. A tool that uses `Commit.diff` to learn which files a commit touches can therefore be made to miss changes, or to see changes that were never made. `_index_from_raw_format` now collects all of stdout before parsing, as `_index_from_patch_format` already does, and `_handle_diff_line` reads the NUL-separated fields in order: a metadata field, then one path, or two for copies and renames. A single trailing line feed is still stripped so the existing non-`-z` fixtures parse as before. The new test in `test/test_diff.py` fails before the change and passes after it. `ruff check`, `ruff format --check` and `mypy` are clean. In a full clone the suite has 6 failures (`test_diff_with_staged_file`, `test_index_mutation`, `test_empty_repo`, `test_rebasing` and the two `test_actor_get_uid_laziness_*` tests), and all 6 also fail without this change. --- git/diff.py | 33 +++++++++++++++++---------------- test/test_diff.py | 19 ++++++++++++++++++- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/git/diff.py b/git/diff.py index b28a29529..123c22840 100644 --- a/git/diff.py +++ b/git/diff.py @@ -733,12 +733,17 @@ def _handle_diff_line(lines_bytes: bytes, repo: "Repo", index: DiffIndex["Diff"] # Discard everything before the first colon, and the colon itself. _, _, lines = lines.partition(":") - for line in lines.split("\x00:"): - if not line: + # Each record is a metadata field starting with a colon, followed by one path, + # or two for copies and renames, each terminated by NUL. A path may itself + # start with a colon or contain a line feed, so read the fields in order + # instead of splitting at colons. + fields = iter(lines.rstrip("\n").split("\x00")) + for meta in fields: + if not meta: # The line data is empty, skip. continue - meta, _, path = line.partition("\x00") - path = path.rstrip("\x00") + if meta.startswith(":"): + meta = meta[1:] a_blob_id: Optional[str] b_blob_id: Optional[str] old_mode, new_mode, a_blob_id, b_blob_id, _change_type = meta.split(None, 4) @@ -748,7 +753,7 @@ def _handle_diff_line(lines_bytes: bytes, repo: "Repo", index: DiffIndex["Diff"] change_type: Lit_change_type = cast(Lit_change_type, _change_type[0]) score_str = "".join(_change_type[1:]) score = int(score_str) if score_str.isdigit() else None - path = path.strip("\n") + path = next(fields, "") a_path = path.encode(defenc) b_path = path.encode(defenc) deleted_file = False @@ -767,13 +772,9 @@ def _handle_diff_line(lines_bytes: bytes, repo: "Repo", index: DiffIndex["Diff"] new_file = True elif change_type == "C": copied_file = True - a_path_str, b_path_str = path.split("\x00", 1) - a_path = a_path_str.encode(defenc) - b_path = b_path_str.encode(defenc) + b_path = next(fields, "").encode(defenc) elif change_type == "R": - a_path_str, b_path_str = path.split("\x00", 1) - a_path = a_path_str.encode(defenc) - b_path = b_path_str.encode(defenc) + b_path = next(fields, "").encode(defenc) rename_from, rename_to = a_path, b_path elif change_type == "T": # Nothing to do. @@ -817,17 +818,17 @@ def _index_from_raw_format(cls, repo: "Repo", proc: "Popen") -> "DiffIndex[Diff] # :100644 100644 687099101... 37c5e30c8... M .gitignore index: "DiffIndex" = DiffIndex() + text_list: List[bytes] = [] stderr_list: List[bytes] = [] def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> None: finalize_process(proc, stderr=b"".join(stderr_list)) + # Output is read line by line, but paths may contain line feeds, so parse + # the records only once all of it has been read. handle_process_output( - proc, - lambda byt: cls._handle_diff_line(byt, repo, index), - stderr_list.append, - finalize_process_with_stderr, - decode_streams=False, + proc, text_list.append, stderr_list.append, finalize_process_with_stderr, decode_streams=False ) + cls._handle_diff_line(b"".join(text_list), repo, index) return index diff --git a/test/test_diff.py b/test/test_diff.py index bbaec72cc..06399b823 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -4,6 +4,7 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ import gc +from io import BytesIO import os.path as osp import shutil import sys @@ -12,10 +13,13 @@ import ddt import pytest -from git import NULL_TREE, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule +from gitdb.base import IStream + +from git import NULL_TREE, Blob, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule from git.cmd import Git from git.diff import decode_path from git.exc import UnsafeOptionError +from git.index.typ import BaseIndexEntry from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory @@ -253,6 +257,19 @@ def test_diff_file_with_colon(self): Diff._handle_diff_line(output, None, res) self.assertEqual(len(res), 3) + @with_rw_directory + def test_diff_raw_paths_with_colon_or_line_feed(self, rw_dir): + repo = Repo.init(rw_dir) + binsha = repo.odb.store(IStream(Blob.type, 1, BytesIO(b"x"))).binsha + fake_meta = ":100644 100644 %s %s M" % ("1" * 40, "2" * 40) + paths = [":planted", "docs/a\n" + fake_meta, "docs/b", "setup.py"] + repo.index.add([BaseIndexEntry((0o100644, binsha, 0, path)) for path in paths]) + commit = repo.index.commit("paths") + + diffs = commit.diff(NULL_TREE) + self.assertEqual([d.b_path for d in diffs], sorted(paths)) + self.assertEqual({d.change_type for d in diffs}, {"A"}) + def test_empty_diff(self): res = [] Diff._handle_diff_line(b"", None, res) From 2a176d9dadac1ba7ed56ac87985100c095251e30 Mon Sep 17 00:00:00 2001 From: Emmanuel Arias Date: Sat, 17 Oct 2026 19:20:12 -0300 Subject: [PATCH 22/25] Fix typo in documentation comment Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- doc/source/conf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/conf.py b/doc/source/conf.py index a8a5427ba..032e55cfe 100644 --- a/doc/source/conf.py +++ b/doc/source/conf.py @@ -111,7 +111,7 @@ # This file should be a Windows icon file (.ico) being 16x16 or 32x32 pixels large. # html_favicon = None -# Do not documento the attributes holidng a class's namespace. Their reprs embed +# Do not document the attributes holding a class's namespace. Their reprs embed # frozensets whose order depends on string hashing, which makes the generated # HTML vary between builds. autodoc_default_options = {"exclude-members": "__dict__, __weakref__"} From 53df5960072f75a3dff4d9ac0ed91ba053da2050 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 18 Oct 2026 14:14:28 +0200 Subject: [PATCH 23/25] review No changes, just notes: All this is probably beating a dead horse, but it can't be bad to have an additional test for this. Notably, the encoding/decoding portions of this are likely to fail as well. From ffefd4ffb380327687da706a389c1835c6907f06 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 18 Oct 2026 13:33:02 +0000 Subject: [PATCH 24/25] build(deps): bump astral-sh/setup-uv from 10.2.0 to 10.3.0 Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 10.2.0 to 10.3.0. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](https://github.com/astral-sh/setup-uv/compare/c18668ad3cf93ea998bef934396af7bb5c839dc7...1c37ad07a6a961277cf70c0d37d6f313000f5884) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: 10.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/dependencies.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/dependencies.yml b/.github/workflows/dependencies.yml index b4046dd9b..4eff39b3c 100644 --- a/.github/workflows/dependencies.yml +++ b/.github/workflows/dependencies.yml @@ -48,7 +48,7 @@ jobs: persist-credentials: false - name: Install uv - uses: astral-sh/setup-uv@c18668ad3cf93ea998bef934396af7bb5c839dc7 # v10.2.0 + uses: astral-sh/setup-uv@1c37ad07a6a961277cf70c0d37d6f313000f5884 # v10.3.0 with: enable-cache: false version: "0.12.13" # Released on 2026-09-10 From feda2411bb4d827093a35fdcc19ef70c52d91d92 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sun, 18 Oct 2026 13:33:08 +0000 Subject: [PATCH 25/25] build(deps): bump Vampire/setup-wsl from 7.0.0 to 7.2.0 Bumps [Vampire/setup-wsl](https://github.com/vampire/setup-wsl) from 7.0.0 to 7.2.0. - [Release notes](https://github.com/vampire/setup-wsl/releases) - [Commits](https://github.com/vampire/setup-wsl/compare/v7.0.0...v7.2.0) --- updated-dependencies: - dependency-name: Vampire/setup-wsl dependency-version: 7.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- .github/workflows/pythonpackage.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 3fbbe2ec6..94630dbfc 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -54,7 +54,7 @@ jobs: - name: Set up WSL (Windows) if: matrix.os-type == 'windows' - uses: Vampire/setup-wsl@v7.0.0 + uses: Vampire/setup-wsl@v7.2.0 with: wsl-version: 1