Skip to content

Commit 88958c9

Browse files
codexByron
authored andcommitted
fix(tree): address partial cache review feedback
Review item #2274 (comment) reports that "subsequent iteration or lookup silently accepts that truncated tree" after an entry fails validation. Stored trees containing a `.git` entry reproduce this on both backends: the first iteration rejects the name, but the second accepts an empty tree or the valid prefix. Build the entries in a local list and attach `_cache` only after every record passes parsing and validation. Failed reads leave the lazy cache unset, so later accesses retry and raise the same error instead of exposing partial data. No backend behavior or tree validation policy changes. Git 2.54.0's `mktree` and `ls-tree` accept and emit the fixture's `.git` entry; GitPython applies its existing metadata-path guard while consuming that output. Four regressions cover invalid entries first and last in SHA-1/SHA-256 trees. All fail before the fix and pass afterwards on CLI and native Gix `ls_tree`. All 97 tree tests pass on each backend. Ruff, ShellCheck and mypy pass.
1 parent 7c77a2f commit 88958c9

3 files changed

Lines changed: 32 additions & 2 deletions

File tree

‎git/objects/tree.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ def _set_cache_(self, attr: str) -> None:
219219
output = self.repo.git._call_process_safe(
220220
"ls_tree", "-z", "--full-tree", self.hexsha, stdout_as_string=False
221221
)
222-
self._cache: List[TreeCacheTup] = []
222+
cache: List[TreeCacheTup] = []
223223
for record in output.split(b"\0"):
224224
if not record:
225225
continue
@@ -230,7 +230,8 @@ def _set_cache_(self, attr: str) -> None:
230230
binsha = bytes.fromhex(oid.decode("ascii"))
231231
if len(binsha) != self.repo._oid_size:
232232
raise ValueError("Invalid tree object ID returned by Git")
233-
self._cache.append((binsha, int(mode, 8), name))
233+
cache.append((binsha, int(mode, 8), name))
234+
self._cache = cache
234235
else:
235236
super()._set_cache_(attr)
236237
# END handle attribute

‎test/test_tree.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,20 @@ def test_tree_cache_edits_do_not_update_object_ids_or_parents():
4141
assert repo.head.commit.tree["sub"].hexsha == original_oid
4242

4343

44+
@pytest.mark.parametrize("object_format", ["sha1", "sha256"])
45+
@pytest.mark.parametrize("invalid_position", ["first", "last"])
46+
def test_invalid_tree_does_not_retain_partial_cache(object_format, invalid_position):
47+
with fixture_repo("tree-invalid.sh", object_format, invalid_position) as repo:
48+
root = read_only_fixture("tree-invalid.sh", object_format, invalid_position)
49+
oid = (root / "tree-id").read_text().strip()
50+
names = [line.split("\t")[1] for line in (root / "entries").read_text().splitlines()]
51+
assert names == ([".git", "valid"] if invalid_position == "first" else ["!valid", ".git"])
52+
tree = Tree(repo, bytes.fromhex(oid), path="")
53+
for _ in range(2):
54+
with pytest.raises(ValueError, match="aliases Git metadata"):
55+
list(tree)
56+
57+
4458
@ddt.ddt
4559
class TestTree(TestBase):
4660
@with_rw_repo("HEAD")

‎tests/fixtures/tree-invalid.sh‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#!/usr/bin/env bash
2+
set -eu
3+
git init --initial-branch=main --object-format="$1" repo
4+
cd repo
5+
blob=$(printf 'content\n' | git hash-object -w --stdin)
6+
if test "$2" = first; then
7+
valid=valid
8+
else
9+
valid='!valid'
10+
fi
11+
{
12+
printf '100644 blob %s\t.git\n' "$blob"
13+
printf '100644 blob %s\t%s\n' "$blob" "$valid"
14+
} | git mktree > ../tree-id
15+
git ls-tree "$(cat ../tree-id)" > ../entries

0 commit comments

Comments
 (0)