Skip to content

Commit d8c0e7b

Browse files
Keerthana-64Keerthana-64
committed
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.
1 parent 28ab8fb commit d8c0e7b

2 files changed

Lines changed: 35 additions & 17 deletions

File tree

‎git/diff.py‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -733,12 +733,17 @@ def _handle_diff_line(lines_bytes: bytes, repo: "Repo", index: DiffIndex["Diff"]
733733
# Discard everything before the first colon, and the colon itself.
734734
_, _, lines = lines.partition(":")
735735

736-
for line in lines.split("\x00:"):
737-
if not line:
736+
# Each record is a metadata field starting with a colon, followed by one path,
737+
# or two for copies and renames, each terminated by NUL. A path may itself
738+
# start with a colon or contain a line feed, so read the fields in order
739+
# instead of splitting at colons.
740+
fields = iter(lines.rstrip("\n").split("\x00"))
741+
for meta in fields:
742+
if not meta:
738743
# The line data is empty, skip.
739744
continue
740-
meta, _, path = line.partition("\x00")
741-
path = path.rstrip("\x00")
745+
if meta.startswith(":"):
746+
meta = meta[1:]
742747
a_blob_id: Optional[str]
743748
b_blob_id: Optional[str]
744749
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"]
748753
change_type: Lit_change_type = cast(Lit_change_type, _change_type[0])
749754
score_str = "".join(_change_type[1:])
750755
score = int(score_str) if score_str.isdigit() else None
751-
path = path.strip("\n")
756+
path = next(fields, "")
752757
a_path = path.encode(defenc)
753758
b_path = path.encode(defenc)
754759
deleted_file = False
@@ -767,13 +772,9 @@ def _handle_diff_line(lines_bytes: bytes, repo: "Repo", index: DiffIndex["Diff"]
767772
new_file = True
768773
elif change_type == "C":
769774
copied_file = True
770-
a_path_str, b_path_str = path.split("\x00", 1)
771-
a_path = a_path_str.encode(defenc)
772-
b_path = b_path_str.encode(defenc)
775+
b_path = next(fields, "").encode(defenc)
773776
elif change_type == "R":
774-
a_path_str, b_path_str = path.split("\x00", 1)
775-
a_path = a_path_str.encode(defenc)
776-
b_path = b_path_str.encode(defenc)
777+
b_path = next(fields, "").encode(defenc)
777778
rename_from, rename_to = a_path, b_path
778779
elif change_type == "T":
779780
# Nothing to do.
@@ -817,17 +818,17 @@ def _index_from_raw_format(cls, repo: "Repo", proc: "Popen") -> "DiffIndex[Diff]
817818
# :100644 100644 687099101... 37c5e30c8... M .gitignore
818819

819820
index: "DiffIndex" = DiffIndex()
821+
text_list: List[bytes] = []
820822
stderr_list: List[bytes] = []
821823

822824
def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> None:
823825
finalize_process(proc, stderr=b"".join(stderr_list))
824826

827+
# Output is read line by line, but paths may contain line feeds, so parse
828+
# the records only once all of it has been read.
825829
handle_process_output(
826-
proc,
827-
lambda byt: cls._handle_diff_line(byt, repo, index),
828-
stderr_list.append,
829-
finalize_process_with_stderr,
830-
decode_streams=False,
830+
proc, text_list.append, stderr_list.append, finalize_process_with_stderr, decode_streams=False
831831
)
832+
cls._handle_diff_line(b"".join(text_list), repo, index)
832833

833834
return index

‎test/test_diff.py‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

66
import gc
7+
from io import BytesIO
78
import os.path as osp
89
import shutil
910
import sys
@@ -12,10 +13,13 @@
1213
import ddt
1314
import pytest
1415

15-
from git import NULL_TREE, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
16+
from gitdb.base import IStream
17+
18+
from git import NULL_TREE, Blob, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
1619
from git.cmd import Git
1720
from git.diff import decode_path
1821
from git.exc import UnsafeOptionError
22+
from git.index.typ import BaseIndexEntry
1923

2024
from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory
2125

@@ -253,6 +257,19 @@ def test_diff_file_with_colon(self):
253257
Diff._handle_diff_line(output, None, res)
254258
self.assertEqual(len(res), 3)
255259

260+
@with_rw_directory
261+
def test_diff_raw_paths_with_colon_or_line_feed(self, rw_dir):
262+
repo = Repo.init(rw_dir)
263+
binsha = repo.odb.store(IStream(Blob.type, 1, BytesIO(b"x"))).binsha
264+
fake_meta = ":100644 100644 %s %s M" % ("1" * 40, "2" * 40)
265+
paths = [":planted", "docs/a\n" + fake_meta, "docs/b", "setup.py"]
266+
repo.index.add([BaseIndexEntry((0o100644, binsha, 0, path)) for path in paths])
267+
commit = repo.index.commit("paths")
268+
269+
diffs = commit.diff(NULL_TREE)
270+
self.assertEqual([d.b_path for d in diffs], sorted(paths))
271+
self.assertEqual({d.change_type for d in diffs}, {"A"})
272+
256273
def test_empty_diff(self):
257274
res = []
258275
Diff._handle_diff_line(b"", None, res)

0 commit comments

Comments
 (0)