Repository navigation
fix(diff): read raw diff records field by field - #2292
Merged
Byron merged 2 commits intoOct 18, 2026
Merged
Conversation
`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.
Member
|
Merging, as the tree didn't change (review was just my thoughts on this). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Diff._handle_diff_lineparsedgit diff --raw -zoutput by splitting it at every NUL followed by a colon, andDiff._index_from_raw_formatfed it one line of stdout at a time. With-zgit writes paths verbatim, so both delimiters can occur inside a path. A path starting with:is parsed as metadata, theValueErroris raised inside the stdout pump thread, and the caller still gets aDiffIndex, with a pathless entry and every later entry missing: for a commit adding:planted,docs/bandsetup.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... Mcame back asdocs/aplus anMentry with blob ids taken from the file name. Paths are chosen by the commit author, so a tool that usesCommit.diffto see which files a commit touches can be made to miss changes._index_from_raw_formatnow collects stdout before parsing, as_index_from_patch_formatalready does, and_handle_diff_linereads 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--zfixtures parse as before. The new test intest/test_diff.pyfails before the change and passes after it.ruff check,ruff format --checkandmypyare clean. In a full clone the suite has 6 failures (test_diff_with_staged_file,test_index_mutation,test_empty_repo,test_rebasingand the twotest_actor_get_uid_laziness_*tests), and all 6 also fail without this change.