From d8c0e7bdcd6bd2932c16c8c073833a9f88d78f3c Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Sat, 17 Oct 2026 16:35:36 +0530 Subject: [PATCH 1/2] 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 53df5960072f75a3dff4d9ac0ed91ba053da2050 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 18 Oct 2026 14:14:28 +0200 Subject: [PATCH 2/2] 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.