FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(diff): read patch header paths the way git apply does · gitpython-developers/GitPython@c7ce984 · GitHub

Repository navigation

Commit c7ce984

Browse files
committed
fix(diff): read patch header paths the way git apply does
`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.
1 parent 9d3aff0 commit c7ce984

2 files changed

Lines changed: 83 additions & 4 deletions

File tree

‎git/diff.py‎

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,8 @@ class Diff:
410410
re_header = re.compile(
411411
rb"""
412412
^diff[ ]--git
413-
[ ](?P<a_path_fallback>"?[ab]/.+?"?)[ ](?P<b_path_fallback>"?[ab]/.+?"?)\n
413+
[ ](?P<header_names>(?P<a_path_fallback>"?[ab]/.+?"?)
414+
[ ](?P<b_path_fallback>"?[ab]/.+?"?))\n
414415
(?:^old[ ]mode[ ](?P<old_mode>\d+)\n
415416
^new[ ]mode[ ](?P<new_mode>\d+)(?:\n|$))?
416417
(?:^similarity[ ]index[ ]\d+%\n
@@ -419,7 +420,7 @@ class Diff:
419420
(?:^new[ ]file[ ]mode[ ](?P<new_file_mode>.+)(?:\n|$))?
420421
(?:^deleted[ ]file[ ]mode[ ](?P<deleted_file_mode>.+)(?:\n|$))?
421422
(?:^similarity[ ]index[ ]\d+%\n
422-
^copy[ ]from[ ].*\n
423+
^copy[ ]from[ ](?P<copied_from_name>.*)\n
423424
^copy[ ]to[ ](?P<copied_file_name>.*)(?:\n|$))?
424425
(?:^index[ ](?P<a_blob_id>[0-9A-Fa-f]+)
425426
\.\.(?P<b_blob_id>[0-9A-Fa-f]+)[ ]?(?P<b_mode>.+)?(?:\n|$))?
@@ -626,6 +627,39 @@ def _pick_best_path(cls, path_match: bytes, rename_match: bytes, path_fallback_m
626627

627628
return None
628629

630+
@staticmethod
631+
def _same_path_in_header(names: bytes) -> Optional[bytes]:
632+
"""Return the path that a ``diff --git a/<path> b/<path>`` line names twice.
633+
634+
git separates the two names with a space but does not quote a path for
635+
containing one, so once the path contains ``" a/"`` or ``" b/"`` the line alone
636+
does not show where the first name ends. When no ``---``, ``+++``, rename or
637+
copy line names the file, both names are the same path, and the split is
638+
accepted only where the two halves agree, as ``git_header_name`` in git's
639+
``apply.c`` does. A quoted first name ends at its closing quote.
640+
641+
:return:
642+
The path without its ``a/`` prefix, or ``None`` if no split agrees.
643+
"""
644+
if names.startswith(b'"'):
645+
end = 1
646+
while end < len(names) and names[end : end + 1] != b'"':
647+
end += 2 if names[end : end + 1] == b"\\" else 1
648+
splits = [end + 1]
649+
else:
650+
splits = [index for index in range(len(names)) if names[index : index + 1] == b" "]
651+
for split in splits:
652+
if names[split : split + 1] != b" ":
653+
continue
654+
a_name, b_name = names[:split], names[split + 1 :]
655+
if a_name.startswith(b'"'):
656+
if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')):
657+
continue
658+
a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1])
659+
if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]:
660+
return a_name[2:]
661+
return None
662+
629663
@classmethod
630664
def _index_from_patch_format(cls, repo: "Repo", proc: Union["Popen", "Git.AutoInterrupt"]) -> DiffIndex["Diff"]:
631665
"""Create a new :class:`DiffIndex` from the given process output which must be
@@ -662,6 +696,7 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N
662696
a_mode, b_mode = None, None # For mypy.
663697
for _header in cls.re_header.finditer(text):
664698
(
699+
header_names,
665700
a_path_fallback,
666701
b_path_fallback,
667702
old_mode,
@@ -670,6 +705,7 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N
670705
rename_to,
671706
new_file_mode,
672707
deleted_file_mode,
708+
copied_from_name,
673709
copied_file_name,
674710
a_blob_id,
675711
b_blob_id,
@@ -684,8 +720,15 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N
684720
bool(copied_file_name),
685721
)
686722

687-
a_path = cls._pick_best_path(a_path, rename_from, a_path_fallback)
688-
b_path = cls._pick_best_path(b_path, rename_to, b_path_fallback)
723+
# Mode changes, binary files and empty files have no `---` and `+++` lines,
724+
# so only the `diff --git` line names them, and it names the same path twice.
725+
named_elsewhere = any((a_path, b_path, rename_from, rename_to, copied_from_name, copied_file_name))
726+
same_path = None if named_elsewhere else cls._same_path_in_header(header_names)
727+
if same_path is None:
728+
a_path = cls._pick_best_path(a_path, rename_from or copied_from_name, a_path_fallback)
729+
b_path = cls._pick_best_path(b_path, rename_to or copied_file_name, b_path_fallback)
730+
else:
731+
a_path = b_path = same_path
689732

690733
# Our only means to find the actual text is to see what has not been matched
691734
# by our regex, and then retro-actively assign it to our index.

‎test/test_diff.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,42 @@ def test_diff_raw_paths_with_colon_or_line_feed(self, rw_dir):
270270
self.assertEqual([d.b_path for d in diffs], sorted(paths))
271271
self.assertEqual({d.change_type for d in diffs}, {"A"})
272272

273+
@with_rw_directory
274+
def test_diff_patch_paths_named_only_by_the_header_line(self, rw_dir):
275+
# Binary files, empty files, mode changes and copies have no `---` and `+++`
276+
# lines, so their paths come from the `diff --git a/<path> b/<path>` line, where
277+
# git does not quote a space. Each path contains " a/" or " b/" so that the line
278+
# alone does not show where the first name ends.
279+
repo = Repo.init(rw_dir)
280+
binary = repo.odb.store(IStream(Blob.type, 1, BytesIO(b"\0"))).binsha
281+
empty = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha
282+
paths = ["assets b/logo.png", "docs a/empty", "quoted\t a/tab"]
283+
repo.index.add(
284+
[
285+
BaseIndexEntry((0o100644, binary, 0, paths[0])),
286+
BaseIndexEntry((0o100644, empty, 0, paths[1])),
287+
BaseIndexEntry((0o100644, binary, 0, paths[2])),
288+
]
289+
)
290+
commit = repo.index.commit("paths")
291+
292+
diffs = commit.diff(NULL_TREE, create_patch=True)
293+
self.assertEqual({(d.a_path, d.b_path) for d in diffs}, {(path, path) for path in paths})
294+
295+
repo.index.add(
296+
[
297+
BaseIndexEntry((0o100755, binary, 0, paths[0])),
298+
BaseIndexEntry((0o100644, binary, 0, "copy b/logo.png")),
299+
]
300+
)
301+
changed = repo.index.commit("mode change and copy")
302+
303+
diffs = commit.diff(changed, create_patch=True, C=True, find_copies_harder=True)
304+
self.assertEqual(
305+
{(d.a_path, d.b_path, d.copied_file) for d in diffs},
306+
{(paths[0], paths[0], False), (paths[0], "copy b/logo.png", True)},
307+
)
308+
273309
def test_empty_diff(self):
274310
res = []
275311
Diff._handle_diff_line(b"", None, res)

0 commit comments

Comments
 (0)

Back | FazBrowse Home | New Git URL