diff --git a/docs/week8-performance.md b/docs/week8-performance.md new file mode 100644 index 0000000..6fdc485 --- /dev/null +++ b/docs/week8-performance.md @@ -0,0 +1,87 @@ +# Week 8 storage performance + +## Reproduction + +The shared moderately large fixture is generated by +[`scripts/benchmark_packfiles.py`](../scripts/benchmark_packfiles.py): 512 +deterministic blob objects, each with a 2,048-byte repeated payload and a +unique prefix. The benchmark uses a fresh temporary repository, reads through +a newly constructed `ObjectStore`, and removes loose objects before the +packed-only read. It reports retained loose bytes separately from pack and +index bytes. + +On Windows from the repository root: + +```powershell +.\.venv\Scripts\python.exe scripts\benchmark_packfiles.py --profile +.\.venv\Scripts\python.exe scripts\benchmark_packfiles.py --no-cache --profile +``` + +`--no-cache` is the pre-Week-8 comparison: it reparses and validates the +published index and complete pack for every lookup. The benchmark does not +change the object format, enable pruning, or use delta compression. + +## M4 measurement + +Environment for the recorded run: + +- Python 3.11.9 +- Windows 10, build 26200 +- 512 objects, 2,048-byte payload parameter +- One fresh-store read of every object in each storage mode + +Cached run (`--profile`): + +| Measurement | Result | +| --- | ---: | +| Loose object write time | 1,972.382 ms | +| Loose stored bytes | 27,534 | +| Loose fresh-store read time | 7,278.564 ms | +| Pack time | 128.798 ms | +| Retained loose bytes after packing | 27,534 | +| Pack bytes | 33,179 | +| Index bytes | 18,444 | +| Packed-only retained loose bytes | 0 | +| Packed-only fresh-store read time | 190.982 ms | + +The loose and packed byte totals are not directly interchangeable: packing +does not prune the loose originals, and the current format has no delta +compression. The packed-only footprint in this run is 51,623 bytes +(33,179-byte pack plus 18,444-byte index), while the loose-only footprint is +27,534 bytes. + +## Measured fix and profile + +The pre-fix comparison (`--no-cache`) reported 5,017.308 ms for the +packed-only fresh-store read. Its repeated-read profile (three passes) spent +28.802 seconds total, including 25.959 seconds in complete pack validation and +21.207 seconds in repeated record decoding. + +The fix caches metadata only after a published index and its matching pack +have passed the existing validation. Each lookup still discovers the current +`pack-*.idx` directory entries, and the cache signature includes index and +pack size plus modification time. A newly published pack is therefore found, +and a changed published file invalidates its cached validation. Selected +records continue to be decoded and hash-checked on every read. + +The cached repeated-read profile took 0.688 seconds for the same three passes. +The profile's remaining hot path is filesystem discovery and metadata checks +(`Path.glob`/`Path.stat`), not full-pack validation. + +The normal object test suite also covers a small correctness fixture with empty, +binary, and Unicode blobs; nested trees; two-parent commits; overlapping +objects across multiple packs; packed-only and mixed reads; duplicate requests; +fresh store instances; and newly published packs. Existing corruption and +publication tests cover invalid indexes and records, truncation and zlib +failures, wrong hashes, and failed publication. These tests assert behavior, +not wall-clock thresholds. + +## Remaining findings and scope + +- Loose reads remain sensitive to Windows filesystem latency; this change does + not add a loose-object cache. +- Packed storage is larger when loose originals are retained. Pruning and + delta compression remain outside this issue. +- Pack/index format redesign remains outside this issue. +- Run `scripts/quality-check.sh` before linking the PR and requesting teammate + review. This worktree intentionally has no commit, push, or pull. diff --git a/minigit/objects.py b/minigit/objects.py index ee15a42..57de788 100644 --- a/minigit/objects.py +++ b/minigit/objects.py @@ -49,6 +49,14 @@ def __init__(self, repo_path=".") -> None: self.root = Path(repo_path) self.objects_dir = self.root / ".minigit" / "objects" self.pack_dir = self.objects_dir / "pack" + self._index_cache: dict[ + Path, + tuple[ + tuple[int, int, int, int], + dict[str, tuple[int, int]], + Path, + ], + ] = {} def hash_object(self, data: bytes, obj_type: str) -> str: """ @@ -218,7 +226,7 @@ def _read_packed_object(self, obj_hash: str) -> tuple[str, bytes] | None: if not self.pack_dir.exists(): return None for index_path in sorted(self.pack_dir.glob("pack-*.idx")): - entries, pack_path = self._read_index(index_path) + entries, pack_path = self._read_cached_index(index_path) entry = entries.get(obj_hash) if entry is None: continue @@ -232,6 +240,31 @@ def _read_packed_object(self, obj_hash: str) -> tuple[str, bytes] | None: return self._decode_packed_record(obj_hash, record) return None + def _read_cached_index(self, index_path: Path) -> tuple[dict[str, tuple[int, int]], Path]: + """ + Reads a cached index from the file system. + """ + pack_path = index_path.with_suffix(".pack") + try: + index_stat = index_path.stat() + pack_stat = pack_path.stat() + except OSError as error: + raise ObjectCorruptError(index_path.name) from error + + signature = ( + index_stat.st_mtime_ns, + index_stat.st_size, + pack_stat.st_mtime_ns, + pack_stat.st_size, + ) + cached = self._index_cache.get(index_path) + if cached is not None and cached[0] == signature: + return cached[1], cached[2] + + entries, pack_path = self._read_index(index_path, pack_path) + self._index_cache[index_path] = (signature, entries, pack_path) + return entries, pack_path + def _read_index( self, index_path: Path, pack_path: Path | None = None ) -> tuple[dict[str, tuple[int, int]], Path]: @@ -291,6 +324,11 @@ def _validate_pack( or position + record_length > len(raw) ): raise ValueError + self._decode_packed_record( + "", + raw[position : position + record_length], + verify_hash=False, + ) records.append((position, record_length)) position += record_length if position != len(raw): @@ -300,10 +338,17 @@ def _validate_pack( indexed_records = list(entries.values()) if set(indexed_records) != valid_records or len(indexed_records) != count: raise ValueError - except (OSError, struct.error, ValueError) as error: + for obj_hash, (offset, record_length) in entries.items(): + self._decode_packed_record( + obj_hash, + raw[offset : offset + record_length], + ) + except (OSError, struct.error, ValueError, ObjectCorruptError) as error: raise ObjectCorruptError(pack_path.name) from error - def _decode_packed_record(self, obj_hash: str, record: bytes) -> tuple[str, bytes]: + def _decode_packed_record( + self, obj_hash: str, record: bytes, verify_hash: bool = True + ) -> tuple[str, bytes]: try: if len(record) < self._RECORD_HEADER.size: raise ValueError @@ -321,7 +366,9 @@ def _decode_packed_record(self, obj_hash: str, record: bytes) -> tuple[str, byte content = decompressor.decompress(compressed) + decompressor.flush() if not decompressor.eof or decompressor.unused_data or decompressor.unconsumed_tail: raise ValueError - if len(content) != data_length or self.hash_object(content, obj_type) != obj_hash: + if len(content) != data_length or ( + verify_hash and self.hash_object(content, obj_type) != obj_hash + ): raise ValueError except (UnicodeDecodeError, ValueError, zlib.error) as error: raise ObjectCorruptError(obj_hash) from error diff --git a/minigit/remote.py b/minigit/remote.py index aa44d9d..1f54290 100644 --- a/minigit/remote.py +++ b/minigit/remote.py @@ -24,6 +24,7 @@ from minigit.objects import ObjectStore MAX_PROTOCOL_LINE_BYTES = 65536 +MAX_OBJECT_BYTES = 64 * 1024 * 1024 def send_line(sock, text: str) -> None: @@ -78,6 +79,27 @@ def recv_exact(sock, buf: bytearray, size: int) -> bytes: return data +def _parse_object_header(header: str) -> tuple[str, int] | None: + """Parse an OBJ header without accepting an unbounded receive size.""" + + command, _, rest = header.partition(" ") + if command != "OBJ": + return None + obj_type, separator, length_text = rest.partition(" ") + if ( + not separator + or obj_type not in {"blob", "tree", "commit"} + or not length_text.isascii() + or not length_text.isdigit() + or len(length_text) > 20 + ): + return None + length = int(length_text) + if length > MAX_OBJECT_BYTES: + return None + return obj_type, length + + def _valid_hash(value: str) -> bool: """Return whether `value` is a full 40-character lowercase hex object hash.""" @@ -303,6 +325,8 @@ def _put_object(self, sock, buf: bytearray, obj_hash: str) -> None: """Send `PUT ` followed by the object's `OBJ` header and bytes.""" obj_type, content = self.store.read_object(obj_hash) + if len(content) > MAX_OBJECT_BYTES: + raise NetworkProtocolError(f"object {obj_hash} exceeds transfer size limit") send_line(sock, f"PUT {obj_hash}") send_line(sock, f"OBJ {obj_type} {len(content)}") sock.sendall(content) @@ -451,16 +475,12 @@ def _receive_object( if command != "OBJ": raise NetworkProtocolError(f"expected OBJ, got {header!r}") - obj_type, _, length_text = rest.partition(" ") - if obj_type not in {"blob", "tree", "commit"} or not ( - length_text.isascii() - and length_text.isdigit() - and len(length_text) <= 20 - and int(length_text) <= sys.maxsize - ): + parsed_header = _parse_object_header(header) + if parsed_header is None: raise NetworkProtocolError(f"malformed OBJ header: {header!r}") + obj_type, length = parsed_header - content = recv_exact(sock, buf, int(length_text)) + content = recv_exact(sock, buf, length) if self.store.hash_object(content, obj_type) != expected_hash: raise NetworkProtocolError(f"object {expected_hash} failed hash verification") if expected_type is not None and obj_type != expected_type: @@ -735,22 +755,16 @@ def _receive_pushed_object(self, conn, buf: bytearray, expected_hash: str) -> bo return False header = receive_line(conn, buf) - command, _, rest = header.partition(" ") - if command != "OBJ": - send_line(conn, "ERR expected OBJ") - return False - - obj_type, _, length_text = rest.partition(" ") - if obj_type not in {"blob", "tree", "commit"} or not ( - length_text.isascii() - and length_text.isdigit() - and len(length_text) <= 20 - and int(length_text) <= sys.maxsize - ): - send_line(conn, "ERR malformed OBJ header") + parsed_header = _parse_object_header(header) + if parsed_header is None: + if not header.startswith("OBJ "): + send_line(conn, "ERR expected OBJ") + else: + send_line(conn, "ERR malformed OBJ header") return False + obj_type, length = parsed_header - content = recv_exact(conn, buf, int(length_text)) + content = recv_exact(conn, buf, length) if self.store.hash_object(content, obj_type) != expected_hash: send_line(conn, f"ERR hash mismatch for {expected_hash}") return False @@ -800,6 +814,9 @@ def _send_object(self, conn, obj_hash: str) -> None: send_line(conn, f"ERR corrupt object {obj_hash}") return + if len(content) > MAX_OBJECT_BYTES: + send_line(conn, f"ERR object {obj_hash} exceeds transfer size limit") + return send_line(conn, f"OBJ {obj_type} {len(content)}") conn.sendall(content) diff --git a/scripts/benchmark_packfiles.py b/scripts/benchmark_packfiles.py new file mode 100644 index 0000000..fb23a2d --- /dev/null +++ b/scripts/benchmark_packfiles.py @@ -0,0 +1,106 @@ +"""Measure loose and packed object storage on a deterministic shared fixture.""" + +import argparse +import cProfile +import io +import platform +import pstats +import sys +import tempfile +import time +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from minigit.objects import ObjectStore + + +def storage_bytes(root: Path) -> int: + return sum( + path.stat().st_size + for path in (root / ".minigit" / "objects").glob("*/*") + if path.is_file() + ) + + +def read_all(store: ObjectStore, hashes: list[str], repeats: int = 1) -> float: + started = time.perf_counter() + for _ in range(repeats): + for obj_hash in hashes: + store.read_object(obj_hash) + return (time.perf_counter() - started) * 1000 + + +def read_uncached_index( + store: ObjectStore, index_path: Path +) -> tuple[dict[str, tuple[int, int]], Path]: + return store._read_index(index_path) + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("--objects", type=int, default=512) + parser.add_argument("--payload-size", type=int, default=2048) + parser.add_argument("--profile", action="store_true") + parser.add_argument("--no-cache", action="store_true") + args = parser.parse_args() + if args.objects <= 0 or args.payload_size <= 0: + parser.error("--objects and --payload-size must be positive") + if args.no_cache: + ObjectStore._read_cached_index = read_uncached_index + + with tempfile.TemporaryDirectory(prefix="minigit-m4-fixture-") as directory: + root = Path(directory) + store = ObjectStore(root) + hashes = [] + started = time.perf_counter() + for index in range(args.objects): + prefix = f"shared-fixture-{index:06d}:".encode() + data = prefix + bytes((index % 251,)) * args.payload_size + hashes.append(store.write_object(data, "blob")) + write_ms = (time.perf_counter() - started) * 1000 + loose_bytes = storage_bytes(root) + + loose_read_ms = read_all(ObjectStore(root), hashes) + started = time.perf_counter() + pack_path, index_path = store.pack_objects(hashes) + pack_ms = (time.perf_counter() - started) * 1000 + retained_loose_bytes = sum( + path.stat().st_size + for path in (root / ".minigit" / "objects").glob("*/*") + if path.is_file() and path.parent.name != "pack" + ) + pack_bytes = pack_path.stat().st_size + index_bytes = index_path.stat().st_size + + for obj_hash in hashes: + store._object_path(obj_hash).unlink() + packed_read_ms = read_all(ObjectStore(root), hashes) + + print(f"python={sys.version.split()[0]}") + print(f"platform={platform.platform()}") + print(f"fixture_objects={len(hashes)}") + print(f"fixture_payload_size={args.payload_size}") + print(f"loose_write_ms={write_ms:.3f}") + print(f"loose_bytes={loose_bytes}") + print(f"loose_fresh_read_ms={loose_read_ms:.3f}") + print(f"pack_ms={pack_ms:.3f}") + print(f"packed_retained_loose_bytes={retained_loose_bytes}") + print(f"pack_bytes={pack_bytes}") + print(f"index_bytes={index_bytes}") + print(f"packed_only_fresh_read_ms={packed_read_ms:.3f}") + + if args.profile: + profiler = cProfile.Profile() + profiler.enable() + read_all(ObjectStore(root), hashes, repeats=3) + profiler.disable() + output = io.StringIO() + pstats.Stats(profiler, stream=output).sort_stats("cumtime").print_stats(12) + print(output.getvalue()) + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_objects.py b/tests/test_objects.py index 51899e4..80da98a 100644 --- a/tests/test_objects.py +++ b/tests/test_objects.py @@ -7,6 +7,7 @@ import pytest from minigit.cli import main +from minigit.commits import CommitManager from minigit.errors import ObjectCorruptError, ObjectNotFoundError from minigit.objects import ObjectStore, TreeEntry @@ -124,6 +125,59 @@ def test_has_object_distinguishes_missing_valid_and_corrupt(tmp_path: Path) -> N store.has_object(obj_hash) +def test_corrupt_loose_object_wins_over_valid_packed_copy_with_fresh_store( + tmp_path: Path, +) -> None: + """A corrupt loose object must not fall back to a valid packed copy.""" + writer = ObjectStore(tmp_path) + obj_hash = writer.write_object(b"content", "blob") + writer.pack_objects([obj_hash]) + writer._object_path(obj_hash).write_bytes(b"corrupt") + + reader = ObjectStore(tmp_path) + with pytest.raises(ObjectCorruptError): + reader.read_object(obj_hash) + with pytest.raises(ObjectCorruptError): + reader.has_object(obj_hash) + with pytest.raises(ObjectCorruptError): + reader.write_object(b"content", "blob") + + +@pytest.mark.parametrize( + ("storage", "expected"), + [ + ("loose", ("blob", b"loose")), + ("packed", ("blob", b"packed")), + ("mixed", ("blob", b"loose")), + ], +) +def test_fresh_store_lookup_modes_preserve_missing_and_corrupt_distinctions( + tmp_path: Path, storage: str, expected: tuple[str, bytes] +) -> None: + """Fresh stores distinguish valid, missing, and corrupt objects in each mode.""" + writer = ObjectStore(tmp_path) + data = b"loose" if storage == "mixed" else storage.encode() + obj_hash = writer.write_object(data, "blob") + if storage in {"packed", "mixed"}: + writer.pack_objects([obj_hash]) + if storage == "packed": + writer._object_path(obj_hash).unlink() + + reader = ObjectStore(tmp_path) + assert reader.read_object(obj_hash) == expected + assert reader.has_object(obj_hash) + assert reader.write_object(data, "blob") == obj_hash + if storage == "packed": + assert not reader._object_path(obj_hash).exists() + with pytest.raises(ObjectNotFoundError): + reader.read_object("a" * 40) + assert not reader.has_object("a" * 40) + + reader._object_path(obj_hash).write_bytes(b"corrupt") + with pytest.raises(ObjectCorruptError): + reader.read_object(obj_hash) + + def test_identical_objects_same_hash(tmp_path: Path): """ Test that writing the same object twice returns the same hash. @@ -426,6 +480,86 @@ def test_pack_objects_mixes_loose_and_packed_reads(tmp_path: Path) -> None: assert store.has_object(packed_hash) +def test_fresh_store_reads_multiple_packs_and_object_types(tmp_path: Path) -> None: + """Read nested trees and a merge commit from overlapping packed and loose storage.""" + writer = ObjectStore(tmp_path) + commits = CommitManager(tmp_path, store=writer) + objects = [ + (b"", "blob"), + (b"\x00binary\xff", "blob"), + ("café".encode(), "blob"), + ] + hashes = [writer.write_object(data, obj_type) for data, obj_type in objects] + nested_hash = writer.write_tree([TreeEntry("100644", "blob", hashes[1], "binary")]) + root_hash = writer.write_tree( + [ + TreeEntry("100644", "blob", hashes[0], "empty"), + TreeEntry("40000", "tree", nested_hash, "nested"), + TreeEntry("100644", "blob", hashes[2], "unicode"), + ] + ) + first_commit = commits.create_commit(root_hash, [], "tester ", "first") + side_commit = commits.create_commit(root_hash, [], "tester ", "side") + merge_commit = commits.create_commit( + root_hash, + [first_commit, side_commit], + "tester ", + "merge", + ) + + first_pack_objects = [hashes[0], nested_hash, root_hash, first_commit] + second_pack_objects = [root_hash, hashes[1], hashes[2], side_commit, merge_commit] + writer.pack_objects([*first_pack_objects, first_pack_objects[0]]) + writer.pack_objects([*second_pack_objects, root_hash]) + for obj_hash in set(first_pack_objects + second_pack_objects): + if obj_hash != hashes[2]: + writer._object_path(obj_hash).unlink() + + reader = ObjectStore(tmp_path) + reader_commits = CommitManager(tmp_path, store=reader) + assert reader.read_object(hashes[0]) == ("blob", b"") + assert reader.read_object(hashes[1]) == ("blob", b"\x00binary\xff") + assert reader.read_object(hashes[2]) == ("blob", "café".encode()) + assert reader.read_tree(root_hash) == [ + TreeEntry("100644", "blob", hashes[0], "empty"), + TreeEntry("40000", "tree", nested_hash, "nested"), + TreeEntry("100644", "blob", hashes[2], "unicode"), + ] + assert reader.read_object(merge_commit)[0] == "commit" + assert reader_commits.read_commit(merge_commit).parents == [first_commit, side_commit] + + +def test_packed_index_cache_discovers_new_packs(tmp_path: Path) -> None: + """A cached lookup still discovers a pack published after the first read.""" + store = ObjectStore(tmp_path) + first_hash = store.write_object(b"first", "blob") + second_hash = store.write_object(b"second", "blob") + store.pack_objects([first_hash]) + store._object_path(first_hash).unlink() + + assert store.read_object(first_hash) == ("blob", b"first") + + store.pack_objects([second_hash]) + store._object_path(second_hash).unlink() + assert store.read_object(second_hash) == ("blob", b"second") + + +def test_packed_index_cache_rechecks_changed_pack(tmp_path: Path) -> None: + """Changing a published pack invalidates its cached validation.""" + store = ObjectStore(tmp_path) + obj_hash = store.write_object(b"content", "blob") + pack_path, _ = store.pack_objects([obj_hash]) + store._object_path(obj_hash).unlink() + + assert store.read_object(obj_hash) == ("blob", b"content") + raw = bytearray(pack_path.read_bytes()) + raw[-1] ^= 0xFF + pack_path.write_bytes(raw) + + with pytest.raises(ObjectCorruptError): + store.read_object(obj_hash) + + def test_duplicate_write_validates_packed_only_object(tmp_path: Path) -> None: store = ObjectStore(tmp_path) obj_hash = store.write_object(b"packed", "blob") @@ -450,6 +584,70 @@ def test_corrupt_pack_record_is_reported(tmp_path: Path) -> None: store.read_object(obj_hash) +def test_packed_lookup_validates_every_record_before_returning_any_object(tmp_path: Path) -> None: + """A corrupt non-selected record invalidates the published pack.""" + store = ObjectStore(tmp_path) + first = store.write_object(b"first", "blob") + second = store.write_object(b"second", "blob") + pack_path, _ = store.pack_objects([first, second]) + store._object_path(first).unlink() + store._object_path(second).unlink() + + raw = bytearray(pack_path.read_bytes()) + first_record_end = store._PACK_HEADER.size + store._RECORD_HEADER.size + type_length = raw[store._PACK_HEADER.size] + compressed_length = int.from_bytes( + raw[store._PACK_HEADER.size + 9 : store._PACK_HEADER.size + 17], "big" + ) + second_record = first_record_end + type_length + compressed_length + raw[second_record + store._RECORD_HEADER.size + 4] ^= 0xFF + pack_path.write_bytes(raw) + + with pytest.raises(ObjectCorruptError): + ObjectStore(tmp_path).read_object(first) + + +@pytest.mark.parametrize("header", ["pack", "index"]) +def test_published_pack_and_index_versions_are_validated(tmp_path: Path, header: str) -> None: + store = ObjectStore(tmp_path) + obj_hash = store.write_object(b"content", "blob") + pack_path, index_path = store.pack_objects([obj_hash]) + path = pack_path if header == "pack" else index_path + raw = bytearray(path.read_bytes()) + version_offset = 4 + raw[version_offset : version_offset + 4] = (store._PACK_VERSION + 1).to_bytes(4, "big") + path.write_bytes(raw) + store._object_path(obj_hash).unlink() + + with pytest.raises(ObjectCorruptError): + ObjectStore(tmp_path).read_object(obj_hash) + + +def test_published_pack_count_and_index_offset_are_validated(tmp_path: Path) -> None: + store = ObjectStore(tmp_path) + obj_hash = store.write_object(b"content", "blob") + pack_path, index_path = store.pack_objects([obj_hash]) + store._object_path(obj_hash).unlink() + + pack = bytearray(pack_path.read_bytes()) + pack[8:12] = (2).to_bytes(4, "big") + pack_path.write_bytes(pack) + with pytest.raises(ObjectCorruptError): + ObjectStore(tmp_path).read_object(obj_hash) + + offset_root = tmp_path / "offset" + offset_store = ObjectStore(offset_root) + obj_hash = offset_store.write_object(b"content", "blob") + pack_path, index_path = offset_store.pack_objects([obj_hash]) + offset_store._object_path(obj_hash).unlink() + index = bytearray(index_path.read_bytes()) + offset_start = offset_store._INDEX_HEADER.size + 20 + index[offset_start : offset_start + 8] = (1).to_bytes(8, "big") + index_path.write_bytes(index) + with pytest.raises(ObjectCorruptError): + ObjectStore(offset_root).read_object(obj_hash) + + def test_bad_pack_index_is_reported(tmp_path: Path) -> None: """Tests that a bad pack index raises ObjectCorruptError when reading the object.""" store = ObjectStore(tmp_path) @@ -467,7 +665,7 @@ def test_bad_pack_index_is_reported(tmp_path: Path) -> None: def test_pack_publication_failure_leaves_previous_objects_readable( tmp_path: Path, monkeypatch ) -> None: - """Tests that a failure during pack publication leaves previous objects readable.""" + """A failed index publication leaves loose objects and no visible pack.""" store = ObjectStore(tmp_path) obj_hash = store.write_object(b"content", "blob") @@ -487,6 +685,33 @@ def fail_index_publish(source, destination): store.pack_objects([obj_hash]) assert store.read_object(obj_hash) == ("blob", b"content") + pack_files = sorted(store.pack_dir.glob("pack-*")) + assert [path.suffix for path in pack_files] == [".pack"] + assert all(not path.name.startswith(".pack-") for path in pack_files) + assert not list(store.pack_dir.glob("*.idx")) + assert not list(store.pack_dir.glob("*.tmp")) + + store._object_path(obj_hash).unlink() + with pytest.raises(ObjectNotFoundError): + ObjectStore(tmp_path).read_object(obj_hash) + + +def test_pack_publication_failure_cleans_temporary_files(tmp_path: Path, monkeypatch) -> None: + """A failure before pack publication removes both temporary files.""" + store = ObjectStore(tmp_path) + obj_hash = store.write_object(b"content", "blob") + + def fail_pack_validation(path): + raise OSError("simulated validation failure") + + monkeypatch.setattr(store, "_validate_pack", fail_pack_validation) + with pytest.raises(OSError): + store.pack_objects([obj_hash]) + + assert not list(store.pack_dir.glob("pack-*.pack")) + assert not list(store.pack_dir.glob("pack-*.idx")) + assert not list(store.pack_dir.glob("*.tmp")) + assert store.read_object(obj_hash) == ("blob", b"content") @pytest.mark.parametrize("obj_type", ["unknown", "blob\ncommit", ""])