From 8af08408d1eb2ca391c6403243589e4c332b69d7 Mon Sep 17 00:00:00 2001 From: Shuhan6017 Date: Sat, 10 Oct 2026 20:55:13 -0400 Subject: [PATCH 1/3] week 5 --- minigit/remote.py | 158 +++++++++++++++++------ tests/test_remote.py | 289 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 405 insertions(+), 42 deletions(-) diff --git a/minigit/remote.py b/minigit/remote.py index 566ef2f..883ad6f 100644 --- a/minigit/remote.py +++ b/minigit/remote.py @@ -65,6 +65,33 @@ def recv_exact(sock, buf: bytearray, size: int) -> bytes: return data +def _valid_hash(value: str) -> bool: + """Return whether `value` is a full 40-character lowercase hex object hash.""" + + return len(value) == 40 and all(c in "0123456789abcdef" for c in value) + + +def _valid_branch(name: str) -> bool: + """Return whether `name` is safe to use as a ref path under `refs/heads/`. + + Rejects empty names, `.`/`..`/empty path segments (traversal), and + characters that would break a ref file or a protocol line. + """ + + return ( + bool(name) + and all(part not in {"", ".", ".."} for part in name.split("/")) + and not any(char in name for char in "\0\\\r\n ") + ) + + +def _rejection_reason(reply: str) -> str: + """Strip the `ERR ` prefix off a server reply so it reads well in a CLI error.""" + + command, _, reason = reply.partition(" ") + return reason if command == "ERR" and reason else reply + + def _has_object(store, obj_hash: str) -> bool: """Return whether `obj_hash` already exists in `store`. @@ -131,12 +158,14 @@ def push(self, remote_address: str, branch: str, token: str) -> None: """Send local commits on `branch` to the remote, rejecting if it has diverged. Resolves the local tip and authenticates, then reads the remote's - current tip through `REF`. Equal tips mean nothing to do; a remote - tip that is not an ancestor of the local tip means someone else - pushed first, and the push is rejected. Otherwise every object - reachable from `branch` that the remote doesn't already have is + current tip through `REF`. Equal tips mean nothing to do. A remote + tip that is not an ancestor of the local tip (unknown locally, ahead + of local, or diverged) means someone else pushed first, and the push + is rejected before anything is uploaded. Otherwise every object + reachable from the local tip that the remote doesn't already have is uploaded (`HAVE`/`PUT`), and the ref only moves after `DONE` reports - the transfer validated on the far side. + the transfer validated on the far side - the server rechecks the + expected old tip there too, so a push that raced ours still loses. """ host, port = self._parse_address(remote_address) @@ -165,27 +194,25 @@ def push(self, remote_address: str, branch: str, token: str) -> None: if len(fields) != 3 or fields[:2] != ["REF", branch]: raise NetworkProtocolError(f"invalid REF response: {reply!r}") remote_hash = fields[2] - if remote_hash != "-" and ( - len(remote_hash) != 40 or any(c not in "0123456789abcdef" for c in remote_hash) - ): + if remote_hash != "-" and not _valid_hash(remote_hash): raise NetworkProtocolError(f"invalid remote hash: {remote_hash!r}") if remote_hash == local_hash: + send_line(sock, "DONE") print(f"{branch} is up to date") return - if remote_hash != "-" and not _is_ancestor(self.commits, remote_hash, local_hash): - raise NetworkProtocolError( - f"remote {branch} has diverged from local: " - f"{remote_hash} is not an ancestor of {local_hash}" - ) + if remote_hash != "-": + self._check_fast_forward(branch, remote_hash, local_hash) send_line(sock, f"PUSH {branch} {remote_hash} {local_hash}") reply = receive_line(sock, buf) if reply != "OK": - raise NetworkProtocolError(f"push rejected: {reply}") + raise NetworkProtocolError(f"push rejected: {_rejection_reason(reply)}") - for obj_hash in self.collect_reachable(branch): + # Pinned to `local_hash`, not re-read from the ref: a local commit + # landing mid-push must not change the snapshot we proposed. + for obj_hash in self._collect_from(local_hash): send_line(sock, f"HAVE {obj_hash}") reply = receive_line(sock, buf) if reply == "NO": @@ -196,7 +223,7 @@ def push(self, remote_address: str, branch: str, token: str) -> None: send_line(sock, "DONE") reply = receive_line(sock, buf) if reply != "OK": - raise NetworkProtocolError(f"push failed: {reply}") + raise NetworkProtocolError(f"push rejected: {_rejection_reason(reply)}") print(f"{branch} now at {local_hash}") except NetworkProtocolError: @@ -206,6 +233,29 @@ def push(self, remote_address: str, branch: str, token: str) -> None: finally: sock.close() + def _check_fast_forward(self, branch: str, remote_hash: str, local_hash: str) -> None: + """Raise unless moving the remote from `remote_hash` to `local_hash` is a fast-forward. + + Ancestry comes from M3's history walk, which follows every parent of + a merge commit, so a remote tip reachable only through a merge's + second parent still counts as an ancestor. + """ + + if _is_ancestor(self.commits, remote_hash, local_hash): + return + if not _has_object(self.store, remote_hash): + raise NetworkProtocolError( + f"push rejected: remote {branch} is at {remote_hash[:7]}, " + "which you don't have locally - pull first" + ) + if _is_ancestor(self.commits, local_hash, remote_hash): + raise NetworkProtocolError( + f"push rejected: remote {branch} is ahead of local - pull first" + ) + raise NetworkProtocolError( + f"push rejected: local and remote {branch} have diverged - pull and merge first" + ) + def _put_object(self, sock, buf: bytearray, obj_hash: str) -> None: """Send `PUT ` followed by the object's `OBJ` header and bytes.""" @@ -304,15 +354,15 @@ def _receive_object(self, sock, buf: bytearray, expected_hash: str) -> None: self.store.write_object(content, obj_type) def collect_reachable(self, branch: str) -> set[str]: - """Return every commit, tree, and blob hash reachable from `branch`'s tip. - - Local-only this week: this is what push will later diff against the - remote's advertised hash to find what's actually missing there. - """ + """Return every commit, tree, and blob hash reachable from `branch`'s tip.""" tip = self.commits.read_ref(branch) if tip is None: return set() + return self._collect_from(tip) + + def _collect_from(self, tip: str) -> set[str]: + """Return every commit, tree, and blob hash reachable from commit `tip`.""" reachable: set[str] = set() for commit_hash in self.commits.walk_history(tip): @@ -337,12 +387,34 @@ def _collect_tree(self, tree_hash: str, reachable: set[str]) -> None: class RemoteServer: - """Accepts a RemoteClient's AUTH + REF handshake over TCP, one client at a time.""" + """Serves one repo's refs and objects to `RemoteClient`s over TCP, one client at a time. + + Sessions are handled strictly one after another, which is what makes a + push's final "recheck ref -> validate history -> write ref" sequence + atomic with respect to other pushes: no second session can run between + the check and the write. If `serve_forever` is ever made concurrent, that + sequence in `_handle_push` must be wrapped in a lock. (Writes made to the + served repo outside this server, e.g. a local commit, aren't covered.) + + Each accepted connection gets `conn_timeout` seconds per read/write, so a + stalled client can't hold the server - and every client queued behind + it - forever. + """ - def __init__(self, repo_path=".", token="", host="127.0.0.1", port=0, store=None, commits=None): + def __init__( + self, + repo_path=".", + token="", + host="127.0.0.1", + port=0, + store=None, + commits=None, + conn_timeout=10.0, + ): self.repo_path = repo_path self.token = token self.host = host + self.conn_timeout = conn_timeout self.store = store if store is not None else ObjectStore(repo_path) self.commits = ( commits if commits is not None else CommitManager(repo_path, store=self.store) @@ -377,6 +449,7 @@ def serve_forever(self) -> None: return # listening socket was closed - shut down try: + conn.settimeout(self.conn_timeout) self._handle_client(conn) except (ObjectNotFoundError, ObjectCorruptError) as exc: try: @@ -420,11 +493,7 @@ def _handle_client(self, conn) -> None: def _send_ref(self, conn, branch: str) -> None: """Reply with the commit hash `branch` currently points at, or `-` if it has none.""" - if ( - not branch - or any(part in {"", ".", ".."} for part in branch.split("/")) - or any(char in branch for char in "\0\\\r\n") - ): + if not _valid_branch(branch): send_line(conn, "ERR invalid branch") return commit_hash = self.commits.read_ref(branch) or "-" @@ -445,20 +514,19 @@ def _handle_push(self, conn, buf: bytearray, args: str) -> None: send_line(conn, "ERR malformed PUSH") return - if ( - not branch - or any(part in {"", ".", ".."} for part in branch.split("/")) - or any(char in branch for char in "\0\\\r\n") - ): + if not _valid_branch(branch): send_line(conn, "ERR invalid branch") return - if len(new_hash) != 40 or any(c not in "0123456789abcdef" for c in new_hash): + if old_hash != "-" and not _valid_hash(old_hash): + send_line(conn, "ERR invalid old hash") + return + if not _valid_hash(new_hash): send_line(conn, "ERR invalid new hash") return expected_old = None if old_hash == "-" else old_hash if self.commits.read_ref(branch) != expected_old: - send_line(conn, f"ERR {branch} changed: expected {old_hash}") + send_line(conn, f"ERR remote {branch} has moved since you last synced - pull first") return send_line(conn, "OK") @@ -468,6 +536,9 @@ def _handle_push(self, conn, buf: bytearray, args: str) -> None: command, _, value = line.partition(" ") if command == "HAVE": + if not _valid_hash(value): + send_line(conn, "ERR invalid object hash") + return send_line(conn, "YES" if _has_object(self.store, value) else "NO") elif command == "PUT": if not self._receive_pushed_object(conn, buf, value): @@ -478,10 +549,11 @@ def _handle_push(self, conn, buf: bytearray, args: str) -> None: send_line(conn, "ERR expected HAVE, PUT, or DONE") return - # Recheck the ref hasn't moved since we accepted the PUSH - another - # client's push could have landed while we were receiving objects. + # Recheck the ref hasn't moved since we accepted the PUSH. From here + # to write_ref below nothing else can move it: sessions are serial + # (see the class docstring), so check + write act as one step. if self.commits.read_ref(branch) != expected_old: - send_line(conn, "ERR ref changed during push") + send_line(conn, f"ERR remote {branch} changed during push - pull first") return try: @@ -505,7 +577,7 @@ def _receive_pushed_object(self, conn, buf: bytearray, expected_hash: str) -> bo session on failure. """ - if len(expected_hash) != 40 or any(c not in "0123456789abcdef" for c in expected_hash): + if not _valid_hash(expected_hash): send_line(conn, "ERR invalid object hash") return False @@ -559,7 +631,7 @@ def _validate_tree(self, tree_hash: str, visited: set[str]) -> None: def _send_object(self, conn, obj_hash: str) -> None: """Reply with the requested object's bytes, or `ERR` if it isn't in the store.""" - if len(obj_hash) != 40 or any(c not in "0123456789abcdef" for c in obj_hash): + if not _valid_hash(obj_hash): send_line(conn, "ERR invalid object hash") return try: @@ -592,6 +664,12 @@ def _send_object(self, conn, obj_hash: str) -> None: # (propose the move) -> repeated HAVE/PUT (upload only what the remote is # missing) -> DONE (remote validates the new history and moves the ref) -> # connection closes. +# +# Conflict detection happens twice. The client refuses before PUSH if the +# remote tip from REF isn't an ancestor of its local tip. The server +# refuses PUSH if isn't its current ref, and again at DONE if the ref +# moved, history is incomplete, or doesn't descend from . Any +# refusal is `ERR ` and leaves the ref where it was. def register_subcommands(subparsers) -> None: diff --git a/tests/test_remote.py b/tests/test_remote.py index 97be842..a4d4b01 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -7,6 +7,7 @@ import pytest +from minigit.cli import main from minigit.commits import CommitManager from minigit.errors import NetworkProtocolError from minigit.objects import ObjectStore @@ -326,11 +327,12 @@ def test_push_disconnect_before_done_leaves_ref_unchanged(push_pair): def test_push_rejects_unrelated_remote_tip(push_pair): pair = push_pair unrelated = _commit(pair.remote_store, pair.remote_commits, {"other.txt": b"x"}, []) - _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + local_tip = _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) - with pytest.raises(NetworkProtocolError): + with pytest.raises(NetworkProtocolError, match="don't have locally - pull first"): pair.client.push(pair.address, "main", "tok") assert pair.remote_commits.read_ref("main") == unrelated + assert pair.local_commits.read_ref("main") == local_tip def test_push_rejects_ref_changed_during_transfer(push_pair): @@ -734,3 +736,286 @@ def close(self): monkeypatch.setattr(socket, "create_connection", lambda *args, **kwargs: sock) with pytest.raises(NetworkProtocolError, match="invalid"): RemoteClient(tmp_path).push("localhost:9418", "main", "tok") + + +# --- push conflict detection (#34) --- + + +def _second_client(tmp_path, pair: PushPair) -> RemoteClient: + """Another independent local repo, pointed at the same remote as `pair.client`.""" + + root = tmp_path / "local2" + root.mkdir() + store = ObjectStore(str(root)) + return RemoteClient( + repo_path=str(root), store=store, commits=CommitManager(str(root), store=store) + ) + + +def _count_puts(monkeypatch) -> list: + """Record every object `RemoteClient` uploads, while still uploading it.""" + + calls = [] + original_put = RemoteClient._put_object + + def counting_put(self, sock, buf, obj_hash): + calls.append(obj_hash) + return original_put(self, sock, buf, obj_hash) + + monkeypatch.setattr(RemoteClient, "_put_object", counting_put) + return calls + + +def _open_push(pair: PushPair, old: str, new: str) -> tuple[socket.socket, bytearray]: + """Authenticate a raw connection and send `PUSH main `; returns the reply buffer.""" + + sock = socket.create_connection(("127.0.0.1", pair.server.port), timeout=5) + buf = bytearray() + send_line(sock, "AUTH tok") + assert receive_line(sock, buf) == "OK" + send_line(sock, f"PUSH main {old} {new}") + return sock, buf + + +def _upload(sock, buf: bytearray, store: ObjectStore, hashes) -> None: + """Raw-protocol HAVE/PUT for each hash, uploading whatever the remote is missing.""" + + for obj_hash in hashes: + send_line(sock, f"HAVE {obj_hash}") + if receive_line(sock, buf) == "NO": + obj_type, content = store.read_object(obj_hash) + send_line(sock, f"PUT {obj_hash}") + send_line(sock, f"OBJ {obj_type} {len(content)}") + sock.sendall(content) + assert receive_line(sock, buf) == "OK" + + +def test_push_fast_forward_then_repeat_transfers_nothing(push_pair, monkeypatch, capsys): + pair = push_pair + c1 = _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + pair.client.push(pair.address, "main", "tok") + c2 = _commit(pair.local_store, pair.local_commits, {"a.txt": b"2"}, [c1]) + pair.client.push(pair.address, "main", "tok") + assert pair.remote_commits.read_ref("main") == c2 + + puts = _count_puts(monkeypatch) + capsys.readouterr() + pair.client.push(pair.address, "main", "tok") + assert puts == [] + assert "up to date" in capsys.readouterr().out + assert pair.remote_commits.read_ref("main") == c2 + + +def test_push_accepts_ancestor_only_via_second_merge_parent(push_pair): + pair = push_pair + remote_tip = _commit(pair.local_store, pair.local_commits, {"a.txt": b"a"}, []) + pair.client.push(pair.address, "main", "tok") + + # `other` is an unrelated root, so the remote tip is reachable only through + # the merge commit's *second* parent - a first-parent-only walk would miss it. + other = _commit(pair.local_store, pair.local_commits, {"b.txt": b"b"}, []) + merge = _commit( + pair.local_store, pair.local_commits, {"a.txt": b"a", "b.txt": b"b"}, [other, remote_tip] + ) + + pair.client.push(pair.address, "main", "tok") + assert pair.remote_commits.read_ref("main") == merge + assert set(pair.remote_commits.walk_history(merge)) == {merge, other, remote_tip} + + +def test_push_rejects_remote_ahead(push_pair): + pair = push_pair + c1 = _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + c2 = _commit(pair.local_store, pair.local_commits, {"a.txt": b"2"}, [c1]) + pair.client.push(pair.address, "main", "tok") + pair.local_commits.write_ref("main", c1) # local falls behind a remote it has seen + + with pytest.raises(NetworkProtocolError, match="ahead of local - pull first"): + pair.client.push(pair.address, "main", "tok") + assert pair.remote_commits.read_ref("main") == c2 + assert pair.local_commits.read_ref("main") == c1 + + +def test_push_rejects_diverged(push_pair): + pair = push_pair + base = _commit(pair.local_store, pair.local_commits, {"a.txt": b"base"}, []) + theirs = _commit(pair.local_store, pair.local_commits, {"a.txt": b"theirs"}, [base]) + pair.client.push(pair.address, "main", "tok") + ours = _commit(pair.local_store, pair.local_commits, {"a.txt": b"ours"}, [base]) + + with pytest.raises(NetworkProtocolError, match="diverged - pull and merge first"): + pair.client.push(pair.address, "main", "tok") + assert pair.remote_commits.read_ref("main") == theirs + assert pair.local_commits.read_ref("main") == ours + + +def test_two_stale_clients_second_rejected(tmp_path, push_pair): + """Week 5 checkpoint: two repos share a remote tip, commit independently, push both.""" + + pair = push_pair + alice = pair.client + bob = _second_client(tmp_path, pair) + + base = _commit(pair.local_store, pair.local_commits, {"a.txt": b"base"}, []) + alice.push(pair.address, "main", "tok") + bob.fetch_objects(pair.address, sorted(alice.collect_reachable("main")), "tok") + bob.commits.write_ref("main", base) + + alice_tip = _commit(pair.local_store, pair.local_commits, {"a.txt": b"alice"}, [base]) + bob_tip = _commit(bob.store, bob.commits, {"a.txt": b"bob"}, [base]) + + alice.push(pair.address, "main", "tok") + with pytest.raises(NetworkProtocolError, match="pull first"): + bob.push(pair.address, "main", "tok") + + assert pair.remote_commits.read_ref("main") == alice_tip + assert bob.commits.read_ref("main") == bob_tip + + +def test_stale_push_rejected_by_server(push_pair): + """A client that skips its own ancestry check still can't overwrite a moved ref.""" + + pair = push_pair + base = _commit(pair.local_store, pair.local_commits, {"a.txt": b"base"}, []) + pair.client.push(pair.address, "main", "tok") + moved = _commit(pair.remote_store, pair.remote_commits, {"a.txt": b"moved"}, [base]) + stale = _commit(pair.local_store, pair.local_commits, {"a.txt": b"stale"}, [base]) + + sock, buf = _open_push(pair, base, stale) + try: + reply = receive_line(sock, buf) + finally: + sock.close() + + assert reply == "ERR remote main has moved since you last synced - pull first" + assert pair.remote_commits.read_ref("main") == moved + + +def test_push_rejects_existing_ref_moved_during_transfer(push_pair): + pair = push_pair + base = _commit(pair.local_store, pair.local_commits, {"a.txt": b"base"}, []) + pair.client.push(pair.address, "main", "tok") + new = _commit(pair.local_store, pair.local_commits, {"a.txt": b"new"}, [base]) + + sock, buf = _open_push(pair, base, new) + try: + assert receive_line(sock, buf) == "OK" + pair.remote_commits.write_ref("main", "b" * 40) # another push lands mid-transfer + _upload(sock, buf, pair.local_store, sorted(pair.client.collect_reachable("main"))) + send_line(sock, "DONE") + assert receive_line(sock, buf) == "ERR remote main changed during push - pull first" + finally: + sock.close() + + assert pair.remote_commits.read_ref("main") == "b" * 40 + + +def test_direct_push_not_fast_forward(push_pair): + pair = push_pair + base = _commit(pair.local_store, pair.local_commits, {"a.txt": b"base"}, []) + pair.client.push(pair.address, "main", "tok") + unrelated = _commit(pair.local_store, pair.local_commits, {"b.txt": b"other"}, []) + + sock, buf = _open_push(pair, base, unrelated) + try: + assert receive_line(sock, buf) == "OK" + _upload(sock, buf, pair.local_store, sorted(pair.client.collect_reachable("main"))) + send_line(sock, "DONE") + assert receive_line(sock, buf) == "ERR not a fast-forward" + finally: + sock.close() + + assert pair.remote_commits.read_ref("main") == base + + +def test_direct_push_missing_objects(push_pair): + pair = push_pair + tip = _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + + sock, buf = _open_push(pair, "-", tip) + try: + assert receive_line(sock, buf) == "OK" + send_line(sock, "DONE") # claims done without uploading anything + assert receive_line(sock, buf).startswith("ERR incomplete history") + finally: + sock.close() + + assert pair.remote_commits.read_ref("main") is None + + +@pytest.mark.parametrize("old", ["zzz", "A" * 40, "a" * 39]) +def test_direct_push_invalid_old_hash(push_pair, old): + pair = push_pair + tip = _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + + sock, buf = _open_push(pair, old, tip) + try: + assert receive_line(sock, buf) == "ERR invalid old hash" + finally: + sock.close() + + assert pair.remote_commits.read_ref("main") is None + + +def test_retry_after_disconnect_reuses_objects(push_pair, monkeypatch): + pair = push_pair + tip = _commit( + pair.local_store, + pair.local_commits, + {"a.txt": b"1", "b.txt": b"2", "dir/c.txt": b"3"}, + [], + ) + objects = sorted(pair.client.collect_reachable("main")) + + sock, buf = _open_push(pair, "-", tip) + try: + assert receive_line(sock, buf) == "OK" + _upload(sock, buf, pair.local_store, objects[:2]) + finally: + sock.close() # drop the connection before DONE + assert pair.remote_commits.read_ref("main") is None + + puts = _count_puts(monkeypatch) + pair.client.push(pair.address, "main", "tok") + assert pair.remote_commits.read_ref("main") == tip + assert sorted(puts) == objects[2:] + + +def test_stalled_client_does_not_block_server(tmp_path): + server = RemoteServer(repo_path=str(tmp_path), token="tok", port=0, conn_timeout=0.5) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + stalled = socket.create_connection(("127.0.0.1", server.port), timeout=5) + try: + buf = bytearray() + send_line(stalled, "AUTH tok") + assert receive_line(stalled, buf) == "OK" + send_line(stalled, f"PUSH main - {'a' * 40}") + assert receive_line(stalled, buf) == "OK" + # ...and then goes silent mid-session, holding the serial server. + + with socket.create_connection(("127.0.0.1", server.port), timeout=3) as sock: + buf = bytearray() + send_line(sock, "AUTH tok") + assert receive_line(sock, buf) == "OK" + send_line(sock, "REF main") + assert receive_line(sock, buf) == "REF main -" + send_line(sock, "DONE") + finally: + stalled.close() + server.close() + thread.join(timeout=2) + assert not thread.is_alive() + + +def test_cli_push_rejection_shows_reason(push_pair, monkeypatch, capsys): + pair = push_pair + _commit(pair.remote_store, pair.remote_commits, {"other.txt": b"x"}, []) + _commit(pair.local_store, pair.local_commits, {"a.txt": b"1"}, []) + monkeypatch.chdir(pair.client.repo_path) + + assert main(["push", pair.address, "main", "--token", "tok"]) == 1 + err = capsys.readouterr().err + assert "push rejected" in err + assert "pull first" in err + assert "tok" not in err From 0e251eaa28bdca9d24ade1cb271e7c466d8549e6 Mon Sep 17 00:00:00 2001 From: Shuhan6017 Date: Sat, 10 Oct 2026 21:38:12 -0400 Subject: [PATCH 2/3] week 6 --- README.md | 30 +++ minigit/remote.py | 403 +++++++++++++++++++++++------ tests/test_remote.py | 445 +++++++++++++++++++++++++++++++- tests/test_week6_integration.py | 133 ++++++++++ 4 files changed, 927 insertions(+), 84 deletions(-) create mode 100644 tests/test_week6_integration.py diff --git a/README.md b/README.md index 9424b67..a6103e4 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,36 @@ A lint check catches unused imports, undefined names, and similar mistakes. A formatting check keeps everyone's code looking the same so diffs show real changes instead of whitespace. Tests are the ones you write in `tests/`. +## Sharing Repos Over the Network + +One repo serves, others push and pull. Every command needs the server's +token, which is never printed back. + +```bash +# in the repo that acts as the remote +minigit serve --port 9418 --token + +# in any other repo +minigit ls-remote 127.0.0.1:9418 --token # list the remote's branches +minigit push 127.0.0.1:9418 main --token # or --all for every local branch +minigit fetch 127.0.0.1:9418 main --token # download history only, no ref changes +minigit pull 127.0.0.1:9418 main --token # fetch, then integrate; or --all +``` + +- A push is rejected if the remote branch moved since you last synced + (`push rejected: ... - pull first`). Pull, merge, then push again. +- Pull fetches and validates the whole history before changing anything. + If the transfer fails, your refs, HEAD, index, and files are untouched, + and running the same pull again reuses what already arrived. +- Pulling a branch you don't have checked out creates or fast-forwards it + without touching your files. A diverged one is refused: check it out and + pull again to merge. +- Pulling into the checked-out branch fast-forwards or merges it. If both + sides changed the same lines, pull stops with the conflicting paths; fix + the conflict markers, `minigit add` the files, and `minigit commit`. + (This part needs Module 3's fetched-tip integration, #40. Until it lands, + pull refuses the checked-out branch and keeps the fetched history.) + ## Docs - [Onboarding guide](https://docs.google.com/document/d/1D77Ncmtunxj7GVADytHo1An6BKzaV-Ep7AxAYfL405k/edit?tab=t.0) - diff --git a/minigit/remote.py b/minigit/remote.py index 883ad6f..42d4d9b 100644 --- a/minigit/remote.py +++ b/minigit/remote.py @@ -11,9 +11,16 @@ import os import socket +import sys +from contextlib import contextmanager from minigit.commits import CommitManager -from minigit.errors import NetworkProtocolError, ObjectCorruptError, ObjectNotFoundError +from minigit.errors import ( + MiniGitError, + NetworkProtocolError, + ObjectCorruptError, + ObjectNotFoundError, +) from minigit.objects import ObjectStore @@ -154,6 +161,54 @@ def _parse_address(self, address: str) -> tuple[str, int]: else: raise NetworkProtocolError(f"address must be host:port, got {address!r}") + def _preflight(self, remote_address: str, token: str, operation: str) -> tuple[str, int]: + """Validate the address and token before touching any local or remote state.""" + + host, port = self._parse_address(remote_address) + if len(token) == 0: + raise NetworkProtocolError(f"{operation} needs a token: pass --token") + return host, port + + @contextmanager + def _session(self, host: str, port: int, token: str, operation: str): + """Connect, authenticate, and yield `(sock, buf)`; always closes the socket. + + Transport failures (refused, timeout, reset) and bad objects met + mid-session all surface as `NetworkProtocolError`. Error messages + name the operation and `host:port` only - never the token. + """ + + try: + sock = socket.create_connection((host, port), timeout=5) + except OSError as exc: + raise NetworkProtocolError(f"could not connect to {host}:{port}: {exc}") from exc + + try: + buf = bytearray() + send_line(sock, f"AUTH {token}") + if receive_line(sock, buf) != "OK": + raise NetworkProtocolError(f"{operation} to {host}:{port}: authentication failed") + yield sock, buf + except NetworkProtocolError: + raise + except (OSError, ObjectNotFoundError, ObjectCorruptError) as exc: + raise NetworkProtocolError(f"{operation} to {host}:{port} failed: {exc}") from exc + finally: + sock.close() + + def _request_ref(self, sock, buf: bytearray, branch: str) -> str: + """Ask the remote for `branch`'s tip; returns a commit hash or `-` if it has none.""" + + send_line(sock, f"REF {branch}") + reply = receive_line(sock, buf) + fields = reply.split(" ") + if len(fields) != 3 or fields[:2] != ["REF", branch]: + raise NetworkProtocolError(f"invalid REF response: {reply!r}") + remote_hash = fields[2] + if remote_hash != "-" and not _valid_hash(remote_hash): + raise NetworkProtocolError(f"invalid remote hash: {remote_hash!r}") + return remote_hash + def push(self, remote_address: str, branch: str, token: str) -> None: """Send local commits on `branch` to the remote, rejecting if it has diverged. @@ -168,34 +223,14 @@ def push(self, remote_address: str, branch: str, token: str) -> None: expected old tip there too, so a push that raced ours still loses. """ - host, port = self._parse_address(remote_address) - if len(token) == 0: - raise NetworkProtocolError("push needs a token: pass --token") + host, port = self._preflight(remote_address, token, "push") local_hash = self.commits.read_ref(branch) if local_hash is None: raise NetworkProtocolError(f"local branch {branch!r} has no commits to push") - try: - sock = socket.create_connection((host, port), timeout=5) - except OSError as exc: - raise NetworkProtocolError(f"could not connect to {host}:{port}: {exc}") from exc - - try: - buf = bytearray() - send_line(sock, f"AUTH {token}") - reply = receive_line(sock, buf) - if reply != "OK": - raise NetworkProtocolError(f"auth failed: {reply}") - - send_line(sock, f"REF {branch}") - reply = receive_line(sock, buf) - fields = reply.split(" ") - if len(fields) != 3 or fields[:2] != ["REF", branch]: - raise NetworkProtocolError(f"invalid REF response: {reply!r}") - remote_hash = fields[2] - if remote_hash != "-" and not _valid_hash(remote_hash): - raise NetworkProtocolError(f"invalid remote hash: {remote_hash!r}") + with self._session(host, port, token, "push") as (sock, buf): + remote_hash = self._request_ref(sock, buf, branch) if remote_hash == local_hash: send_line(sock, "DONE") @@ -226,12 +261,6 @@ def push(self, remote_address: str, branch: str, token: str) -> None: raise NetworkProtocolError(f"push rejected: {_rejection_reason(reply)}") print(f"{branch} now at {local_hash}") - except NetworkProtocolError: - raise - except (OSError, ObjectNotFoundError, ObjectCorruptError) as exc: - raise NetworkProtocolError(f"push to {host}:{port} failed: {exc}") from exc - finally: - sock.close() def _check_fast_forward(self, branch: str, remote_hash: str, local_hash: str) -> None: """Raise unless moving the remote from `remote_hash` to `local_hash` is a fast-forward. @@ -268,32 +297,156 @@ def _put_object(self, sock, buf: bytearray, obj_hash: str) -> None: if reply != "OK": raise NetworkProtocolError(f"remote rejected {obj_hash}: {reply}") + def list_refs(self, remote_address: str, token: str) -> dict[str, str]: + """Return every branch the remote advertises, as `{name: commit hash}`.""" + + host, port = self._preflight(remote_address, token, "ls-remote") + + refs: dict[str, str] = {} + with self._session(host, port, token, "ls-remote") as (sock, buf): + send_line(sock, "REFS") + while (line := receive_line(sock, buf)) != "END": + fields = line.split(" ") + if ( + len(fields) != 3 + or fields[0] != "REF" + or not _valid_branch(fields[1]) + or not _valid_hash(fields[2]) + or fields[1] in refs + ): + raise NetworkProtocolError(f"invalid REFS entry: {line!r}") + refs[fields[1]] = fields[2] + send_line(sock, "DONE") + return refs + + def fetch(self, remote_address: str, branch: str, token: str) -> str: + """Download `branch`'s complete history from the remote; return its tip hash. + + The tip is read once and the whole transfer is pinned to it, so the + remote branch moving mid-fetch can't switch snapshots. Only objects + missing locally are requested, but the walk still descends through + objects that are already present - an interrupted fetch can leave a + commit stored without its tree - so on return every commit, tree, + and blob reachable from the tip is present, hash-checked, and of the + type its parent says it is. No ref, HEAD, index, or file is touched; + objects received before a failure stay stored for a retry to reuse. + """ + + host, port = self._preflight(remote_address, token, "fetch") + if not _valid_branch(branch): + raise NetworkProtocolError(f"invalid branch name: {branch!r}") + + with self._session(host, port, token, "fetch") as (sock, buf): + tip = self._request_ref(sock, buf, branch) + if tip == "-": + raise NetworkProtocolError(f"remote has no branch {branch!r}") + self._fetch_history(sock, buf, tip) + send_line(sock, "DONE") + return tip + + def _fetch_history(self, sock, buf: bytearray, tip: str) -> None: + """Make every object reachable from commit `tip` present locally, following all parents.""" + + seen: set[str] = set() + pending = [tip] + while pending: + commit_hash = pending.pop() + if commit_hash in seen: + continue + seen.add(commit_hash) + + self._ensure_object(sock, buf, commit_hash, "commit") + commit = self.commits.read_commit(commit_hash) + self._fetch_tree(sock, buf, commit.tree, seen) + pending.extend(commit.parents) + + def _fetch_tree(self, sock, buf: bytearray, tree_hash: str, seen: set[str]) -> None: + """Make `tree_hash` and everything nested under it present locally, once each.""" + + if tree_hash in seen: + return + seen.add(tree_hash) + + self._ensure_object(sock, buf, tree_hash, "tree") + for entry in self.store.read_tree(tree_hash): + if entry.type == "tree": + self._fetch_tree(sock, buf, entry.hash, seen) + elif entry.hash not in seen: + seen.add(entry.hash) + self._ensure_object(sock, buf, entry.hash, "blob") + + def _ensure_object(self, sock, buf: bytearray, obj_hash: str, expected_type: str) -> None: + """Request `obj_hash` unless it's already stored, and check it is an `expected_type`.""" + + if _has_object(self.store, obj_hash): + obj_type, _ = self.store.read_object(obj_hash) # re-hashes: catches local corruption + if obj_type != expected_type: + raise NetworkProtocolError( + f"object {obj_hash} is a {obj_type}, expected a {expected_type}" + ) + return + + send_line(sock, f"WANT {obj_hash}") + self._receive_object(sock, buf, obj_hash, expected_type) + def pull(self, remote_address: str, branch: str, token: str) -> None: - """Fetch `branch` from the remote and update the matching local ref.""" + """Fetch `branch` from the remote and integrate it into the local branch of that name. - host, port = self._parse_address(remote_address) - if len(token) == 0: - raise NetworkProtocolError("pull needs a token: pass --token") + Integration is Module 3's job (`integrate_fetched`, #40): it runs + only after the fetched history is complete and validated, and a + `MergeConflictError` from it propagates unchanged so the user sees + the conflicting paths. + """ - try: - sock = socket.create_connection((host, port), timeout=5) - except OSError as exc: - raise NetworkProtocolError(f"could not connect to {host}:{port}: {exc}") from exc + fetched = self.fetch(remote_address, branch, token) + before = self.commits.read_ref(branch) + merge_hash = self._integrate(branch, fetched) + after = self.commits.read_ref(branch) - try: - buf = bytearray() - send_line(sock, f"AUTH {token}") - reply = receive_line(sock, buf) - if reply != "OK": - raise NetworkProtocolError(f"auth failed: {reply}") + if merge_hash is not None: + print(f"{branch}: merged {fetched[:7]} as {merge_hash[:7]}") + elif after == before: + print(f"{branch} is up to date") + else: + print(f"{branch} now at {after}") - send_line(sock, f"REF {branch}") - reply = receive_line(sock, buf) - remote_hash = reply.rsplit(" ", 1)[1] - print(f"remote {branch} is at {remote_hash}") - print("# Week 6 - same exchange in reverse") - finally: - sock.close() + def _integrate(self, branch: str, fetched: str) -> str | None: + """Hand a fetched, validated tip to M3; returns a new merge commit hash or None.""" + + integrate_fetched = getattr(self.commits, "integrate_fetched", None) + if integrate_fetched is not None: + return integrate_fetched(branch, fetched) + return self._integrate_by_ref(branch, fetched) + + def _integrate_by_ref(self, branch: str, fetched: str) -> None: + """Interim stand-in for M3's `integrate_fetched` until #40 lands. + + Handles only the cases that are pure ref moves through M3's + `read_ref`/`write_ref`: no-op when local already contains the fetched + tip, and creating or fast-forwarding a branch that isn't checked + out. Anything needing a checkout or a merge is refused rather than + reimplemented here; the fetched objects stay stored either way. + """ + + local = self.commits.read_ref(branch) + if local == fetched or (local is not None and _is_ancestor(self.commits, fetched, local)): + return None + + # M3 treats a repo with no HEAD file as being on `main`. + if branch == (self.commits.read_head() or "main"): + raise MiniGitError( + f"can't update checked-out branch {branch!r} yet: pulling into it needs " + "Module 3's fetched-tip integration (#40). The fetched history is stored " + "locally; pull again once that lands" + ) + if local is not None and not _is_ancestor(self.commits, local, fetched): + raise MiniGitError( + f"local {branch} has diverged from the remote: " + f"check out {branch} and pull again to merge" + ) + + self.commits.write_ref(branch, fetched) + return None def fetch_objects(self, remote_address: str, hashes: list[str], token: str) -> None: """Fetch each of `hashes` from the remote and write it into the local store. @@ -304,34 +457,19 @@ def fetch_objects(self, remote_address: str, hashes: list[str], token: str) -> N corrupted or mismatched reply never lands in the object store. """ - host, port = self._parse_address(remote_address) - if len(token) == 0: - raise NetworkProtocolError("fetch needs a token: pass --token") - - try: - sock = socket.create_connection((host, port), timeout=5) - except OSError as exc: - raise NetworkProtocolError(f"could not connect to {host}:{port}: {exc}") from exc - - try: - buf = bytearray() - send_line(sock, f"AUTH {token}") - reply = receive_line(sock, buf) - if reply != "OK": - raise NetworkProtocolError(f"auth failed: {reply}") - + host, port = self._preflight(remote_address, token, "fetch") + with self._session(host, port, token, "fetch") as (sock, buf): for obj_hash in dict.fromkeys(hashes): send_line(sock, f"WANT {obj_hash}") self._receive_object(sock, buf, obj_hash) - send_line(sock, "DONE") - except OSError as exc: - raise NetworkProtocolError(f"connection to {host}:{port} failed: {exc}") from exc - finally: - sock.close() - def _receive_object(self, sock, buf: bytearray, expected_hash: str) -> None: - """Read one `OBJ`/`ERR` reply for `expected_hash` and store it if it checks out.""" + def _receive_object( + self, sock, buf: bytearray, expected_hash: str, expected_type: str | None = None + ) -> str: + """Read one `OBJ`/`ERR` reply for `expected_hash`, store it if it checks out, + and return its type. `expected_type`, when given, must match too. + """ header = receive_line(sock, buf) command, _, rest = header.partition(" ") @@ -350,8 +488,13 @@ def _receive_object(self, sock, buf: bytearray, expected_hash: str) -> None: content = recv_exact(sock, buf, int(length_text)) 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: + raise NetworkProtocolError( + f"object {expected_hash} is a {obj_type}, expected a {expected_type}" + ) self.store.write_object(content, obj_type) + return obj_type def collect_reachable(self, branch: str) -> set[str]: """Return every commit, tree, and blob hash reachable from `branch`'s tip.""" @@ -462,7 +605,11 @@ def serve_forever(self) -> None: conn.close() def _handle_client(self, conn) -> None: - """Authenticate the connection, then answer REF / WANT requests until DONE.""" + """Authenticate the connection, then answer REF / REFS / WANT requests until DONE. + + Nothing is answered before a successful AUTH: any other first line + gets `ERR bad auth` and the connection is closed. + """ buf = bytearray() @@ -481,15 +628,35 @@ def _handle_client(self, conn) -> None: return elif command == "REF": self._send_ref(conn, value) + elif command == "REFS": + self._send_refs(conn) elif command == "WANT": self._send_object(conn, value) elif command == "PUSH": self._handle_push(conn, buf, value) return else: - send_line(conn, "ERR expected REF, WANT, PUSH, or DONE") + send_line(conn, "ERR expected REF, REFS, WANT, PUSH, or DONE") return + def _send_refs(self, conn) -> None: + """Reply with one `REF ` line per branch, then `END`. + + Branches whose name or contents wouldn't survive the wire format + (unsafe names, empty or malformed ref files) are left out. + """ + + for name in self.commits.list_branches(): + if not _valid_branch(name): + continue + try: + commit_hash = self.commits.read_ref(name) + except OSError: + continue # e.g. a directory under refs/heads/, not a ref file + if commit_hash and _valid_hash(commit_hash): + send_line(conn, f"REF {name} {commit_hash}") + send_line(conn, "END") + def _send_ref(self, conn, branch: str) -> None: """Reply with the commit hash `branch` currently points at, or `-` if it has none.""" @@ -652,6 +819,8 @@ def _send_object(self, conn, obj_hash: str) -> None: # # AUTH - client authenticates the connection with its token # REF - ask for / report the commit hash a branch currently points to +# REFS - list every branch: zero or more `REF `, then END +# END - terminates a REFS listing # WANT - request the object with this hash (fetch/pull) # PUSH - propose moving from ("-" = unborn) to # HAVE - ask whether the remote already has an object @@ -665,6 +834,13 @@ def _send_object(self, conn, obj_hash: str) -> None: # missing) -> DONE (remote validates the new history and moves the ref) -> # connection closes. # +# A fetch session: AUTH -> REF (pin the tip) -> WANT/OBJ for each commit, +# tree, and blob reachable from it that isn't already local -> DONE. Pull +# is a fetch followed by Module 3 integrating the tip into the local branch. +# Multi-branch transfers repeat the single-branch session per name from +# REFS (or per local branch for push); shared objects travel once because +# later sessions find them already present. +# # Conflict detection happens twice. The client refuses before PUSH if the # remote tip from REF isn't an ancestor of its local tip. The server # refuses PUSH if isn't its current ref, and again at DONE if the ref @@ -673,37 +849,98 @@ def _send_object(self, conn, obj_hash: str) -> None: def register_subcommands(subparsers) -> None: - """Register the `push`, `pull`, and `serve` subcommands with the CLI parser.""" + """Register the networking subcommands with the CLI parser.""" push_parser = subparsers.add_parser("push", help="push a branch to a remote") push_parser.add_argument("address") - push_parser.add_argument("branch") + push_parser.add_argument("branch", nargs="?") + push_parser.add_argument("--all", action="store_true", help="push every local branch") push_parser.add_argument("--token", default="") push_parser.set_defaults(handler=cmd_push) - pull_parser = subparsers.add_parser("pull", help="pull a branch to a local") + pull_parser = subparsers.add_parser("pull", help="fetch a branch and integrate it locally") pull_parser.add_argument("address") - pull_parser.add_argument("branch") + pull_parser.add_argument("branch", nargs="?") + pull_parser.add_argument("--all", action="store_true", help="pull every remote branch") pull_parser.add_argument("--token", default="") pull_parser.set_defaults(handler=cmd_pull) + fetch_parser = subparsers.add_parser("fetch", help="download a branch's history only") + fetch_parser.add_argument("address") + fetch_parser.add_argument("branch") + fetch_parser.add_argument("--token", default="") + fetch_parser.set_defaults(handler=cmd_fetch) + + ls_remote_parser = subparsers.add_parser("ls-remote", help="list a remote's branches") + ls_remote_parser.add_argument("address") + ls_remote_parser.add_argument("--token", default="") + ls_remote_parser.set_defaults(handler=cmd_ls_remote) + serve_parser = subparsers.add_parser("serve", help="serve this repo to push/pull clients") serve_parser.add_argument("--port", type=int, required=True) serve_parser.add_argument("--token", default="") serve_parser.set_defaults(handler=cmd_serve) +def _branches_from_args(args, all_branches) -> list[str]: + """Resolve `` vs `--all` into the list of branches to operate on.""" + + if args.all == (args.branch is not None): + raise MiniGitError("pass either a branch name or --all") + return sorted(all_branches()) if args.all else [args.branch] + + +def _run_each(operation, branches: list[str]) -> int: + """Run `operation(branch)` for each branch, reporting failures without stopping. + + A single branch's error propagates as usual. With several, each + failure is printed against its branch and the exit code is 1 if any + branch failed. + """ + + if len(branches) == 1: + operation(branches[0]) + return 0 + + failed = False + for branch in branches: + try: + operation(branch) + except MiniGitError as exc: + print(f"minigit: {branch}: {exc}", file=sys.stderr) + failed = True + return 1 if failed else 0 + + def cmd_push(args) -> int: """Handle `minigit push` from the CLI.""" - RemoteClient().push(args.address, args.branch, args.token) - return 0 + client = RemoteClient() + branches = _branches_from_args(args, client.commits.list_branches) + return _run_each(lambda branch: client.push(args.address, branch, args.token), branches) def cmd_pull(args) -> int: """Handle `minigit pull` from the CLI.""" - RemoteClient().pull(args.address, args.branch, args.token) + client = RemoteClient() + branches = _branches_from_args(args, lambda: client.list_refs(args.address, args.token)) + return _run_each(lambda branch: client.pull(args.address, branch, args.token), branches) + + +def cmd_fetch(args) -> int: + """Handle `minigit fetch` from the CLI.""" + + tip = RemoteClient().fetch(args.address, args.branch, args.token) + print(f"fetched {args.branch} at {tip}") + return 0 + + +def cmd_ls_remote(args) -> int: + """Handle `minigit ls-remote` from the CLI.""" + + for name, commit_hash in sorted(RemoteClient().list_refs(args.address, args.token).items()): + print(f"{commit_hash}\t{name}") return 0 diff --git a/tests/test_remote.py b/tests/test_remote.py index a4d4b01..a33e495 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -9,7 +9,13 @@ from minigit.cli import main from minigit.commits import CommitManager -from minigit.errors import NetworkProtocolError +from minigit.errors import ( + MergeConflictError, + MiniGitError, + NetworkProtocolError, + ObjectCorruptError, + ObjectNotFoundError, +) from minigit.objects import ObjectStore from minigit.objects import TreeEntry as RealTreeEntry from minigit.remote import RemoteClient, RemoteServer, receive_line, recv_exact, send_line @@ -1019,3 +1025,440 @@ def test_cli_push_rejection_shows_reason(push_pair, monkeypatch, capsys): assert "push rejected" in err assert "pull first" in err assert "tok" not in err + + +# --- fetch / pull / multi-ref (#41) --- + + +def _commit_on( + commits: CommitManager, branch: str, files: dict, parents: list, message: str +) -> str: + """Write a commit with fixed metadata straight onto `branch`, without touching HEAD.""" + + tree = _write_tree(commits.store, files) + lines = [f"tree {tree}", *(f"parent {p}" for p in parents)] + lines += ["author tester 0", "committer tester 0", "", message] + commit_hash = commits.store.write_object("\n".join(lines).encode(), "commit") + commits.write_ref(branch, commit_hash) + return commit_hash + + +class FakeIntegratingCommitManager(CommitManager): + """Stands in for M3's #40 `integrate_fetched` until the real one lands. + + Follows #40's spec: the checked-out branch is checked out + advanced on + a fast-forward, merged (here: a two-parent commit reusing the fetched + tree) on divergence, or raises `MergeConflictError` when told to; other + branches are created or fast-forwarded by ref only, and rejected when + diverged. Records each call and whether the fetched history was + complete at that moment. + """ + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.calls: list[tuple[str, str, bool]] = [] + self.conflict_paths: list[str] = [] + + def _history_complete(self, tip: str) -> bool: + try: + for commit_hash in self.walk_history(tip): + self.tree.read_tree_entries(self.read_commit(commit_hash).tree) + return True + except (ObjectNotFoundError, ObjectCorruptError): + return False + + def integrate_fetched(self, branch: str, fetched_hash: str) -> str | None: + self.calls.append((branch, fetched_hash, self._history_complete(fetched_hash))) + local = self.read_ref(branch) + if local == fetched_hash or (local and self.is_ancestor(fetched_hash, local)): + return None + fast_forward = local is None or self.is_ancestor(local, fetched_hash) + + if branch != self._current_branch(): + if not fast_forward: + raise MiniGitError(f"{branch} diverged; check it out and pull again") + self.write_ref(branch, fetched_hash) + return None + + if fast_forward: + self.tree.checkout(self.read_commit(fetched_hash).tree) + self.write_ref(branch, fetched_hash) + return None + if self.conflict_paths: + raise MergeConflictError(self.conflict_paths) + tree = self.read_commit(fetched_hash).tree + return self.create_commit(tree, [local, fetched_hash], "tester ", "merge") + + +@dataclass +class PullPair: + client: RemoteClient + local: FakeIntegratingCommitManager + remote: CommitManager + address: str + local_root: Path + + +@pytest.fixture +def pull_pair(tmp_path, push_pair): + """`push_pair`'s server, plus a fresh local repo whose M3 has `integrate_fetched`.""" + + root = tmp_path / "puller" + root.mkdir() + store = ObjectStore(str(root)) + local = FakeIntegratingCommitManager(str(root), store=store) + client = RemoteClient(repo_path=str(root), store=store, commits=local) + return PullPair(client, local, push_pair.remote_commits, push_pair.address, root) + + +def _count_receives(monkeypatch) -> list: + """Record every object payload `RemoteClient` receives, while still receiving it.""" + + calls = [] + original = RemoteClient._receive_object + + def counting(self, sock, buf, expected_hash, expected_type=None): + calls.append(expected_hash) + return original(self, sock, buf, expected_hash, expected_type) + + monkeypatch.setattr(RemoteClient, "_receive_object", counting) + return calls + + +def _scripted_client(monkeypatch, tmp_path, script: bytes) -> RemoteClient: + """A real client whose socket replays `script` as the server's side of the session.""" + + class ScriptedSocket(FakeReceiveSocket): + def sendall(self, data): + pass + + def close(self): + pass + + sock = ScriptedSocket(script) + monkeypatch.setattr(socket, "create_connection", lambda *args, **kwargs: sock) + return RemoteClient(tmp_path) + + +def _merge_history(remote: CommitManager) -> tuple[str, str, str]: + """base <- left <- merge(left, base): a merge whose second parent is the common ancestor.""" + + files = {"a.txt": b"base\n", "bin/data.bin": bytes(range(256)), "d/e/f.txt": b"deep\n"} + base = _commit_on(remote, "main", files, [], "base") + left = _commit_on(remote, "main", {**files, "a.txt": b"left\n"}, [base], "left") + merge = _commit_on(remote, "main", {**files, "a.txt": b"merged\n"}, [left, base], "merge") + return base, left, merge + + +def test_fetch_downloads_full_history_without_touching_refs(push_pair): + pair = push_pair + base, left, merge = _merge_history(pair.remote_commits) + + assert pair.client.fetch(pair.address, "main", "tok") == merge + + remote_client = RemoteClient( + pair.server.repo_path, store=pair.remote_store, commits=pair.remote_commits + ) + expected = remote_client._collect_from(merge) + for obj_hash in expected: + assert pair.local_store.read_object(obj_hash) == pair.remote_store.read_object(obj_hash) + assert set(pair.local_commits.walk_history(merge)) == {base, left, merge} + assert pair.local_commits.read_ref("main") is None + assert not (Path(pair.client.repo_path) / ".minigit" / "HEAD").exists() + + +def test_repeat_fetch_receives_no_objects(push_pair, monkeypatch): + pair = push_pair + _merge_history(pair.remote_commits) + pair.client.fetch(pair.address, "main", "tok") + + received = _count_receives(monkeypatch) + pair.client.fetch(pair.address, "main", "tok") + assert received == [] + + +def test_fetch_requests_missing_tree_under_present_commit(push_pair, monkeypatch): + """An interrupted fetch can leave a commit stored without its tree.""" + + pair = push_pair + tip = _commit_on(pair.remote_commits, "main", {"a.txt": b"1"}, [], "c") + obj_type, content = pair.remote_store.read_object(tip) + pair.local_store.write_object(content, obj_type) # commit present, tree + blob missing + + received = _count_receives(monkeypatch) + pair.client.fetch(pair.address, "main", "tok") + tree = pair.remote_commits.read_commit(tip).tree + assert tip not in received + assert tree in received + assert len(received) == 2 + + +def test_fetch_shared_objects_across_branches_transfer_once(push_pair, monkeypatch): + pair = push_pair + shared = {"shared.bin": b"\x00" * 1000, "lib/util.txt": b"util\n"} + _commit_on(pair.remote_commits, "main", {**shared, "m.txt": b"m"}, [], "m") + _commit_on(pair.remote_commits, "feature", {**shared, "f.txt": b"f"}, [], "f") + + received = _count_receives(monkeypatch) + pair.client.fetch(pair.address, "main", "tok") + pair.client.fetch(pair.address, "feature", "tok") + assert len(received) == len(set(received)) + assert pair.local_store.hash_object(b"\x00" * 1000, "blob") in received + + +def test_fetch_missing_branch_raises(push_pair): + with pytest.raises(NetworkProtocolError, match="no branch 'nope'"): + push_pair.client.fetch(push_pair.address, "nope", "tok") + + +@pytest.mark.parametrize("branch", ["../escape", "a b", "", "x/../y"]) +def test_fetch_invalid_branch_name_raises(push_pair, branch): + with pytest.raises(NetworkProtocolError, match="invalid branch"): + push_pair.client.fetch(push_pair.address, branch, "tok") + + +def test_bad_token_is_rejected_and_never_echoed(push_pair): + _commit_on(push_pair.remote_commits, "main", {"a.txt": b"1"}, [], "c") + for call in ( + lambda: push_pair.client.fetch(push_pair.address, "main", "s3cret-xyz"), + lambda: push_pair.client.list_refs(push_pair.address, "s3cret-xyz"), + ): + with pytest.raises(NetworkProtocolError, match="authentication failed") as info: + call() + assert "s3cret" not in str(info.value) + + +def test_command_before_auth_is_refused_and_server_survives(push_pair): + pair = push_pair + for first_line in ["REF main", "REFS", f"WANT {'a' * 40}"]: + with socket.create_connection(("127.0.0.1", pair.server.port), timeout=2) as sock: + buf = bytearray() + send_line(sock, first_line) + assert receive_line(sock, buf) == "ERR bad auth" + assert pair.client.list_refs(pair.address, "tok") == {} + + +def _blob_hash(data: bytes) -> str: + return hashlib.sha1(f"blob {len(data)}".encode() + b"\0" + data).hexdigest() + + +@pytest.mark.parametrize( + "reply, message", + [ + (b"OBJ blob 1\nx", "is a blob, expected a commit"), + (b"OBJ commit 3\nabc", "failed hash verification"), + (b"OBJ commit 50\nabc", "connection closed"), + (b"ERR unknown object\n", "could not provide"), + (b"garbage\n", "expected OBJ"), + ], +) +def test_fetch_bad_object_replies_raise(monkeypatch, tmp_path, reply, message): + tip = _blob_hash(b"x") # a valid hash the scripted server lies about + script = b"OK\nREF main " + tip.encode() + b"\n" + reply + client = _scripted_client(monkeypatch, tmp_path, script) + with pytest.raises(NetworkProtocolError, match=message): + client.fetch("localhost:9418", "main", "tok") + assert not client.store.has_object(tip) + + +@pytest.mark.parametrize( + "script", + [ + b"OK\nREF main zzz\nEND\n", + b"OK\nREF ../x " + b"a" * 40 + b"\nEND\n", + b"OK\nREF main " + b"a" * 40 + b"\nREF main " + b"b" * 40 + b"\nEND\n", + b"OK\nERR expected REF, WANT, PUSH, or DONE\n", + b"OK\nREF main " + b"a" * 40 + b"\n", # no END before the connection closes + ], +) +def test_list_refs_malformed_replies_raise(monkeypatch, tmp_path, script): + client = _scripted_client(monkeypatch, tmp_path, script) + with pytest.raises(NetworkProtocolError): + client.list_refs("localhost:9418", "tok") + + +def test_list_refs_lists_every_branch_and_skips_non_refs(push_pair): + pair = push_pair + assert pair.client.list_refs(pair.address, "tok") == {} + main_tip = _commit_on(pair.remote_commits, "main", {"a.txt": b"1"}, [], "m") + feature_tip = _commit_on(pair.remote_commits, "feature", {"b.txt": b"2"}, [], "f") + (Path(pair.server.repo_path) / ".minigit" / "refs" / "heads" / "nested").mkdir() + + assert pair.client.list_refs(pair.address, "tok") == {"main": main_tip, "feature": feature_tip} + + +def test_interrupted_fetch_keeps_state_and_retry_completes(push_pair, monkeypatch): + pair = push_pair + _, _, merge = _merge_history(pair.remote_commits) + total = len( + RemoteClient( + pair.server.repo_path, store=pair.remote_store, commits=pair.remote_commits + )._collect_from(merge) + ) + + original = RemoteClient._receive_object + calls = [] + + def flaky(self, sock, buf, expected_hash, expected_type=None): + if len(calls) == 3: + raise NetworkProtocolError("connection dropped") + calls.append(expected_hash) + return original(self, sock, buf, expected_hash, expected_type) + + monkeypatch.setattr(RemoteClient, "_receive_object", flaky) + with pytest.raises(NetworkProtocolError): + pair.client.fetch(pair.address, "main", "tok") + assert pair.local_commits.read_ref("main") is None + minigit_dir = Path(pair.client.repo_path) / ".minigit" + assert not (minigit_dir / "HEAD").exists() and not (minigit_dir / "index").exists() + + monkeypatch.setattr(RemoteClient, "_receive_object", original) + received = _count_receives(monkeypatch) + assert pair.client.fetch(pair.address, "main", "tok") == merge + assert len(received) == total - 3 + + +def test_pull_unborn_then_fast_forward_checks_out_files(pull_pair, capsys): + pair = pull_pair + base = _commit_on(pair.remote, "main", {"a.txt": b"one\n"}, [], "base") + pair.client.pull(pair.address, "main", "tok") + assert pair.local.read_ref("main") == base + assert (pair.local_root / "a.txt").read_bytes() == b"one\n" + + ahead = _commit_on(pair.remote, "main", {"a.txt": b"two\n"}, [base], "ahead") + capsys.readouterr() + pair.client.pull(pair.address, "main", "tok") + assert pair.local.read_ref("main") == ahead + assert (pair.local_root / "a.txt").read_bytes() == b"two\n" + assert f"main now at {ahead}" in capsys.readouterr().out + assert pair.local.calls == [("main", base, True), ("main", ahead, True)] + + +def test_pull_up_to_date_and_local_ahead_change_nothing(pull_pair, capsys, monkeypatch): + pair = pull_pair + base = _commit_on(pair.remote, "main", {"a.txt": b"one\n"}, [], "base") + pair.client.pull(pair.address, "main", "tok") + + received = _count_receives(monkeypatch) + capsys.readouterr() + pair.client.pull(pair.address, "main", "tok") + assert "main is up to date" in capsys.readouterr().out + assert received == [] + + local_tip = _commit_on(pair.local, "main", {"a.txt": b"local\n"}, [base], "local") + pair.client.pull(pair.address, "main", "tok") + assert pair.local.read_ref("main") == local_tip + assert "main is up to date" in capsys.readouterr().out + + +def test_pull_diverged_reports_merge_commit(pull_pair, capsys): + pair = pull_pair + base = _commit_on(pair.remote, "main", {"a.txt": b"base\n"}, [], "base") + pair.client.pull(pair.address, "main", "tok") + local_tip = _commit_on(pair.local, "main", {"a.txt": b"base\n", "l.txt": b"l"}, [base], "l") + remote_tip = _commit_on(pair.remote, "main", {"a.txt": b"base\n", "r.txt": b"r"}, [base], "r") + + capsys.readouterr() + pair.client.pull(pair.address, "main", "tok") + merge = pair.local.read_ref("main") + assert pair.local.read_commit(merge).parents == [local_tip, remote_tip] + assert f"main: merged {remote_tip[:7]} as {merge[:7]}" in capsys.readouterr().out + + +def test_pull_conflict_propagates_paths_and_keeps_ref(pull_pair): + pair = pull_pair + base = _commit_on(pair.remote, "main", {"a.txt": b"base\n"}, [], "base") + pair.client.pull(pair.address, "main", "tok") + local_tip = _commit_on(pair.local, "main", {"a.txt": b"ours\n"}, [base], "ours") + _commit_on(pair.remote, "main", {"a.txt": b"theirs\n"}, [base], "theirs") + pair.local.conflict_paths = ["a.txt"] + + with pytest.raises(MergeConflictError) as info: + pair.client.pull(pair.address, "main", "tok") + assert info.value.paths == ["a.txt"] + assert pair.local.read_ref("main") == local_tip + + +def test_pull_never_integrates_after_failed_fetch(pull_pair): + pair = pull_pair + tip = _commit_on(pair.remote, "main", {"a.txt": b"1"}, [], "c") + blob = pair.remote.store.hash_object(b"1", "blob") + pair.remote.store._object_path(blob).write_bytes(b"corrupt") + + with pytest.raises(NetworkProtocolError, match="corrupt object"): + pair.client.pull(pair.address, "main", "tok") + assert pair.local.calls == [] + assert pair.local.read_ref("main") is None + assert pair.local.store.has_object(tip) # complete objects kept for a retry + + +def test_pull_without_m3_creates_and_fast_forwards_inactive_branch(push_pair): + pair = push_pair + assert not hasattr(pair.local_commits, "integrate_fetched") + f1 = _commit_on(pair.remote_commits, "feature", {"f.txt": b"1"}, [], "f1") + pair.client.pull(pair.address, "feature", "tok") + assert pair.local_commits.read_ref("feature") == f1 + + f2 = _commit_on(pair.remote_commits, "feature", {"f.txt": b"2"}, [f1], "f2") + pair.client.pull(pair.address, "feature", "tok") + assert pair.local_commits.read_ref("feature") == f2 + assert not (Path(pair.client.repo_path) / "f.txt").exists() # no checkout + + +def test_pull_without_m3_rejects_diverged_inactive_branch(push_pair): + pair = push_pair + base = _commit_on(pair.remote_commits, "feature", {"f.txt": b"base"}, [], "base") + pair.client.pull(pair.address, "feature", "tok") + local_tip = _commit_on(pair.local_commits, "feature", {"f.txt": b"ours"}, [base], "ours") + _commit_on(pair.remote_commits, "feature", {"f.txt": b"theirs"}, [base], "theirs") + + with pytest.raises(MiniGitError, match="check out feature and pull again"): + pair.client.pull(pair.address, "feature", "tok") + assert pair.local_commits.read_ref("feature") == local_tip + + +def test_pull_without_m3_refuses_checked_out_branch(push_pair): + pair = push_pair + tip = _commit_on(pair.remote_commits, "main", {"a.txt": b"1"}, [], "c") + + with pytest.raises(MiniGitError, match="#40"): + pair.client.pull(pair.address, "main", "tok") + assert pair.local_commits.read_ref("main") is None + assert pair.local_store.has_object(tip) + + +def test_cli_ls_remote_and_push_all(push_pair, monkeypatch, capsys): + pair = push_pair + main_tip = _commit_on(pair.local_commits, "main", {"a.txt": b"1"}, [], "m") + feature_tip = _commit_on(pair.local_commits, "feature", {"b.txt": b"2"}, [main_tip], "f") + monkeypatch.chdir(pair.client.repo_path) + + assert main(["push", pair.address, "--all", "--token", "tok"]) == 0 + assert pair.remote_commits.read_ref("main") == main_tip + assert pair.remote_commits.read_ref("feature") == feature_tip + + capsys.readouterr() + assert main(["ls-remote", pair.address, "--token", "tok"]) == 0 + assert capsys.readouterr().out == f"{feature_tip}\tfeature\n{main_tip}\tmain\n" + + +def test_cli_pull_all_reports_each_branch(push_pair, monkeypatch, capsys): + pair = push_pair + _commit_on(pair.remote_commits, "main", {"a.txt": b"1"}, [], "m") + feature_tip = _commit_on(pair.remote_commits, "feature", {"b.txt": b"2"}, [], "f") + monkeypatch.chdir(pair.client.repo_path) + + # main is the checked-out branch, which needs M3's #40 helper; feature still lands. + assert main(["pull", pair.address, "--all", "--token", "tok"]) == 1 + out, err = capsys.readouterr() + assert pair.local_commits.read_ref("feature") == feature_tip + assert f"feature now at {feature_tip}" in out + assert "minigit: main:" in err + + +@pytest.mark.parametrize("args", [["push", "--all", "main"], ["push"], ["pull"]]) +def test_cli_branch_or_all_required(push_pair, monkeypatch, capsys, args): + monkeypatch.chdir(push_pair.client.repo_path) + command, *rest = args + assert main([command, push_pair.address, *rest, "--token", "tok"]) == 1 + assert "either a branch name or --all" in capsys.readouterr().err diff --git a/tests/test_week6_integration.py b/tests/test_week6_integration.py new file mode 100644 index 0000000..ee1d32e --- /dev/null +++ b/tests/test_week6_integration.py @@ -0,0 +1,133 @@ +"""Week 6 checkpoint: two independent repos exchanging branches through the CLI. + +The parts that pull into a checked-out branch need Module 3's fetched-tip +integration (#40); they skip until `CommitManager.integrate_fetched` exists +and then run automatically. +""" + +import threading + +import pytest + +from minigit.cli import main +from minigit.commits import CommitManager +from minigit.remote import RemoteServer + +needs_m3_integration = pytest.mark.skipif( + not hasattr(CommitManager, "integrate_fetched"), + reason="pulling into the checked-out branch needs M3's integrate_fetched (#40)", +) + + +@pytest.fixture +def server(tmp_path): + remote = tmp_path / "remote" + remote.mkdir() + server = RemoteServer(remote, token="test-token") + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + yield server + server.close() + thread.join(timeout=2) + + +def _run(monkeypatch, repo, *argv) -> int: + """Run one fresh `minigit` CLI call inside `repo`.""" + + monkeypatch.chdir(repo) + return main(list(argv)) + + +def _commit_file(monkeypatch, repo, path, text, message) -> str: + (repo / path).write_text(text) + assert _run(monkeypatch, repo, "add", path) == 0 + assert _run(monkeypatch, repo, "commit", "-m", message) == 0 + manager = CommitManager(repo) + return manager.read_ref(manager.read_head()) + + +def _repo_with_merge_commit(monkeypatch, root): + """main and feature diverge from a base, then main records a two-parent merge.""" + + root.mkdir() + assert _run(monkeypatch, root, "init") == 0 + base = _commit_file(monkeypatch, root, "a.txt", "line\n", "base") + assert _run(monkeypatch, root, "branch", "feature") == 0 + assert _run(monkeypatch, root, "checkout", "feature") == 0 + feature = _commit_file(monkeypatch, root, "f.txt", "feature\n", "feature") + assert _run(monkeypatch, root, "checkout", "main") == 0 + main_tip = _commit_file(monkeypatch, root, "a.txt", "main\n", "main") + + # Three-way merge isn't in M3 yet (#33), so record the merge commit directly. + (root / "f.txt").write_text("feature\n") + assert _run(monkeypatch, root, "add", "f.txt") == 0 + manager = CommitManager(root) + tree = manager.tree.build_tree_from_index() + merge = manager.create_commit(tree, [main_tip, feature], "tester ", "merge") + return base, feature, main_tip, merge + + +def test_two_repos_exchange_branches_and_merge_commit(tmp_path, monkeypatch, capsys, server): + address = f"127.0.0.1:{server.port}" + a, b = tmp_path / "a", tmp_path / "b" + _, feature, main_tip, merge = _repo_with_merge_commit(monkeypatch, a) + + assert _run(monkeypatch, a, "push", address, "--all", "--token", "test-token") == 0 + + b.mkdir() + assert _run(monkeypatch, b, "init") == 0 + capsys.readouterr() + assert _run(monkeypatch, b, "ls-remote", address, "--token", "test-token") == 0 + assert capsys.readouterr().out == f"{feature}\tfeature\n{merge}\tmain\n" + + # feature isn't checked out in B, so it's imported by ref alone. + assert _run(monkeypatch, b, "pull", address, "feature", "--token", "test-token") == 0 + assert CommitManager(b).read_ref("feature") == feature + capsys.readouterr() + assert _run(monkeypatch, b, "pull", address, "feature", "--token", "test-token") == 0 + assert "feature is up to date" in capsys.readouterr().out + + # The merge commit's full history (both parents) arrives intact. + assert _run(monkeypatch, b, "fetch", address, "main", "--token", "test-token") == 0 + receiver = CommitManager(b) + assert receiver.read_commit(merge).parents == [main_tip, feature] + assert set(receiver.walk_history(merge)) == set(CommitManager(a).walk_history(merge)) + + # A and B both commit on feature; A pushes first, so B's push is rejected. + assert _run(monkeypatch, b, "checkout", "feature") == 0 + assert (b / "f.txt").read_text() == "feature\n" + _commit_file(monkeypatch, b, "f.txt", "from b\n", "b") + assert _run(monkeypatch, a, "checkout", "feature") == 0 + a_feature = _commit_file(monkeypatch, a, "f.txt", "from a\n", "a") + assert _run(monkeypatch, a, "push", address, "feature", "--token", "test-token") == 0 + + capsys.readouterr() + assert _run(monkeypatch, b, "push", address, "feature", "--token", "test-token") == 1 + assert "pull first" in capsys.readouterr().err + assert CommitManager(server.repo_path).read_ref("feature") == a_feature + + +@needs_m3_integration +def test_pull_into_checked_out_branch_and_resolve_conflict(tmp_path, monkeypatch, server): + address = f"127.0.0.1:{server.port}" + a, b = tmp_path / "a", tmp_path / "b" + _, _, _, merge = _repo_with_merge_commit(monkeypatch, a) + assert _run(monkeypatch, a, "push", address, "main", "--token", "test-token") == 0 + + b.mkdir() + assert _run(monkeypatch, b, "init") == 0 + assert _run(monkeypatch, b, "pull", address, "main", "--token", "test-token") == 0 + assert CommitManager(b).read_ref("main") == merge + assert (b / "a.txt").read_text() == "main\n" + + # Same line changed on both sides -> pull reports the conflict. + a_tip = _commit_file(monkeypatch, a, "a.txt", "from a\n", "a") + assert _run(monkeypatch, a, "push", address, "main", "--token", "test-token") == 0 + b_tip = _commit_file(monkeypatch, b, "a.txt", "from b\n", "b") + assert _run(monkeypatch, b, "pull", address, "main", "--token", "test-token") == 1 + assert "<<<<<<<" in (b / "a.txt").read_text() + + # Resolve -> add -> commit, across fresh CLI calls, yields a two-parent commit. + resolved = _commit_file(monkeypatch, b, "a.txt", "resolved\n", "resolve") + assert CommitManager(b).read_commit(resolved).parents == [b_tip, a_tip] + assert _run(monkeypatch, b, "push", address, "main", "--token", "test-token") == 0 From 80f143e1e2243e3e73839aae84b0726d43526bec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 13 Oct 2026 01:01:04 +0000 Subject: [PATCH 3/3] Handle push/pull arg order without argparse exit Co-authored-by: Shuhan6017 <135551146+Shuhan6017@users.noreply.github.com> --- minigit/cli.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/minigit/cli.py b/minigit/cli.py index 512230d..a554979 100644 --- a/minigit/cli.py +++ b/minigit/cli.py @@ -70,7 +70,17 @@ def build_parser() -> argparse.ArgumentParser: def main(argv: Sequence[str] | None = None) -> int: parser = build_parser() - args = parser.parse_args(argv) + args, unknown = parser.parse_known_args(argv) + if unknown: + if ( + getattr(args, "command", None) in {"push", "pull"} + and len(unknown) == 1 + and not unknown[0].startswith("-") + and getattr(args, "branch", None) is None + ): + args.branch = unknown[0] + else: + parser.error(f"unrecognized arguments: {' '.join(unknown)}") handler = getattr(args, "handler", None) if handler is None: