From c7ce9843247c5efa788830060921afb1435a3d00 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Mon, 19 Oct 2026 01:07:28 +0530 Subject: [PATCH 1/2] fix(diff): read patch header paths the way git apply does `Diff._index_from_patch_format` takes a file's paths from the `diff --git a/ b/` line whenever a patch header has no `---` and `+++` lines, which is the case for binary files, empty files and mode-only changes. It also did so for both names of a copy, because the `copy from` line was not captured and the `copy to` line only set `copied_file`. `Diff.re_header` splits that line at the first space followed by `a/` or `b/`, but git does not quote a path for containing a space, so a path containing ` a/` or ` b/` is split in the wrong place: - A binary file `assets b/logo.png` is reported with `a_path` `assets` and `b_path` `logo.png b/assets b/logo.png`; an empty file `docs a/empty` comes back as `docs` and `empty b/docs a/empty`. - A copy of `assets b/logo.png` to `copy b/logo.png` comes back as `assets` and `logo.png b/copy b/logo.png`. - For a quoted name such as `"a/quoted\t a/tab"`, the first name is cut at the space inside the quotes, `decode_path` fails its prefix assertion, and the whole `diff(create_patch=True)` call raises `AssertionError`. Paths are chosen by whoever authored the commit. A tool that uses `Commit.diff(create_patch=True)` to learn which files a commit touches is shown an `a_path` prefix of the author's choosing, does not see the real path as `b_path`, or gets no diff at all. `re_header` now captures the `copy from` name, and the copy lines are used for the paths the way the rename lines already are. When no `---`, `+++`, rename or copy line names the file, both names on the `diff --git` line are the same path, and `_same_path_in_header` splits the line the way `git_header_name` in git's `apply.c` does: at the end of a quoted first name, or at the space where the two halves agree. A line that does not split that way keeps the previous result, so the existing 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 3 failures (`test_diff_with_staged_file`, `test_empty_repo` and `test_rebasing`), and all 3 also fail without this change. --- git/diff.py | 51 +++++++++++++++++++++++++++++++++++++++++++---- test/test_diff.py | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/git/diff.py b/git/diff.py index 123c22840..85a868c79 100644 --- a/git/diff.py +++ b/git/diff.py @@ -410,7 +410,8 @@ class Diff: re_header = re.compile( rb""" ^diff[ ]--git - [ ](?P"?[ab]/.+?"?)[ ](?P"?[ab]/.+?"?)\n + [ ](?P(?P"?[ab]/.+?"?) + [ ](?P"?[ab]/.+?"?))\n (?:^old[ ]mode[ ](?P\d+)\n ^new[ ]mode[ ](?P\d+)(?:\n|$))? (?:^similarity[ ]index[ ]\d+%\n @@ -419,7 +420,7 @@ class Diff: (?:^new[ ]file[ ]mode[ ](?P.+)(?:\n|$))? (?:^deleted[ ]file[ ]mode[ ](?P.+)(?:\n|$))? (?:^similarity[ ]index[ ]\d+%\n - ^copy[ ]from[ ].*\n + ^copy[ ]from[ ](?P.*)\n ^copy[ ]to[ ](?P.*)(?:\n|$))? (?:^index[ ](?P[0-9A-Fa-f]+) \.\.(?P[0-9A-Fa-f]+)[ ]?(?P.+)?(?:\n|$))? @@ -626,6 +627,39 @@ def _pick_best_path(cls, path_match: bytes, rename_match: bytes, path_fallback_m return None + @staticmethod + def _same_path_in_header(names: bytes) -> Optional[bytes]: + """Return the path that a ``diff --git a/ b/`` line names twice. + + git separates the two names with a space but does not quote a path for + containing one, so once the path contains ``" a/"`` or ``" b/"`` the line alone + does not show where the first name ends. When no ``---``, ``+++``, rename or + copy line names the file, both names are the same path, and the split is + accepted only where the two halves agree, as ``git_header_name`` in git's + ``apply.c`` does. A quoted first name ends at its closing quote. + + :return: + The path without its ``a/`` prefix, or ``None`` if no split agrees. + """ + if names.startswith(b'"'): + end = 1 + while end < len(names) and names[end : end + 1] != b'"': + end += 2 if names[end : end + 1] == b"\\" else 1 + splits = [end + 1] + else: + splits = [index for index in range(len(names)) if names[index : index + 1] == b" "] + for split in splits: + if names[split : split + 1] != b" ": + continue + a_name, b_name = names[:split], names[split + 1 :] + if a_name.startswith(b'"'): + if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')): + continue + a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1]) + if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]: + return a_name[2:] + return None + @classmethod def _index_from_patch_format(cls, repo: "Repo", proc: Union["Popen", "Git.AutoInterrupt"]) -> DiffIndex["Diff"]: """Create a new :class:`DiffIndex` from the given process output which must be @@ -662,6 +696,7 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N a_mode, b_mode = None, None # For mypy. for _header in cls.re_header.finditer(text): ( + header_names, a_path_fallback, b_path_fallback, old_mode, @@ -670,6 +705,7 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N rename_to, new_file_mode, deleted_file_mode, + copied_from_name, copied_file_name, a_blob_id, b_blob_id, @@ -684,8 +720,15 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N bool(copied_file_name), ) - a_path = cls._pick_best_path(a_path, rename_from, a_path_fallback) - b_path = cls._pick_best_path(b_path, rename_to, b_path_fallback) + # Mode changes, binary files and empty files have no `---` and `+++` lines, + # so only the `diff --git` line names them, and it names the same path twice. + named_elsewhere = any((a_path, b_path, rename_from, rename_to, copied_from_name, copied_file_name)) + same_path = None if named_elsewhere else cls._same_path_in_header(header_names) + if same_path is None: + a_path = cls._pick_best_path(a_path, rename_from or copied_from_name, a_path_fallback) + b_path = cls._pick_best_path(b_path, rename_to or copied_file_name, b_path_fallback) + else: + a_path = b_path = same_path # Our only means to find the actual text is to see what has not been matched # by our regex, and then retro-actively assign it to our index. diff --git a/test/test_diff.py b/test/test_diff.py index 06399b823..f5f0ac7c3 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -270,6 +270,42 @@ def test_diff_raw_paths_with_colon_or_line_feed(self, rw_dir): self.assertEqual([d.b_path for d in diffs], sorted(paths)) self.assertEqual({d.change_type for d in diffs}, {"A"}) + @with_rw_directory + def test_diff_patch_paths_named_only_by_the_header_line(self, rw_dir): + # Binary files, empty files, mode changes and copies have no `---` and `+++` + # lines, so their paths come from the `diff --git a/ b/` line, where + # git does not quote a space. Each path contains " a/" or " b/" so that the line + # alone does not show where the first name ends. + repo = Repo.init(rw_dir) + binary = repo.odb.store(IStream(Blob.type, 1, BytesIO(b"\0"))).binsha + empty = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha + paths = ["assets b/logo.png", "docs a/empty", "quoted\t a/tab"] + repo.index.add( + [ + BaseIndexEntry((0o100644, binary, 0, paths[0])), + BaseIndexEntry((0o100644, empty, 0, paths[1])), + BaseIndexEntry((0o100644, binary, 0, paths[2])), + ] + ) + commit = repo.index.commit("paths") + + diffs = commit.diff(NULL_TREE, create_patch=True) + self.assertEqual({(d.a_path, d.b_path) for d in diffs}, {(path, path) for path in paths}) + + repo.index.add( + [ + BaseIndexEntry((0o100755, binary, 0, paths[0])), + BaseIndexEntry((0o100644, binary, 0, "copy b/logo.png")), + ] + ) + changed = repo.index.commit("mode change and copy") + + diffs = commit.diff(changed, create_patch=True, C=True, find_copies_harder=True) + self.assertEqual( + {(d.a_path, d.b_path, d.copied_file) for d in diffs}, + {(paths[0], paths[0], False), (paths[0], "copy b/logo.png", True)}, + ) + def test_empty_diff(self): res = [] Diff._handle_diff_line(b"", None, res) From de50b145e3e54e11882cfb938c14303ad0379a90 Mon Sep 17 00:00:00 2001 From: Byron Date: Wed, 21 Oct 2026 13:46:29 +0200 Subject: [PATCH 2/2] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P2] Avoid copying the header at every possible separator — /Users/byron/dev/github.com/gitpython-developers/GitPython.diff-patch-header-paths/git/diff.py:650-654 For a long, space-heavy filename, this loop copies the header once per space, making parsing quadratic in path length. A Git-generated mode-only diff for the filename `b"x " * 250000` took about 3 seconds through `Tree.diff(..., create_patch=True)`, versus 0.023 seconds on the parent. Git accepts such tree entries without checkout, so larger commit-controlled paths can stall callers. Reject impossible split lengths before allocating slices, or check only the equal-length midpoint. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6.0 Astra --- git/diff.py | 46 +++++++++++++++++++------------------- test/test_diff.py | 56 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 26 deletions(-) diff --git a/git/diff.py b/git/diff.py index 85a868c79..48ed863f4 100644 --- a/git/diff.py +++ b/git/diff.py @@ -9,16 +9,9 @@ import re import warnings -from git.cmd import Git, handle_process_output -from git.compat import defenc -from git.objects.base import IndexObject -from git.objects.blob import Blob -from git.objects.util import mode_str_to_int -from git.util import finalize_process, hex_to_bin - # typing ------------------------------------------------------------------ - from typing import ( + TYPE_CHECKING, Any, Iterator, List, @@ -26,12 +19,18 @@ Optional, Sequence, Tuple, - TYPE_CHECKING, TypeVar, Union, cast, ) -from git.types import PathLike, Literal + +from git.cmd import Git, handle_process_output +from git.compat import defenc +from git.objects.base import IndexObject +from git.objects.blob import Blob +from git.objects.util import mode_str_to_int +from git.types import Literal, PathLike +from git.util import finalize_process, hex_to_bin if TYPE_CHECKING: from subprocess import Popen @@ -645,19 +644,22 @@ def _same_path_in_header(names: bytes) -> Optional[bytes]: end = 1 while end < len(names) and names[end : end + 1] != b'"': end += 2 if names[end : end + 1] == b"\\" else 1 - splits = [end + 1] + split = end + 1 else: - splits = [index for index in range(len(names)) if names[index : index + 1] == b" "] - for split in splits: - if names[split : split + 1] != b" ": - continue - a_name, b_name = names[:split], names[split + 1 :] - if a_name.startswith(b'"'): - if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')): - continue - a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1]) - if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]: - return a_name[2:] + # Equal unquoted paths have equal-length names, so only the midpoint + # can separate them. Trying every space would copy the header quadratically. + if len(names) % 2 == 0: + return None + split = len(names) // 2 + if names[split : split + 1] != b" ": + return None + a_name, b_name = names[:split], names[split + 1 :] + if a_name.startswith(b'"'): + if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')): + return None + a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1]) + if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]: + return a_name[2:] return None @classmethod diff --git a/test/test_diff.py b/test/test_diff.py index f5f0ac7c3..07e2011d7 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -4,23 +4,21 @@ # 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 import tempfile +from io import BytesIO import ddt import pytest - from gitdb.base import IStream -from git import NULL_TREE, Blob, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule +from git import NULL_TREE, Blob, Diff, Diffable, DiffIndex, 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 @@ -306,6 +304,56 @@ def test_diff_patch_paths_named_only_by_the_header_line(self, rw_dir): {(paths[0], paths[0], False), (paths[0], "copy b/logo.png", True)}, ) + @ddt.data( + (b"a/x b/x", b"x"), + (b"a/x b/x ", b"x "), + (b"a/x b/y", None), + (b"a/x b/xx", None), + (b"a/x-b/x", None), + (b"", None), + (b'"a/x\\t" "b/x\\t"', b"x\t"), + (b'"a/x\\t" "b/x\\011"', b"x\t"), + (b'"a/x\\t" "b/y\\t"', None), + (b'"a/x\\t" "b/x\\t', None), + ) + def test_same_path_in_header(self, case): + names, expected = case + self.assertEqual(Diff._same_path_in_header(names), expected) + + def test_same_path_in_header_copies_linear_bytes(self): + class SliceCountingBytes(bytes): + sliced_bytes = 0 + + def __getitem__(self, key): + result = super().__getitem__(key) + if isinstance(key, slice): + self.sliced_bytes += len(result) + return result + + path = b"x " * 1000 + names = SliceCountingBytes(b"a/" + path + b" b/" + path) + self.assertEqual(Diff._same_path_in_header(names), path) + # Count copied bytes rather than asserting a machine-dependent time limit. + self.assertLessEqual(names.sliced_bytes, 2 * len(names)) + + @with_rw_directory + def test_diff_patch_mode_only_with_long_space_heavy_path(self, rw_dir): + repo = Repo.init(rw_dir) + path = b"x " * 250000 + blob = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha + trees = [] + # Store trees directly: Git accepts paths longer than filesystem or index limits. + for mode in (b"100644", b"100755"): + data = mode + b" " + path + b"\0" + blob + tree = repo.odb.store(IStream(b"tree", len(data), BytesIO(data))) + trees.append(repo.tree(tree.binsha.hex())) + + diffs = trees[0].diff(trees[1], create_patch=True) + self.assertEqual(len(diffs), 1) + self.assertEqual((diffs[0].a_rawpath, diffs[0].b_rawpath), (path, path)) + self.assertEqual((diffs[0].a_mode, diffs[0].b_mode), (0o100644, 0o100755)) + self.assertEqual(diffs[0].diff, b"") + def test_empty_diff(self): res = [] Diff._handle_diff_line(b"", None, res)