Skip to content

fix(diff): read patch header paths the way git apply does - #2295

Open
Keerthana-64 wants to merge 1 commit into
gitpython-developers:mainfrom
Keerthana-64:diff-patch-header-paths
Open

Keerthana-64 wants to merge 1 commit into
gitpython-developers:mainfrom
Keerthana-64:diff-patch-header-paths

Conversation

@Keerthana-64

Copy link
Copy Markdown
Contributor

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, and it did the same for both names of a copy because the copy from line was not captured. 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 was reported with a_path == "assets" and b_path == "logo.png b/assets b/logo.png", a copy of assets b/logo.png to copy b/logo.png came back as assets and logo.png b/copy b/logo.png, and for a quoted name such as "a/quoted\t a/tab" the cut inside the quotes made decode_path fail its prefix assertion, so diff(create_patch=True) raised AssertionError. Paths are chosen by whoever authored the commit, so 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.

The fix captures the copy from name 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 the diff --git line are the same path, and the new _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.

`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.
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.

1 participant