Repository navigation
fix(diff): read patch header paths the way git apply does - #2295
Open
Keerthana-64 wants to merge 1 commit into
Open
Keerthana-64 wants to merge 1 commit into
Keerthana-64 wants to merge 1 commit into
Conversation
`Diff._index_from_patch_format` takes a file's paths from the `diff --git a/<path> b/<path>` 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.
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._index_from_patch_formattakes a file's paths from thediff --git a/<path> b/<path>line whenever a patch header has no---and+++lines, which is the case for binary files, empty files and mode-only changes, and it did the same for both names of a copy because thecopy fromline was not captured.Diff.re_headersplits that line at the first space followed bya/orb/, but git does not quote a path for containing a space, so a path containinga/orb/is split in the wrong place. A binary fileassets b/logo.pngwas reported witha_path == "assets"andb_path == "logo.png b/assets b/logo.png", a copy ofassets b/logo.pngtocopy b/logo.pngcame back asassetsandlogo.png b/copy b/logo.png, and for a quoted name such as"a/quoted\t a/tab"the cut inside the quotes madedecode_pathfail its prefix assertion, sodiff(create_patch=True)raisedAssertionError. Paths are chosen by whoever authored the commit, so a tool that usesCommit.diff(create_patch=True)to learn which files a commit touches is shown ana_pathprefix of the author's choosing, does not see the real path asb_path, or gets no diff at all.The fix captures the
copy fromname and uses the copy lines for the paths the way the rename lines already are. When no---,+++, rename or copy line names the file, both names on thediff --gitline are the same path, and the new_same_path_in_headersplits the line the waygit_header_namein git'sapply.cdoes: 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 intest/test_diff.pyfails before the change and passes after it.ruff check,ruff format --checkandmypyare clean. In a full clone the suite has 3 failures (test_diff_with_staged_file,test_empty_repoandtest_rebasing), and all 3 also fail without this change.