From c7ce9843247c5efa788830060921afb1435a3d00 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Mon, 19 Oct 2026 01:07:28 +0530 Subject: [PATCH] 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/ b/` 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. --- git/diff.py | 51 +++++++++++++++++++++++++++++++++++++++++++---- test/test_diff.py | 36 +++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/git/diff.py b/git/diff.py index 123c22840..85a868c79 100644 --- a/git/diff.py +++ b/git/diff.py @@ -410,7 +410,8 @@ class Diff: re_header = re.compile( rb""" ^diff[ ]--git - [ ](?P"?[ab]/.+?"?)[ ](?P"?[ab]/.+?"?)\n + [ ](?P(?P"?[ab]/.+?"?) + [ ](?P"?[ab]/.+?"?))\n (?:^old[ ]mode[ ](?P\d+)\n ^new[ ]mode[ ](?P\d+)(?:\n|$))? (?:^similarity[ ]index[ ]\d+%\n @@ -419,7 +420,7 @@ class Diff: (?:^new[ ]file[ ]mode[ ](?P.+)(?:\n|$))? (?:^deleted[ ]file[ ]mode[ ](?P.+)(?:\n|$))? (?:^similarity[ ]index[ ]\d+%\n - ^copy[ ]from[ ].*\n + ^copy[ ]from[ ](?P.*)\n ^copy[ ]to[ ](?P.*)(?:\n|$))? (?:^index[ ](?P[0-9A-Fa-f]+) \.\.(?P[0-9A-Fa-f]+)[ ]?(?P.+)?(?:\n|$))? @@ -626,6 +627,39 @@ def _pick_best_path(cls, path_match: bytes, rename_match: bytes, path_fallback_m return None + @staticmethod + def _same_path_in_header(names: bytes) -> Optional[bytes]: + """Return the path that a ``diff --git a/ b/`` line names twice. + + git separates the two names with a space but does not quote a path for + containing one, so once the path contains ``" a/"`` or ``" b/"`` the line alone + does not show where the first name ends. When no ``---``, ``+++``, rename or + copy line names the file, both names are the same path, and the split is + accepted only where the two halves agree, as ``git_header_name`` in git's + ``apply.c`` does. A quoted first name ends at its closing quote. + + :return: + The path without its ``a/`` prefix, or ``None`` if no split agrees. + """ + if names.startswith(b'"'): + end = 1 + while end < len(names) and names[end : end + 1] != b'"': + end += 2 if names[end : end + 1] == b"\\" else 1 + splits = [end + 1] + else: + splits = [index for index in range(len(names)) if names[index : index + 1] == b" "] + for split in splits: + if names[split : split + 1] != b" ": + continue + a_name, b_name = names[:split], names[split + 1 :] + if a_name.startswith(b'"'): + if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')): + continue + a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1]) + if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]: + return a_name[2:] + return None + @classmethod def _index_from_patch_format(cls, repo: "Repo", proc: Union["Popen", "Git.AutoInterrupt"]) -> DiffIndex["Diff"]: """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 a_mode, b_mode = None, None # For mypy. for _header in cls.re_header.finditer(text): ( + header_names, a_path_fallback, b_path_fallback, old_mode, @@ -670,6 +705,7 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N rename_to, new_file_mode, deleted_file_mode, + copied_from_name, copied_file_name, a_blob_id, b_blob_id, @@ -684,8 +720,15 @@ def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> N bool(copied_file_name), ) - a_path = cls._pick_best_path(a_path, rename_from, a_path_fallback) - b_path = cls._pick_best_path(b_path, rename_to, b_path_fallback) + # Mode changes, binary files and empty files have no `---` and `+++` lines, + # so only the `diff --git` line names them, and it names the same path twice. + named_elsewhere = any((a_path, b_path, rename_from, rename_to, copied_from_name, copied_file_name)) + same_path = None if named_elsewhere else cls._same_path_in_header(header_names) + if same_path is None: + a_path = cls._pick_best_path(a_path, rename_from or copied_from_name, a_path_fallback) + b_path = cls._pick_best_path(b_path, rename_to or copied_file_name, b_path_fallback) + else: + a_path = b_path = same_path # Our only means to find the actual text is to see what has not been matched # by our regex, and then retro-actively assign it to our index. diff --git a/test/test_diff.py b/test/test_diff.py index 06399b823..f5f0ac7c3 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -270,6 +270,42 @@ def test_diff_raw_paths_with_colon_or_line_feed(self, rw_dir): self.assertEqual([d.b_path for d in diffs], sorted(paths)) self.assertEqual({d.change_type for d in diffs}, {"A"}) + @with_rw_directory + def test_diff_patch_paths_named_only_by_the_header_line(self, rw_dir): + # Binary files, empty files, mode changes and copies have no `---` and `+++` + # lines, so their paths come from the `diff --git a/ b/` line, where + # git does not quote a space. Each path contains " a/" or " b/" so that the line + # alone does not show where the first name ends. + repo = Repo.init(rw_dir) + binary = repo.odb.store(IStream(Blob.type, 1, BytesIO(b"\0"))).binsha + empty = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha + paths = ["assets b/logo.png", "docs a/empty", "quoted\t a/tab"] + repo.index.add( + [ + BaseIndexEntry((0o100644, binary, 0, paths[0])), + BaseIndexEntry((0o100644, empty, 0, paths[1])), + BaseIndexEntry((0o100644, binary, 0, paths[2])), + ] + ) + commit = repo.index.commit("paths") + + diffs = commit.diff(NULL_TREE, create_patch=True) + self.assertEqual({(d.a_path, d.b_path) for d in diffs}, {(path, path) for path in paths}) + + repo.index.add( + [ + BaseIndexEntry((0o100755, binary, 0, paths[0])), + BaseIndexEntry((0o100644, binary, 0, "copy b/logo.png")), + ] + ) + changed = repo.index.commit("mode change and copy") + + diffs = commit.diff(changed, create_patch=True, C=True, find_copies_harder=True) + self.assertEqual( + {(d.a_path, d.b_path, d.copied_file) for d in diffs}, + {(paths[0], paths[0], False), (paths[0], "copy b/logo.png", True)}, + ) + def test_empty_diff(self): res = [] Diff._handle_diff_line(b"", None, res)