From 38c34edc2107f01cf06ed9c08b04d5abb5e71753 Mon Sep 17 00:00:00 2001 From: "Myeongjin (Daniel)" Date: Thu, 22 Oct 2026 02:01:18 -0400 Subject: [PATCH 1/2] Integrate a fetched commit into a local branch through the existing merge engine. --- minigit/commits.py | 134 +++++++++++++++++++++-- tests/test_commits.py | 244 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 372 insertions(+), 6 deletions(-) diff --git a/minigit/commits.py b/minigit/commits.py index 83f8330..d8486a6 100644 --- a/minigit/commits.py +++ b/minigit/commits.py @@ -10,6 +10,7 @@ """ import os +import tempfile import time from dataclasses import dataclass @@ -183,16 +184,70 @@ def read_ref(self, name: str) -> str | None: with open(ref_path) as f: return f.read().strip() - def write_ref(self, name: str, commit_hash: str) -> None: - """Point a branch's ref file at the given commit hash, creating refs/heads/ if needed""" - os.makedirs(self._refs_dir(), exist_ok=True) - with open(self._ref_path(name), "w") as f: - f.write(f"{commit_hash}\n") + def validate_branch_name(self, name: str) -> None: + """Reject a branch name that escapes refs/heads or breaks the ref line.""" + if ( + not name + or any(part in {"", ".", ".."} for part in name.split("/")) + or any(char in name for char in "\0\\\r\n") + ): + raise MiniGitError(f"invalid branch name: {name!r}") + + def _validate_fetched_history(self, commit_hash: str) -> None: + """Require every commit, tree, and blob reachable from commit_hash. + + Both parents are walked. A missing or corrupt object raises before a ref + or the work tree is updated. + """ + visited_trees: set[str] = set() + for current in self.walk_history(commit_hash): + commit = self.read_commit(current) + self._validate_fetched_tree(commit.tree, visited_trees) + + def _validate_fetched_tree(self, tree_hash: str, visited: set[str]) -> None: + """Check one tree and everything stored under it, once.""" + if tree_hash in visited: + return + visited.add(tree_hash) + for entry in self.store.read_tree(tree_hash): + if entry.type == "tree": + self._validate_fetched_tree(entry.hash, visited) + else: + obj_type, _ = self.store.read_object(entry.hash) + if obj_type != "blob": + raise ObjectCorruptError(entry.hash) def _ref_path(self, name: str) -> str: - """Return the file path for a single branch's ref""" + """Return the file path for a single branch's ref.""" + self.validate_branch_name(name) return os.path.join(self._refs_dir(), name) + def write_ref(self, name: str, commit_hash: str) -> None: + """Point a branch ref at commit_hash. + + The new contents are written to a temporary file in the ref directory + and then published with os.replace. A failed publish leaves the previous + ref readable and removes the temporary file. + """ + ref_path = self._ref_path(name) + os.makedirs(os.path.dirname(ref_path), exist_ok=True) + temporary_path = None + try: + with tempfile.NamedTemporaryFile( + mode="w", + dir=os.path.dirname(ref_path), + prefix=f".{os.path.basename(ref_path)}.", + delete=False, + ) as temporary_file: + temporary_path = temporary_file.name + temporary_file.write(f"{commit_hash}\n") + temporary_file.flush() + os.replace(temporary_path, ref_path) + temporary_path = None + finally: + if temporary_path is not None: + os.unlink(temporary_path) + def _head_path(self) -> str: """Return path of file that is the current branch""" return os.path.join(self.root, ".minigit", "HEAD") @@ -376,6 +431,73 @@ def merge(self, branch_name) -> str | None: return self._three_way_merge(current_hash, target, branch_name) + def integrate_fetched_tip(self, branch_name: str, commit_hash: str) -> str | None: + """Integrate one fetched commit into a named local branch. + + Inputs: + branch_name: the local branch to update. + commit_hash: the fetched commit, already stored in this repo. + + Returns: + None when the ref is created, fast-forwarded, or left unchanged. + The new commit hash when the checked-out branch has a clean merge. + + Errors: + MiniGitError for an invalid branch name, a merge already in progress, + staged changes on a checked-out update, unrelated history, multiple + merge bases, an unsupported conflict, or a diverged branch that is + not checked out. MergeConflictError for a text conflict on the + checked-out branch. ObjectNotFoundError or ObjectCorruptError when + the fetched history is missing or corrupt. + + Local state: + Checked-out unborn branch, or local tip is an ancestor of the fetched + commit: check out the fetched tree, then move that branch ref. HEAD + stays on the branch. + Checked-out equal tips, or fetched commit is an ancestor of the local + tip: change nothing. + Checked-out divergence: the existing three-way merge. A clean result + has parents [local tip, fetched tip]. A text conflict leaves both + tips in place and saves both parents and the unresolved paths. + Absent branch that is not checked out: create its ref after history + validation. HEAD, files, and the index stay as they are. + Existing branch that is not checked out: fast-forward that ref, or + leave it, without changing HEAD, files, or the index. + Diverged branch that is not checked out: raise, with an instruction + to check out that branch and pull again. + """ + self.validate_branch_name(branch_name) + if self._read_pending_merge() is not None: + raise MiniGitError("merge in progress") + self._validate_fetched_history(commit_hash) + + local_hash = self.read_ref(branch_name) + checked_out = branch_name == self._current_branch() + + if checked_out: + if local_hash is not None and ( + commit_hash == local_hash or self.is_ancestor(commit_hash, local_hash) + ): + return None + if local_hash is None or self.is_ancestor(local_hash, commit_hash): + self._require_no_staged_changes() + self.tree.checkout(self.read_commit(commit_hash).tree) + self.write_ref(branch_name, commit_hash) + return None + return self._three_way_merge(local_hash, commit_hash, branch_name) + + if local_hash is None: + self.write_ref(branch_name, commit_hash) + return None + if commit_hash == local_hash or self.is_ancestor(commit_hash, local_hash): + return None + if self.is_ancestor(local_hash, commit_hash): + self.write_ref(branch_name, commit_hash) + return None + raise MiniGitError( + f"branch '{branch_name}' has diverged; check out that branch and pull again" + ) + def find_merge_base(self, ours: str, theirs: str) -> str: """Return the single lowest common ancestor of two commits.""" common = set(self.walk_history(ours)) & set(self.walk_history(theirs)) diff --git a/tests/test_commits.py b/tests/test_commits.py index 8aa2653..d757bc7 100644 --- a/tests/test_commits.py +++ b/tests/test_commits.py @@ -904,3 +904,247 @@ def test_cli_conflict_then_resolve_add_commit(tmp_path, monkeypatch): commit = fresh.read_commit(fresh.read_ref("main")) assert commit.parents == [ours, theirs] assert not (tmp_path / ".minigit" / "MERGE_HEAD").exists() + + +# fetched-tip integration + + +def test_integrate_unborn_checks_out_then_advances_ref(tmp_path): + """An unborn checked-out branch gets the fetched tree, then the ref.""" + m = make_manager(tmp_path) + tree = make_tree(m) + tip = m.create_commit(tree, [], AUTHOR, "fetched") + (tmp_path / ".minigit" / "refs" / "heads" / "main").unlink() + assert m.integrate_fetched_tip("main", tip) is None + assert m.read_ref("main") == tip + assert m._current_branch() == "main" + assert m.tree.checked_out == [tree] + + +def test_integrate_equal_tips_change_nothing(tmp_path): + """The same tip does not move the ref or check out.""" + m = make_manager(tmp_path) + tip = m.create_commit(make_tree(m), [], AUTHOR, "same") + m.write_head("main") + assert m.integrate_fetched_tip("main", tip) is None + assert m.read_ref("main") == tip + assert m.tree.checked_out == [] + + +def test_integrate_remote_ahead_fast_forwards(tmp_path): + """A downloaded descendant is checked out, then the current ref moves to it.""" + m = make_manager(tmp_path) + tree = make_tree(m) + local = m.create_commit(tree, [], AUTHOR, "local") + remote = m.create_commit(tree, [local], AUTHOR, "remote") + m.write_ref("main", local) + m.write_head("main") + assert m.integrate_fetched_tip("main", remote) is None + assert m.read_ref("main") == remote + assert m.read_head() == "main" + assert m.tree.checked_out == [tree] + + +def test_integrate_local_ahead_is_a_no_op(tmp_path): + """A downloaded ancestor stays behind the local tip.""" + m = make_manager(tmp_path) + tree = make_tree(m) + ancestor = m.create_commit(tree, [], AUTHOR, "ancestor") + local = m.create_commit(tree, [ancestor], AUTHOR, "local") + m.write_head("main") + assert m.integrate_fetched_tip("main", ancestor) is None + assert m.read_ref("main") == local + assert m.tree.checked_out == [] + + +def test_integrate_clean_divergence_commits_local_then_fetched(tmp_path): + """A clean split writes one merge commit with parents [local, fetched].""" + m = make_manager(tmp_path) + local, fetched = setup_diverged( + m, + {"shared.txt": "base\n"}, + {"shared.txt": "base\n", "ours.txt": "ours\n"}, + {"shared.txt": "base\n", "theirs.txt": "theirs\n"}, + ) + merge_hash = m.integrate_fetched_tip("main", fetched) + assert m.read_commit(merge_hash).parents == [local, fetched] + assert m.read_ref("main") == merge_hash + assert m.read_head() == "main" + + +def test_integrate_text_conflict_keeps_both_tips(tmp_path): + """A same-line conflict saves the merge and leaves both tips in place.""" + m = make_manager(tmp_path) + local, fetched = setup_diverged( + m, + {"file.txt": "x\n"}, + {"file.txt": "ours\n"}, + {"file.txt": "theirs\n"}, + ) + with pytest.raises(MergeConflictError) as info: + m.integrate_fetched_tip("main", fetched) + assert info.value.paths == ["file.txt"] + assert m.read_ref("main") == local + assert m.read_ref("feature") == fetched + assert (tmp_path / ".minigit" / "MERGE_HEAD").read_text().strip() == fetched + assert (tmp_path / ".minigit" / "MERGE_OURS").read_text().strip() == local + + +def test_integrate_absent_branch_creates_ref_only(tmp_path): + """A missing branch that is not checked out is created without checkout.""" + m = make_manager(tmp_path) + tree = make_tree(m) + tip = m.create_commit(tree, [], AUTHOR, "main") + m.write_head("main") + assert m.integrate_fetched_tip("feature", tip) is None + assert m.read_ref("feature") == tip + assert m.read_ref("main") == tip + assert m.read_head() == "main" + assert m.tree.checked_out == [] + + +def test_integrate_inactive_fast_forward_does_not_touch_head_or_files(tmp_path): + """Another branch moves forward without checkout, HEAD, or index changes.""" + m = make_manager(tmp_path) + tree = make_tree(m) + older = m.create_commit(tree, [], AUTHOR, "older") + m.create_branch("feature", older) + newer = m.create_commit(tree, [older], AUTHOR, "newer") + m.write_head("main") + assert m.integrate_fetched_tip("feature", newer) is None + assert m.read_ref("feature") == newer + assert m.read_ref("main") == newer + assert m.read_head() == "main" + assert m.tree.checked_out == [] + + +def test_integrate_inactive_divergence_rejects_without_local_changes(tmp_path): + """A split branch that is not checked out is refused, and local work stays.""" + m = make_manager(tmp_path) + local, fetched = setup_diverged( + m, + {"file.txt": "base\n"}, + {"file.txt": "ours\n"}, + {"file.txt": "theirs\n"}, + ) + with pytest.raises(MiniGitError, match="check out that branch and pull again"): + m.integrate_fetched_tip("feature", local) + assert m.read_ref("main") == local + assert m.read_ref("feature") == fetched + assert m.read_head() == "main" + assert m.tree.checked_out == [] + assert not (tmp_path / ".minigit" / "MERGE_HEAD").exists() + + +def test_integrate_fetched_merge_uses_second_parent_as_base(tmp_path): + """A fetched merge whose second parent is the base is walked once per commit.""" + m = make_manager(tmp_path) + base = commit_tree_files(m, {"shared.txt": "base\n"}, "base") + m.create_branch("side", base) + local = commit_tree_files(m, {"shared.txt": "base\n", "local.txt": "L\n"}, "local") + m.write_head("side") + side = commit_tree_files(m, {"shared.txt": "base\n", "side.txt": "S\n"}, "side") + side_tree = m.read_commit(side).tree + m.write_head("main") + m.tree.index_tree = m.read_commit(local).tree + fetched = m.store.write_object( + m._format_commit(side_tree, [side, base], AUTHOR, "fetched-merge").encode(), + "commit", + ) + assert m.find_merge_base(local, fetched) == base + merge_hash = m.integrate_fetched_tip("main", fetched) + assert m.read_commit(merge_hash).parents == [local, fetched] + history = m.walk_history(merge_hash) + assert history.count(base) == 1 + assert set(history) == {merge_hash, local, fetched, side, base} + + +def test_integrate_missing_history_does_not_move_ref(tmp_path): + """A missing fetched commit leaves the existing ref in place.""" + m = make_manager(tmp_path) + local = m.create_commit(make_tree(m), [], AUTHOR, "local") + with pytest.raises(ObjectNotFoundError): + m.integrate_fetched_tip("main", "ab" * 20) + assert m.read_ref("main") == local + assert not (tmp_path / ".minigit" / "refs" / "heads" / "feature").exists() + + +def test_integrate_corrupt_history_does_not_move_ref(tmp_path): + """A blob passed off as a commit does not create or move a ref.""" + m = make_manager(tmp_path) + local = m.create_commit(make_tree(m), [], AUTHOR, "local") + blob = m.store.write_object(b"not a commit", "blob") + with pytest.raises(ObjectCorruptError): + m.integrate_fetched_tip("feature", blob) + assert m.read_ref("main") == local + assert m.read_ref("feature") is None + + +def test_integrate_rejects_invalid_branch_names(tmp_path): + """Traversal, absolute paths, and protocol control characters are refused.""" + m = make_manager(tmp_path) + tip = m.create_commit(make_tree(m), [], AUTHOR, "tip") + for name in ["", "..", "../x", "/abs", "a/../../b", "bad\nname", "bad\0name", "a\\b"]: + with pytest.raises(MiniGitError): + m.integrate_fetched_tip(name, tip) + assert m.read_ref("main") == tip + assert sorted(path.name for path in (tmp_path / ".minigit" / "refs" / "heads").iterdir()) == [ + "main" + ] + + +def test_failed_ref_publish_leaves_old_ref_readable(tmp_path, monkeypatch): + """A failed os.replace keeps the previous ref text.""" + m = make_manager(tmp_path) + tree = make_tree(m) + local = m.create_commit(tree, [], AUTHOR, "local") + remote = m.create_commit(tree, [local], AUTHOR, "remote") + m.write_ref("main", local) + + def fail_replace(src, dst): + """Simulate a publish that does not reach the ref file.""" + raise OSError("replace failed") + + monkeypatch.setattr("minigit.commits.os.replace", fail_replace) + with pytest.raises(OSError, match="replace failed"): + m.integrate_fetched_tip("main", remote) + ref = tmp_path / ".minigit" / "refs" / "heads" / "main" + assert ref.read_text().strip() == local + + +def test_pending_merge_blocks_integrate(tmp_path): + """A second integration is refused while conflict state is still saved.""" + m = make_manager(tmp_path) + local, fetched = setup_diverged( + m, + {"file.txt": "x\n"}, + {"file.txt": "ours\n"}, + {"file.txt": "theirs\n"}, + ) + with pytest.raises(MergeConflictError): + m.integrate_fetched_tip("main", fetched) + with pytest.raises(MiniGitError, match="merge in progress"): + m.integrate_fetched_tip("main", fetched) + assert m.read_ref("main") == local + assert m.read_ref("feature") == fetched + + +def test_integrate_conflict_resolves_across_fresh_processes(tmp_path, monkeypatch): + """Resolve, add, and commit in a new process keep parents [local, fetched].""" + monkeypatch.chdir(tmp_path) + m = make_real_manager(tmp_path) + base = commit_file(m, "file.txt", "x\n", "base") + m.create_branch("feature", base) + local = commit_file(m, "file.txt", "ours\n", "ours") + m.switch_branch("feature") + fetched = commit_file(m, "file.txt", "theirs\n", "theirs") + m.switch_branch("main") + with pytest.raises(MergeConflictError): + CommitManager(str(tmp_path)).integrate_fetched_tip("main", fetched) + assert main(["commit", "-m", "too-soon"]) == 1 + (tmp_path / "file.txt").write_text("resolved\n") + assert main(["add", "file.txt"]) == 0 + assert main(["commit", "-m", "unused"]) == 0 + fresh = CommitManager(str(tmp_path)) + assert fresh.read_commit(fresh.read_ref("main")).parents == [local, fetched] + assert not (tmp_path / ".minigit" / "MERGE_HEAD").exists() From 2bc73acce88900f64b453e85de87f70cf8790939 Mon Sep 17 00:00:00 2001 From: aman shah Date: Sun, 25 Oct 2026 19:39:54 -0400 Subject: [PATCH 2/2] Validate ref paths and list nested branches safely --- minigit/commits.py | 42 ++++++++++++++++++++++++++++++++++-------- tests/test_commits.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/minigit/commits.py b/minigit/commits.py index d8486a6..1448ebd 100644 --- a/minigit/commits.py +++ b/minigit/commits.py @@ -184,12 +184,14 @@ def read_ref(self, name: str) -> str | None: with open(ref_path) as f: return f.read().strip() - def validate_branch_name(self, name: str) -> None: + @staticmethod + def validate_branch_name(name: str) -> None: """Reject a branch name that escapes refs/heads or breaks the ref line.""" if ( not name - or any(part in {"", ".", ".."} for part in name.split("/")) - or any(char in name for char in "\0\\\r\n") + or any(not part or part.startswith(".") for part in name.split("/")) + or any(char.isspace() or ord(char) < 32 or ord(char) == 127 for char in name) + or "\\" in name ): raise MiniGitError(f"invalid branch name: {name!r}") @@ -220,7 +222,18 @@ def _validate_fetched_tree(self, tree_hash: str, visited: set[str]) -> None: def _ref_path(self, name: str) -> str: """Return the file path for a single branch's ref.""" self.validate_branch_name(name) - return os.path.join(self._refs_dir(), name) + path = self.root + parts = [".minigit", "refs", "heads", *name.split("/")] + for index, part in enumerate(parts): + path = os.path.join(path, part) + if os.path.islink(path): + raise MiniGitError(f"symlink in ref path: {name!r}") + if os.path.exists(path): + if index < len(parts) - 1 and not os.path.isdir(path): + raise MiniGitError(f"ref path is blocked: {name!r}") + if index == len(parts) - 1 and not os.path.isfile(path): + raise MiniGitError(f"ref path is not a file: {name!r}") + return path def write_ref(self, name: str, commit_hash: str) -> None: """Point a branch ref at commit_hash. @@ -266,6 +279,7 @@ def read_head(self) -> str | None: def write_head(self, name: str) -> None: """Point HEAD at the given branch.""" + self._ref_path(name) with open(self._head_path(), "w") as f: f.write(f"ref: refs/heads/{name}\n") @@ -399,10 +413,22 @@ def create_branch(self, name, commit_hash) -> None: self.write_ref(name, commit_hash) def list_branches(self) -> list[str]: - if not os.path.isdir(self._refs_dir()): - return [] - else: - return sorted(os.listdir(self._refs_dir())) + branches = [] + for directory, dirnames, filenames in os.walk(self._refs_dir()): + dirnames[:] = [ + name + for name in dirnames + if not name.startswith(".") and not os.path.islink(os.path.join(directory, name)) + ] + for filename in filenames: + name = os.path.relpath(os.path.join(directory, filename), self._refs_dir()) + name = name.replace(os.sep, "/") + try: + self._ref_path(name) + except MiniGitError: + continue + branches.append(name) + return sorted(branches) def merge(self, branch_name) -> str | None: """Fast-forward or three-way merge `branch_name` into the current branch. diff --git a/tests/test_commits.py b/tests/test_commits.py index d757bc7..8138c0f 100644 --- a/tests/test_commits.py +++ b/tests/test_commits.py @@ -1148,3 +1148,37 @@ def test_integrate_conflict_resolves_across_fresh_processes(tmp_path, monkeypatc fresh = CommitManager(str(tmp_path)) assert fresh.read_commit(fresh.read_ref("main")).parents == [local, fetched] assert not (tmp_path / ".minigit" / "MERGE_HEAD").exists() + + +@pytest.mark.parametrize("name", ["bad name", "bad\tname", "bad\x1bname", "bad\x7fname"]) +def test_ref_names_reject_protocol_whitespace_and_controls(tmp_path, name): + manager = make_manager(tmp_path) + with pytest.raises(MiniGitError, match="invalid branch name"): + manager.write_ref(name, "a" * 40) + assert manager.list_branches() == [] + + +def test_nested_branches_are_listed_without_temporary_files(tmp_path): + manager = make_manager(tmp_path) + tip = manager.create_commit(make_tree(manager), [], AUTHOR, "base") + manager.create_branch("feature/topic", tip) + (tmp_path / ".minigit/refs/heads/.main.pending").write_text(tip) + assert manager.list_branches() == ["feature/topic", "main"] + assert manager.read_ref("feature/topic") == tip + + +def test_ref_paths_reject_symlinks_and_file_directory_collisions(tmp_path): + manager = make_manager(tmp_path) + tip = manager.create_commit(make_tree(manager), [], AUTHOR, "base") + outside = tmp_path / "outside" + outside.mkdir() + (outside / "topic").write_text("untouched") + (tmp_path / ".minigit/refs/heads/escape").symlink_to(outside, target_is_directory=True) + for name in ["escape/topic", "main/topic"]: + with pytest.raises(MiniGitError): + manager.write_ref(name, tip) + manager.create_branch("feature/topic", tip) + with pytest.raises(MiniGitError): + manager.read_ref("feature") + assert (outside / "topic").read_text() == "untouched" + assert manager.list_branches() == ["feature/topic", "main"]