Skip to content

Commit de50b14

Browse files
Byroncodex
andcommitted
review
- [P2] Avoid copying the header at every possible separator — /Users/byron/dev/github.com/gitpython-developers/GitPython.diff-patch-header-paths/git/diff.py:650-654 For a long, space-heavy filename, this loop copies the header once per space, making parsing quadratic in path length. A Git-generated mode-only diff for the filename `b"x " * 250000` took about 3 seconds through `Tree.diff(..., create_patch=True)`, versus 0.023 seconds on the parent. Git accepts such tree entries without checkout, so larger commit-controlled paths can stall callers. Reject impossible split lengths before allocating slices, or check only the equal-length midpoint. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6.0 Astra <codex@openai.com>
1 parent c7ce984 commit de50b14

2 files changed

Lines changed: 76 additions & 26 deletions

File tree

‎git/diff.py‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,28 @@
99
import re
1010
import warnings
1111

12-
from git.cmd import Git, handle_process_output
13-
from git.compat import defenc
14-
from git.objects.base import IndexObject
15-
from git.objects.blob import Blob
16-
from git.objects.util import mode_str_to_int
17-
from git.util import finalize_process, hex_to_bin
18-
1912
# typing ------------------------------------------------------------------
20-
2113
from typing import (
14+
TYPE_CHECKING,
2215
Any,
2316
Iterator,
2417
List,
2518
Match,
2619
Optional,
2720
Sequence,
2821
Tuple,
29-
TYPE_CHECKING,
3022
TypeVar,
3123
Union,
3224
cast,
3325
)
34-
from git.types import PathLike, Literal
26+
27+
from git.cmd import Git, handle_process_output
28+
from git.compat import defenc
29+
from git.objects.base import IndexObject
30+
from git.objects.blob import Blob
31+
from git.objects.util import mode_str_to_int
32+
from git.types import Literal, PathLike
33+
from git.util import finalize_process, hex_to_bin
3534

3635
if TYPE_CHECKING:
3736
from subprocess import Popen
@@ -645,19 +644,22 @@ def _same_path_in_header(names: bytes) -> Optional[bytes]:
645644
end = 1
646645
while end < len(names) and names[end : end + 1] != b'"':
647646
end += 2 if names[end : end + 1] == b"\\" else 1
648-
splits = [end + 1]
647+
split = end + 1
649648
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:]
649+
# Equal unquoted paths have equal-length names, so only the midpoint
650+
# can separate them. Trying every space would copy the header quadratically.
651+
if len(names) % 2 == 0:
652+
return None
653+
split = len(names) // 2
654+
if names[split : split + 1] != b" ":
655+
return None
656+
a_name, b_name = names[:split], names[split + 1 :]
657+
if a_name.startswith(b'"'):
658+
if not (a_name.endswith(b'"') and b_name.startswith(b'"') and b_name.endswith(b'"')):
659+
return None
660+
a_name, b_name = _unquote_path(a_name[1:-1]), _unquote_path(b_name[1:-1])
661+
if a_name.startswith(b"a/") and b_name.startswith(b"b/") and a_name[2:] == b_name[2:]:
662+
return a_name[2:]
661663
return None
662664

663665
@classmethod

‎test/test_diff.py‎

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,21 @@
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

66
import gc
7-
from io import BytesIO
87
import os.path as osp
98
import shutil
109
import sys
1110
import tempfile
11+
from io import BytesIO
1212

1313
import ddt
1414
import pytest
15-
1615
from gitdb.base import IStream
1716

18-
from git import NULL_TREE, Blob, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
17+
from git import NULL_TREE, Blob, Diff, Diffable, DiffIndex, GitCommandError, Repo, Submodule
1918
from git.cmd import Git
2019
from git.diff import decode_path
2120
from git.exc import UnsafeOptionError
2221
from git.index.typ import BaseIndexEntry
23-
2422
from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory
2523

2624

@@ -306,6 +304,56 @@ def test_diff_patch_paths_named_only_by_the_header_line(self, rw_dir):
306304
{(paths[0], paths[0], False), (paths[0], "copy b/logo.png", True)},
307305
)
308306

307+
@ddt.data(
308+
(b"a/x b/x", b"x"),
309+
(b"a/x b/x ", b"x "),
310+
(b"a/x b/y", None),
311+
(b"a/x b/xx", None),
312+
(b"a/x-b/x", None),
313+
(b"", None),
314+
(b'"a/x\\t" "b/x\\t"', b"x\t"),
315+
(b'"a/x\\t" "b/x\\011"', b"x\t"),
316+
(b'"a/x\\t" "b/y\\t"', None),
317+
(b'"a/x\\t" "b/x\\t', None),
318+
)
319+
def test_same_path_in_header(self, case):
320+
names, expected = case
321+
self.assertEqual(Diff._same_path_in_header(names), expected)
322+
323+
def test_same_path_in_header_copies_linear_bytes(self):
324+
class SliceCountingBytes(bytes):
325+
sliced_bytes = 0
326+
327+
def __getitem__(self, key):
328+
result = super().__getitem__(key)
329+
if isinstance(key, slice):
330+
self.sliced_bytes += len(result)
331+
return result
332+
333+
path = b"x " * 1000
334+
names = SliceCountingBytes(b"a/" + path + b" b/" + path)
335+
self.assertEqual(Diff._same_path_in_header(names), path)
336+
# Count copied bytes rather than asserting a machine-dependent time limit.
337+
self.assertLessEqual(names.sliced_bytes, 2 * len(names))
338+
339+
@with_rw_directory
340+
def test_diff_patch_mode_only_with_long_space_heavy_path(self, rw_dir):
341+
repo = Repo.init(rw_dir)
342+
path = b"x " * 250000
343+
blob = repo.odb.store(IStream(Blob.type, 0, BytesIO(b""))).binsha
344+
trees = []
345+
# Store trees directly: Git accepts paths longer than filesystem or index limits.
346+
for mode in (b"100644", b"100755"):
347+
data = mode + b" " + path + b"\0" + blob
348+
tree = repo.odb.store(IStream(b"tree", len(data), BytesIO(data)))
349+
trees.append(repo.tree(tree.binsha.hex()))
350+
351+
diffs = trees[0].diff(trees[1], create_patch=True)
352+
self.assertEqual(len(diffs), 1)
353+
self.assertEqual((diffs[0].a_rawpath, diffs[0].b_rawpath), (path, path))
354+
self.assertEqual((diffs[0].a_mode, diffs[0].b_mode), (0o100644, 0o100755))
355+
self.assertEqual(diffs[0].diff, b"")
356+
309357
def test_empty_diff(self):
310358
res = []
311359
Diff._handle_diff_line(b"", None, res)

0 commit comments

Comments
 (0)