Skip to content

fix(diff): read raw diff records field by field - #2292

Merged
Byron merged 2 commits into
gitpython-developers:mainfrom
Keerthana-64:diff-raw-record-fields
Oct 18, 2026
Merged

Byron merged 2 commits into
gitpython-developers:mainfrom
Keerthana-64:diff-raw-record-fields

Conversation

@Keerthana-64

Copy link
Copy Markdown
Contributor

Diff._handle_diff_line parsed git diff --raw -z output by splitting it at every NUL followed by a colon, and Diff._index_from_raw_format fed it one line of stdout at a time. With -z git writes paths verbatim, so both delimiters can occur inside a path. A path starting with : is parsed as metadata, the ValueError is raised inside the stdout pump thread, and the caller still gets a DiffIndex, with a pathless entry and every later entry missing: for a commit adding :planted, docs/b and setup.py, commit.diff(NULL_TREE) returned only [(None, 'A')]. A path containing a line feed is truncated there, and a colon after it starts a fabricated record: docs/a\n:100644 100644 111... 222... M came back as docs/a plus an M entry with blob ids taken from the file name. Paths are chosen by the commit author, so a tool that uses Commit.diff to see which files a commit touches can be made to miss changes.

_index_from_raw_format now collects stdout before parsing, as _index_from_patch_format already does, and _handle_diff_line reads the NUL-separated fields in order: one metadata field, then one path, or two for copies and renames. A 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.

Keerthana-64 and others added 2 commits October 18, 2026 00:52
`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.
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.
@Byron

Byron commented Oct 18, 2026

Copy link
Copy Markdown
Member

Merging, as the tree didn't change (review was just my thoughts on this).

@Byron
Byron merged commit f6ac084 into gitpython-developers:main Oct 18, 2026
40 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants