Merge pull request #2295 from Keerthana-64/diff-patch-header-paths

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

Author: Byron

git/diff.py

@@ -9,29 +9,28 @@
 import re
 import warnings
 
-from git.cmd import Git, handle_process_output
-from git.compat import defenc
-from git.objects.base import IndexObject
-from git.objects.blob import Blob
-from git.objects.util import mode_str_to_int
-from git.util import finalize_process, hex_to_bin
-
 # typing ------------------------------------------------------------------
-
 from typing import (
+    TYPE_CHECKING,
     Any,
     Iterator,
     List,
     Match,
     Optional,
     Sequence,
     Tuple,
-    TYPE_CHECKING,
     TypeVar,
     Union,
     cast,
 )
-from git.types import PathLike, Literal
+
+from git.cmd import Git, handle_process_output
+from git.compat import defenc
+from git.objects.base import IndexObject
+from git.objects.blob import Blob
+from git.objects.util import mode_str_to_int
+from git.types import Literal, PathLike
+from git.util import finalize_process, hex_to_bin
 
 if TYPE_CHECKING:
     from subprocess import Popen
@@ -410,7 +409,8 @@ class Diff:
     re_header = re.compile(
         rb"""
                                 ^diff[ ]--git
-                                    [ ](?P<a_path_fallback>"?[ab]/.+?"?)[ ](?P<b_path_fallback>"?[ab]/.+?"?)\n
+                                    [ ](?P<header_names>(?P<a_path_fallback>"?[ab]/.+?"?)
+                                        [ ](?P<b_path_fallback>"?[ab]/.+?"?))\n
                                 (?:^old[ ]mode[ ](?P<old_mode>\d+)\n
                                    ^new[ ]mode[ ](?P<new_mode>\d+)(?:\n|$))?
                                 (?:^similarity[ ]index[ ]\d+%\n
@@ -419,7 +419,7 @@ class Diff:
                                 (?:^new[ ]file[ ]mode[ ](?P<new_file_mode>.+)(?:\n|$))?
                                 (?:^deleted[ ]file[ ]mode[ ](?P<deleted_file_mode>.+)(?:\n|$))?
                                 (?:^similarity[ ]index[ ]\d+%\n
-                                   ^copy[ ]from[ ].*\n
+                                   ^copy[ ]from[ ](?P<copied_from_name>.*)\n
                                    ^copy[ ]to[ ](?P<copied_file_name>.*)(?:\n|$))?
                                 (?:^index[ ](?P<a_blob_id>[0-9A-Fa-f]+)
                                     \.\.(?P<b_blob_id>[0-9A-Fa-f]+)[ ]?(?P<b_mode>.+)?(?:\n|$))?
@@ -626,6 +626,42 @@ 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/<path> b/<path>`` 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
+            split = end + 1
+        else:
+            # Equal unquoted paths have equal-length names, so only the midpoint
+            # can separate them. Trying every space would copy the header quadratically.
+            if len(names) % 2 == 0:
+                return None
+            split = len(names) // 2
+        if names[split : split + 1] != b" ":
+            return None
+        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'"')):
+                return None
+            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 +698,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 +707,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 +722,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.

test/test_diff.py

@@ -4,23 +4,21 @@
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
 import gc
-from io import BytesIO
 import os.path as osp
 import shutil
 import sys
 import tempfile
+from io import BytesIO
 
 import ddt
 import pytest
-
 from gitdb.base import IStream
 
-from git import NULL_TREE, Blob, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
+from git import NULL_TREE, Blob, Diff, Diffable, DiffIndex, GitCommandError, Repo, Submodule
 from git.cmd import Git
 from git.diff import decode_path
 from git.exc import UnsafeOptionError
 from git.index.typ import BaseIndexEntry
-
 from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory
 
 
@@ -270,6 +268,92 @@ 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/<path> b/<path>` 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)},
+        )
+
+    @ddt.data(
+        (b"a/x b/x", b"x"),
+        (b"a/x  b/x ", b"x "),
+        (b"a/x b/y", None),
+        (b"a/x b/xx", None),
+        (b"a/x-b/x", None),
+        (b"", None),
+        (b'"a/x\\t" "b/x\\t"', b"x\t"),
+        (b'"a/x\\t" "b/x\\011"', b"x\t"),
+        (b'"a/x\\t" "b/y\\t"', None),
+        (b'"a/x\\t" "b/x\\t', None),
+    )
+    def test_same_path_in_header(self, case):
+        names, expected = case
+        self.assertEqual(Diff._same_path_in_header(names), expected)
+
+    def test_same_path_in_header_copies_linear_bytes(self):
+        class SliceCountingBytes(bytes):
+            sliced_bytes = 0
+
+            def __getitem__(self, key):
+                result = super().__getitem__(key)
+                if isinstance(key, slice):
+                    self.sliced_bytes += len(result)
+                return result
+
+        path = b"x " * 1000
+        names = SliceCountingBytes(b"a/" + path + b" b/" + path)
+        self.assertEqual(Diff._same_path_in_header(names), path)
+        # Count copied bytes rather than asserting a machine-dependent time limit.
+        self.assertLessEqual(names.sliced_bytes, 2 * len(names))
+
+    @with_rw_directory
+    def test_diff_patch_mode_only_with_long_space_heavy_path(self, rw_dir):
+        repo = Repo.init(rw_dir)
+        path = b"x " * 250000
+        blob = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha
+        trees = []
+        # Store trees directly: Git accepts paths longer than filesystem or index limits.
+        for mode in (b"100644", b"100755"):
+            data = mode + b" " + path + b"\0" + blob
+            tree = repo.odb.store(IStream(b"tree", len(data), BytesIO(data)))
+            trees.append(repo.tree(tree.binsha.hex()))
+
+        diffs = trees[0].diff(trees[1], create_patch=True)
+        self.assertEqual(len(diffs), 1)
+        self.assertEqual((diffs[0].a_rawpath, diffs[0].b_rawpath), (path, path))
+        self.assertEqual((diffs[0].a_mode, diffs[0].b_mode), (0o100644, 0o100755))
+        self.assertEqual(diffs[0].diff, b"")
+
     def test_empty_diff(self):
         res = []
         Diff._handle_diff_line(b"", None, res)