From 13d88f24af0147e0b6dcf1f8ec02e0ed3210d445 Mon Sep 17 00:00:00 2001 From: "Myeongjin (Daniel)" Date: Mon, 21 Sep 2026 01:50:11 -0400 Subject: [PATCH 1/2] add commit parsing and history for real commits --- minigit/commits.py | 99 ++++++++++++++++++++----- tests/test_commits.py | 164 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 235 insertions(+), 28 deletions(-) diff --git a/minigit/commits.py b/minigit/commits.py index 2479882..d9bd1db 100644 --- a/minigit/commits.py +++ b/minigit/commits.py @@ -12,7 +12,7 @@ import os import time -from minigit.errors import MiniGitError, RefExistsError, RefNotFoundError +from minigit.errors import MiniGitError, ObjectCorruptError, RefExistsError, RefNotFoundError from minigit.index import WorkingTree from minigit.objects import ObjectStore @@ -96,6 +96,15 @@ def register_subcommands(subparsers) -> None: log_parser.set_defaults(handler=_cmd_log) +class CommitData: + def __init__(self, tree: str, parents: list[str], author: str, committer: str, message: str): + self.tree = tree + self.parents = parents + self.author = author + self.committer = committer + self.message = message + + class CommitManager: def __init__(self, repo_path=".", store=None, tree=None): self.root = os.path.abspath(repo_path) @@ -179,6 +188,67 @@ def _current_branch(self) -> str: """Return the name of the branch HEAD currently points at.""" return self.read_head() or "main" + def read_commit(self, commit_hash: str) -> CommitData: + + obj_type, data = self.store.read_object(commit_hash) + if obj_type != "commit": + raise ObjectCorruptError(commit_hash) + try: + body = data.decode() + header, message = body.split("\n\n", 1) + lines = header.splitlines() + + if not lines[0].startswith("tree "): + raise ValueError("missing tree line") + tree = lines[0][len("tree ") :] + + i = 1 + parents = [] + while lines[i].startswith("parent "): + parents.append(lines[i][len("parent ") :]) + i += 1 + if not lines[i].startswith("author "): + raise ValueError("mising author line") + author = lines[i][len("author ") :].rsplit(" ", 1)[0] + i += 1 + + if not lines[i].startswith("committer "): + raise ValueError("missing committer line") + committer = lines[i][len("committer ") :].rsplit(" ", 1)[0] + + except (UnicodeDecodeError, IndexError, ValueError) as error: + raise ObjectCorruptError(commit_hash) from error + + return CommitData(tree, parents, author, committer, message) + + def get_head_tree(self) -> str | None: + head = self._current_branch() + if os.path.exists(self._ref_path(head)): + commit_hash = self.read_ref(head) + elif len(head) == 40: + commit_hash = head + else: + return None + + return self.read_commit(commit_hash).tree + + def walk_history(self, start_hash: str) -> list[str]: + visited = [] + seen = set() + stack = [start_hash] + + while stack: + commit_hash = stack.pop() + if commit_hash in seen: + continue + seen.add(commit_hash) + visited.append(commit_hash) + + parents = self.read_commit(commit_hash).parents + stack.extend(reversed(parents)) + + return visited + def create_commit(self, tree_hash, parents, author, message) -> str: """ Create a new commit object, write it to the object store, and advance @@ -187,6 +257,10 @@ def create_commit(self, tree_hash, parents, author, message) -> str: The caller is responsible for resolving `parents` (e.g. via `read_ref` on the current branch, or `[]` for a root commit). """ + obj_type, _ = self.store.read_object(tree_hash) + if obj_type != "tree": + raise ObjectCorruptError(tree_hash) + branch = self._current_branch() body = self._format_commit(tree_hash, parents, author, message) commit_hash = self.store.write_object(body.encode(), "commit") @@ -219,24 +293,17 @@ def merge(self, branch_name) -> str | None: def log(self) -> list[str]: """Return one summary line per commit reachable from HEAD, newest first""" - - branch = self._current_branch() - commit_hash = self.read_ref(branch) + commit_hash = self.read_ref(self._current_branch()) if not commit_hash: return [] lines = [] - while commit_hash: - _, data = self.store.read_object(commit_hash) - body = data.decode() - message = body.split("\n\n", 1)[1].splitlines()[0] - lines.append(f"{commit_hash[:7]} {message}") - - parent_hash = None - for line in body.splitlines(): - if line.startswith("parent "): - parent_hash = line.split(" ", 1)[1] - break - commit_hash = parent_hash + for i in self.walk_history(commit_hash): + msg = self.read_commit(i).message + if msg: + summary = msg.splitlines()[0] + else: + summary = "" + lines.append(f"{i[:7]} {summary}") return lines diff --git a/tests/test_commits.py b/tests/test_commits.py index 0a59d66..e015456 100644 --- a/tests/test_commits.py +++ b/tests/test_commits.py @@ -4,9 +4,11 @@ import pytest from minigit.commits import CommitManager -from minigit.errors import RefExistsError, RefNotFoundError +from minigit.errors import ObjectCorruptError, ObjectNotFoundError, RefExistsError, RefNotFoundError from minigit.objects import ObjectStore +AUTHOR = "Daniel " + class FakeWorkingTree: """Minimal stand-in for WorkingTree. No filesystem operations.""" @@ -22,6 +24,11 @@ def make_manager(temp_path): ) +def make_tree(m) -> str: + """Write a real (empty) tree object so create_commit's validation passes.""" + return m.store.write_object(b"", "tree") + + # testing commits @@ -73,28 +80,29 @@ def test_merge_commit_two_parent_lines_in_order(tmp_path): # testing create_commit + refs def test_create_commit_returns_hash(tmp_path): m = make_manager(tmp_path) - result = m.create_commit("0" * 40, [], "Daniel ", "init") + result = m.create_commit(make_tree(m), [], AUTHOR, "init") assert len(result) == 40 def test_first_commit_has_no_parent_lines(tmp_path): m = make_manager(tmp_path) - commit_hash = m.create_commit("a" * 40, [], "Daniel ", "init") + commit_hash = m.create_commit(make_tree(m), [], AUTHOR, "init") _, body = m.store.read_object(commit_hash) assert "parent" not in body.decode() def test_first_commit_creates_ref_file(tmp_path): m = make_manager(tmp_path) - m.create_commit("a" * 40, [], "Daniel ", "init") + m.create_commit(make_tree(m), [], AUTHOR, "init") ref_file = tmp_path / ".minigit" / "refs" / "heads" / "main" assert ref_file.exists() def test_second_commit_has_one_parent_line_pointing_at_first(tmp_path): m = make_manager(tmp_path) - first = m.create_commit("a" * 40, [], "Daniel ", "init") - second = m.create_commit("b" * 40, [first], "Daniel ", "second") + tree = make_tree(m) + first = m.create_commit(tree, [], AUTHOR, "init") + second = m.create_commit(tree, [first], AUTHOR, "second") _, body = m.store.read_object(second) parent_lines = [line for line in body.decode().splitlines() if line.startswith("parent ")] assert len(parent_lines) == 1 @@ -103,13 +111,122 @@ def test_second_commit_has_one_parent_line_pointing_at_first(tmp_path): def test_second_commit_moves_the_ref_file(tmp_path): m = make_manager(tmp_path) - first = m.create_commit("a" * 40, [], "Daniel ", "init") - second = m.create_commit("b" * 40, [first], "Daniel ", "second") + tree = make_tree(m) + first = m.create_commit(tree, [], AUTHOR, "init") + second = m.create_commit(tree, [first], AUTHOR, "second") ref_file = tmp_path / ".minigit" / "refs" / "heads" / "main" assert ref_file.read_text() == second + "\n" assert ref_file.read_text() != first +def test_create_commit_rejects_non_tree_and_leaves_ref_unchanged(tmp_path): + m = make_manager(tmp_path) + first = m.create_commit(make_tree(m), [], AUTHOR, "init") + blob = m.store.write_object(b"hello", "blob") + with pytest.raises(ObjectCorruptError): + m.create_commit(blob, [first], AUTHOR, "bad") + assert m.read_ref("main") == first + + +def test_create_commit_missing_tree_raises(tmp_path): + m = make_manager(tmp_path) + with pytest.raises(ObjectNotFoundError): + m.create_commit("f" * 40, [], AUTHOR, "bad") + + +# testing read_commit +def test_read_commit_round_trips_fields(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + h = m.create_commit(tree, [], AUTHOR, "init") + c = m.read_commit(h) + assert c.tree == tree + assert c.parents == [] + assert c.author == AUTHOR + assert c.committer == AUTHOR + assert c.message == "init" + + +def test_read_commit_keeps_all_parents_in_order(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + p1 = m.create_commit(tree, [], AUTHOR, "one") + p2 = m.create_commit(tree, [], AUTHOR, "two") + merge = m.create_commit(tree, [p1, p2], AUTHOR, "merge") + assert m.read_commit(merge).parents == [p1, p2] + + +def test_read_commit_preserves_multiline_message(tmp_path): + m = make_manager(tmp_path) + msg = "summary\n\nlonger body\nwith two lines" + h = m.create_commit(make_tree(m), [], AUTHOR, msg) + assert m.read_commit(h).message == msg + + +def test_read_commit_wrong_type_raises(tmp_path): + m = make_manager(tmp_path) + blob = m.store.write_object(b"hello", "blob") + with pytest.raises(ObjectCorruptError): + m.read_commit(blob) + + +def test_read_commit_malformed_body_raises(tmp_path): + m = make_manager(tmp_path) + bad = m.store.write_object(b"not a real commit", "commit") + with pytest.raises(ObjectCorruptError): + m.read_commit(bad) + + +# testing get_head_tree +def test_get_head_tree_unborn_branch_returns_none(tmp_path): + m = make_manager(tmp_path) + assert m.get_head_tree() is None + + +def test_get_head_tree_after_commit(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + m.create_commit(tree, [], AUTHOR, "init") + assert m.get_head_tree() == tree + + +def test_get_head_tree_detached_head(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + h = m.create_commit(tree, [], AUTHOR, "init") + (tmp_path / ".minigit" / "HEAD").write_text(h + "\n") + assert m.get_head_tree() == tree + + +def test_get_head_tree_missing_commit_raises(tmp_path): + m = make_manager(tmp_path) + m.write_ref("main", "f" * 40) + with pytest.raises(ObjectNotFoundError): + m.get_head_tree() + + +# testing walk_history +def test_walk_history_linear_newest_first(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + a = m.create_commit(tree, [], AUTHOR, "a") + b = m.create_commit(tree, [a], AUTHOR, "b") + c = m.create_commit(tree, [b], AUTHOR, "c") + assert m.walk_history(c) == [c, b, a] + + +def test_walk_history_merge_visits_both_sides_once(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + base = m.create_commit(tree, [], AUTHOR, "base") + left = m.create_commit(tree, [base], AUTHOR, "left") + right = m.create_commit(tree, [base], AUTHOR, "right") + merge = m.create_commit(tree, [left, right], AUTHOR, "merge") + result = m.walk_history(merge) + assert result == [merge, left, base, right] + assert len(result) == len(set(result)) + + # testing branches def test_create_and_list_branches(tmp_path): m = make_manager(tmp_path) @@ -140,10 +257,11 @@ def test_switch_branch_updates_current_branch(tmp_path): def test_branches_point_at_different_hashes_after_switch(tmp_path): m = make_manager(tmp_path) - first = m.create_commit("a" * 40, [], "Daniel ", "on main") + tree = make_tree(m) + first = m.create_commit(tree, [], AUTHOR, "on main") m.create_branch("feature", first) m.switch_branch("feature") - second = m.create_commit("b" * 40, [first], "Daniel ", "on feature") + second = m.create_commit(tree, [first], AUTHOR, "on feature") m.switch_branch("main") main_ref = tmp_path / ".minigit" / "refs" / "heads" / "main" feature_ref = tmp_path / ".minigit" / "refs" / "heads" / "feature" @@ -155,9 +273,31 @@ def test_branches_point_at_different_hashes_after_switch(tmp_path): # testing log def test_log_has_one_line_per_commit_newest_first(tmp_path): m = make_manager(tmp_path) - first = m.create_commit("a" * 40, [], "Daniel ", "first") - m.create_commit("b" * 40, [first], "Daniel ", "second") + tree = make_tree(m) + first = m.create_commit(tree, [], AUTHOR, "first") + m.create_commit(tree, [first], AUTHOR, "second") lines = m.log() assert len(lines) == 2 assert "second" in lines[0] assert "first" in lines[1] + + +def test_log_shows_both_sides_of_merge_without_duplicates(tmp_path): + m = make_manager(tmp_path) + tree = make_tree(m) + base = m.create_commit(tree, [], AUTHOR, "base") + left = m.create_commit(tree, [base], AUTHOR, "left") + right = m.create_commit(tree, [base], AUTHOR, "right") + m.create_commit(tree, [left, right], AUTHOR, "merge") + lines = m.log() + assert len(lines) == 4 + assert sum("base" in line for line in lines) == 1 + + +def test_fresh_manager_reads_same_history(tmp_path): + m1 = make_manager(tmp_path) + tree = make_tree(m1) + a = m1.create_commit(tree, [], AUTHOR, "a") + m1.create_commit(tree, [a], AUTHOR, "b") + m2 = make_manager(tmp_path) + assert m2.log() == m1.log() From 927212ed91400603810d3bd87ccfd241608d8ded Mon Sep 17 00:00:00 2001 From: aman shah Date: Mon, 21 Sep 2026 14:55:50 -0400 Subject: [PATCH 2/2] Validate commit headers and share the injected object store --- minigit/commits.py | 14 ++++++++++++-- tests/test_commits.py | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/minigit/commits.py b/minigit/commits.py index d9bd1db..13877c2 100644 --- a/minigit/commits.py +++ b/minigit/commits.py @@ -109,7 +109,7 @@ class CommitManager: def __init__(self, repo_path=".", store=None, tree=None): self.root = os.path.abspath(repo_path) self.store = store if store is not None else ObjectStore(repo_path) - self.tree = tree if tree is not None else WorkingTree(repo_path) + self.tree = tree if tree is not None else WorkingTree(repo_path, store=self.store) def _format_commit(self, tree_hash, parents, author, message) -> str: """Format commit object as a string""" @@ -208,13 +208,23 @@ def read_commit(self, commit_hash: str) -> CommitData: parents.append(lines[i][len("parent ") :]) i += 1 if not lines[i].startswith("author "): - raise ValueError("mising author line") + raise ValueError("missing author line") author = lines[i][len("author ") :].rsplit(" ", 1)[0] i += 1 if not lines[i].startswith("committer "): raise ValueError("missing committer line") committer = lines[i][len("committer ") :].rsplit(" ", 1)[0] + if i != len(lines) - 1: + raise ValueError("unexpected commit headers") + for object_hash in [tree, *parents]: + if len(object_hash) != 40 or any(c not in "0123456789abcdef" for c in object_hash): + raise ValueError("invalid object hash") + for identity_line in lines[-2:]: + identity, timestamp = identity_line.split(" ", 1)[1].rsplit(" ", 1) + if not identity.strip(): + raise ValueError("missing identity") + int(timestamp) except (UnicodeDecodeError, IndexError, ValueError) as error: raise ObjectCorruptError(commit_hash) from error diff --git a/tests/test_commits.py b/tests/test_commits.py index e015456..a6dd70c 100644 --- a/tests/test_commits.py +++ b/tests/test_commits.py @@ -301,3 +301,20 @@ def test_fresh_manager_reads_same_history(tmp_path): m1.create_commit(tree, [a], AUTHOR, "b") m2 = make_manager(tmp_path) assert m2.log() == m1.log() + + +@pytest.mark.parametrize( + "header", + [ + "tree invalid\nauthor Test 1\ncommitter Test 1", + f"tree {'a' * 40}\nparent invalid\nauthor Test 1\ncommitter Test 1", + f"tree {'a' * 40}\nauthor Test\ncommitter Test 1", + f"tree {'a' * 40}\nauthor Test 1\ncommitter Test invalid", + f"tree {'a' * 40}\nauthor Test 1\ncommitter Test 1\nextra header", + ], +) +def test_read_commit_rejects_invalid_headers(tmp_path, header): + manager = make_manager(tmp_path) + obj_hash = manager.store.write_object((header + "\n\nmessage").encode(), "commit") + with pytest.raises(ObjectCorruptError): + manager.read_commit(obj_hash)