From 3ce793d05ea9415e935542ca26d0d8d413e0d6ad Mon Sep 17 00:00:00 2001 From: Charlie Scherer Date: Thu, 17 Sep 2026 21:28:55 +0000 Subject: [PATCH] Clone a task's submodules from the repo's own checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spawn-prep filled a per-task clone's submodules by fetching each one from its forge-resolved URL — paid by every task on the repo, and for a large submodule it dominates spawn-prep, while the superproject itself is nearly free (`clone --local` hardlinks the cache's objects). When the repo's `git_url` names a checkout on this host, that checkout already holds every submodule's objects, so clone them from it instead: a local clone with a hardlinked object store and no network. The donor is matched by path, never by URL (the donor and the per-task clone resolve relative `.gitmodules` URLs against different origins), so hydration walks the tree a level at a time — init, override the resolved URL with the donor's checkout, update this level, recurse — and finishes with one `submodule sync --recursive` that puts the canonical URLs back in the config and in each submodule's own origin. It is only an optimisation: no local checkout, a raise, or a submodule still uninitialized afterwards all fall back to the plain recursive update, which is exactly what a repo without a donor gets. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 11 +- docs/design/BACKLOG.md | 14 +- .../0011-provisioning-per-task-clone.md | 40 ++- src/panopticon/core/git.py | 143 +++++++- src/panopticon/sessionservice/spawn.py | 103 +++++- src/panopticon/terminal/quickstart.py | 38 +-- tests/core/test_git.py | 81 +++++ tests/sessionservice/test_spawn.py | 307 ++++++++++++++++-- 8 files changed, 647 insertions(+), 90 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c50ba9fd..4ef37887 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -213,9 +213,14 @@ on every PR (the same commands the Makefile wraps). - `tests/test_spawn.py` — spawn-prep (ADR 0011): unit tests pin the `clone --local` of the per-task checkout and the idempotency gate (skips when the checkout already exists), plus the **submodule** init — emitted after the `origin` repoint, gated on `submodule status` reporting an - uninitialized (`-`) submodule so it retries but never touches an initialized one; a `skipif` - integration test fills in a real submodule and then **moves** the checkout, pinning that the - recorded links stay relative (the ADR 0011 mounts-anywhere property). + uninitialized (`-`) submodule so it retries but never touches an initialized one — and the + **donor hydration** (ADR 0011 §1c): the per-level init/url-override/update/sync when the repo has + a checkout on this host, and every fallback to the plain fetch (no donor, donor gone, hydration + raised, a submodule still uninitialized). A `skipif` integration test fills in a real submodule + and then **moves** the checkout, pinning that the recorded links stay relative (the ADR 0011 + mounts-anywhere property); another hydrates a nested submodule from a source repo whose + submodules' own repos have been moved away, pinning that the objects are hardlinked from it and + that `sync` leaves no donor path behind. - `tests/test_prefill.py` — the input-box prefill poller: unit tests drive `prefill_pane` with a fake tmux runner + injected `sleep`/raw-log — pin the `pipe-pane`/`load-buffer`/`paste-buffer -p` commands when the box becomes ready, and every best-effort give-up (empty prompt, timeout, diff --git a/docs/design/BACKLOG.md b/docs/design/BACKLOG.md index 7d12e9d8..5d84b8ab 100644 --- a/docs/design/BACKLOG.md +++ b/docs/design/BACKLOG.md @@ -81,13 +81,13 @@ in the ADRs; this file is for the smaller stuff that doesn't have a home there y `docker buildx bake` (target inheritance, which maps cleanly onto base→workflow→repo). Not needed now; the fragment approach is the minimal thing that works. _(Slice 6, P3)_ -- [ ] **Share submodule objects with the cache clone** — spawn-prep initializes a task's - submodules from their forge-resolved URLs (ADR 0011 §1b), so every task pays a full submodule - fetch. Sharing the repo cache's object store would avoid it, but the obvious lever - (`submodule.alternateLocation=superproject`) derives the alternate from the superproject's - `origin`, which spawn deliberately repoints at the forge before initializing — so it needs the - per-submodule alternate paths (`/.git/modules/`) passed explicitly, and the cache - clone made submodule-aware. Only worth it for repos with large submodules. _(P3)_ +- [ ] **Share submodule objects for forge-hosted repos** — a task whose repo has a checkout on + this host now hardlink-clones its submodules out of it (ADR 0011 §1c), but a repo whose `git_url` + is a hosted forge has no local donor and still pays a full submodule fetch per task. Making the + repo's **cache** clone submodule-aware would give those repos a donor too. The obvious lever + (`submodule.alternateLocation=superproject`) isn't it: the alternate is derived from the + superproject's `origin`, which spawn deliberately repoints at the forge — so it would be the same + path-keyed hydration, pointed at the cache. Only worth it for repos with large submodules. _(P3)_ ## Tracked elsewhere (pointers, do not duplicate) diff --git a/docs/design/decisions/0011-provisioning-per-task-clone.md b/docs/design/decisions/0011-provisioning-per-task-clone.md index 102bc1c0..8362a09b 100644 --- a/docs/design/decisions/0011-provisioning-per-task-clone.md +++ b/docs/design/decisions/0011-provisioning-per-task-clone.md @@ -66,11 +66,41 @@ The "mounts at any container path" property survives, because `submodule update` submodule's gitdir pointer (`gitdir: ../../.git/modules/`) and its `core.worktree` **relatively** — nothing absolute to mirror, same as the superproject. -The repo's **cache** clone stays submodule-free: the per-task clone fetches submodules from their -forge-resolved URLs, so cache-side submodule checkouts would be disk and time for nothing. The cost -is one submodule fetch per task; sharing the cache's object store instead -(`submodule.alternateLocation=superproject`) computes the alternate from the superproject's *origin*, -which is exactly what we repoint at the forge — so it needs more than a flag. Backlogged. +### 1c. Submodules come from the repo's own checkout when there is one + +Fetching each submodule from its URL is paid by **every** task on the repo, and for a large +submodule it dominates spawn-prep — while the superproject itself is nearly free (`clone --local` +hardlinks the cache's objects). When the repo's `git_url` names a checkout on this host (the +local-git flow), that checkout already holds every submodule's objects, so spawn-prep clones them +**from it** — a local clone, hardlinked object store, no network (`hydrate_submodules`). + +The donor is matched **by path**, never by URL: the donor resolved its relative `.gitmodules` URLs +against *its own* `origin` while the per-task clone resolves them against `git_url`, so the same +submodule can legitimately have two different URLs. Per superproject level: + +1. `submodule init` — git resolves the declared URLs into `submodule..url`; +2. for each submodule the donor has checked out, that resolved URL is overwritten with the donor's + path (one the donor lacks keeps its own URL and is simply fetched); +3. `submodule update` for **this level only** — a nested submodule's URL cannot be resolved, let + alone redirected, before its parent exists; +4. recurse into each submodule against the matching donor level. + +Then, once at the top, `submodule sync --recursive` restores the canonical URLs — in the config +*and* in each submodule's own `origin` — so no host path from the donor reaches the container, and +the agent fetches and pushes where it should. + +It is **only** an optimisation, never a precondition: if there's no local checkout, if hydration +raises, or if it leaves any submodule uninitialized (a donor behind the recorded commit), spawn-prep +falls back to the plain `submodule update --init --recursive` above. Hardlinks make the task's +objects independent of a later `git gc` in the donor — the same property `clone --local` already +relies on for the superproject — and a cross-filesystem donor degrades to a copy, still local. + +The repo's **cache** clone stays submodule-free: it is not the donor, and cache-side submodule +checkouts would be disk and time for nothing. A repo whose `git_url` is a hosted forge has no local +donor and still pays one submodule fetch per task; making the cache clone submodule-aware would +close that too (backlogged) — `submodule.alternateLocation=superproject` isn't the lever, since it +computes the alternate from the superproject's *origin*, which is exactly what we repoint at the +forge. ### 2. Provisioning = branch whatever's there diff --git a/src/panopticon/core/git.py b/src/panopticon/core/git.py index f8a9cb4c..c0678ac1 100644 --- a/src/panopticon/core/git.py +++ b/src/panopticon/core/git.py @@ -18,11 +18,48 @@ import subprocess from collections.abc import Sequence from dataclasses import dataclass +from pathlib import Path from typing import Protocol #: Feature-branch namespace (PARITY §8/§14, renamed from cloude-cade's ``cloude/``). BRANCH_PREFIX = "panopticon" +#: URL schemes that mean a networked (hosted-forge) remote rather than a local path. +_FORGE_SCHEMES = ("https://", "http://", "ssh://", "git://", "ftp://", "ftps://") + + +def is_forge_url(git_url: str) -> bool: + """True when ``git_url`` names a hosted-forge remote (network push/PR/CI), not a local path. + + Recognizes URL-scheme remotes (``https://…``, ``ssh://…``, …) and scp-like ``user@host:path`` + remotes; treats a bare filesystem path or a ``file://`` URL as local-only. + """ + url = git_url.strip() + if url.lower().startswith("file://"): + return False + if url.lower().startswith(_FORGE_SCHEMES): + return True + # scp-like syntax: user@host:path — an '@' and a ':' before any '/'. A Windows drive path + # (``C:\…``) has the ':' but no '@', so it stays local. + at, colon, slash = url.find("@"), url.find(":"), url.find("/") + return at != -1 and colon > at and (slash == -1 or colon < slash) + + +def local_repo_path(git_url: str) -> str | None: + """The filesystem path ``git_url`` names, or ``None`` when it names a networked remote. + + The counterpart of :func:`is_forge_url`: a bare path or a ``file://`` URL is somewhere on this + host — which is what makes panopticon's host-side push, and cloning a task's submodules from + the repo's own checkout (:func:`panopticon.sessionservice.spawn.hydrate_submodules`), possible + at all. + """ + if is_forge_url(git_url): + return None + url = git_url.strip() + if url.lower().startswith("file://"): + url = url[len("file://") :] + return str(Path(url).expanduser()) if url else None + class GitError(RuntimeError): """A ``git`` command that exited non-zero, carrying its ``stderr``. @@ -87,6 +124,27 @@ def parse_submodule_status(output: str) -> dict[str, str]: return states +def parse_submodule_paths(output: str) -> dict[str, str]: + """Parse ``git config --get-regexp`` output into ``{submodule name: path}``. + + Each line is ``submodule..path `` — read from ``.gitmodules`` rather than from + ``git submodule status`` because it answers *before* ``submodule init`` has run and for + submodules that aren't checked out. The name is whatever sits between the ``submodule.`` + prefix and the ``.path`` suffix (it usually **is** the path, dots and slashes included), and + the value is the rest of the line, so a path containing spaces survives. Lines that don't + match the shape are skipped. + """ + paths: dict[str, str] = {} + for line in output.splitlines(): + key, _, value = line.partition(" ") + if not value or not key.startswith("submodule.") or not key.endswith(".path"): + continue + name = key[len("submodule.") : -len(".path")] + if name: + paths[name] = value + return paths + + @dataclass(frozen=True) class Worktree: """A created worktree: its branch and on-disk path.""" @@ -164,8 +222,59 @@ def submodule_status(self, *, repo_path: str) -> dict[str, str]: out = self._run(["git", "-C", repo_path, "submodule", "status", "--recursive"]) return parse_submodule_status(out) - def update_submodules(self, *, repo_path: str) -> None: - """``git -C submodule update --init --recursive`` — fill in the submodule checkouts. + def submodule_paths(self, *, repo_path: str) -> dict[str, str]: + """The submodules this repo *declares* — ``{name: path}``, empty when there are none. + + Reads ``.gitmodules`` (``git config --file``), not ``git submodule status``: the caller + overriding a submodule's URL (:meth:`set_submodule_url`) needs the **name** git keys that + config on, and needs it for a submodule that isn't checked out yet. Only this level's + submodules — a nested one is declared in its own superproject's ``.gitmodules``, so the + caller recurses. ``check=False`` because ``git config`` exits non-zero when the file (or a + match) is absent, which is just "no submodules". + """ + out = self._run( + [ + "git", + "-C", + repo_path, + "config", + "--file", + ".gitmodules", + "--get-regexp", + "^submodule\\..*\\.path$", + ], + check=False, + ) + return parse_submodule_paths(out) + + def init_submodules(self, *, repo_path: str) -> None: + """``git -C submodule init`` — resolve each declared URL into ``submodule..url``. + + Separated from ``update`` so a caller can *override* the resolved URL in between (the + donor hydration in ``sessionservice.spawn``); ``update --init`` would clone before the + override could land. + """ + self._run(["git", "-C", repo_path, "submodule", "init"]) + + def set_submodule_url(self, *, repo_path: str, name: str, url: str) -> None: + """``git -C config submodule..url `` — where ``update`` clones from. + + The config value wins over ``.gitmodules`` until :meth:`sync_submodules` restores it. + """ + self._run(["git", "-C", repo_path, "config", f"submodule.{name}.url", url]) + + def sync_submodules(self, *, repo_path: str) -> None: + """``git -C submodule sync --recursive`` — restore the canonical submodule URLs. + + Rewrites every ``submodule..url`` from ``.gitmodules`` (resolved against the + superproject's ``origin``) **and** repoints each checked-out submodule's own + ``remote.origin.url`` at it — so a temporary local-donor override leaves nothing of the + host's paths behind in the checkout the container gets. + """ + self._run(["git", "-C", repo_path, "submodule", "sync", "--recursive"]) + + def update_submodules(self, *, repo_path: str, recursive: bool = True) -> None: + """``git -C submodule update --init [--recursive]`` — fill in the submodule checkouts. ``protocol.file.allow=always`` is **required**, not cosmetic: since git 2.38 a submodule whose resolved URL is a local path is refused (``transport 'file' not allowed``, @@ -178,20 +287,24 @@ def update_submodules(self, *, repo_path: str) -> None: Submodule URLs are resolved against the superproject's ``remote.origin.url`` *here*, so the caller must point ``origin`` at the forge first (:meth:`set_origin`) — resolving a relative URL against the cache path would look for the submodule next to the cache clone. + + ``recursive=False`` updates **this level only**: a nested submodule's URL can't be resolved + (or overridden) before its parent exists, so the donor hydration walks the tree a level at + a time instead. """ - self._run( - [ - "git", - "-C", - repo_path, - "-c", - "protocol.file.allow=always", - "submodule", - "update", - "--init", - "--recursive", - ] - ) + args = [ + "git", + "-C", + repo_path, + "-c", + "protocol.file.allow=always", + "submodule", + "update", + "--init", + ] + if recursive: + args.append("--recursive") + self._run(args) def push(self, *, repo_path: str, remote: str, branch: str) -> None: """``git -C push `` — send one branch, as-is (never forced). diff --git a/src/panopticon/sessionservice/spawn.py b/src/panopticon/sessionservice/spawn.py index 2bababe1..c59e62f1 100644 --- a/src/panopticon/sessionservice/spawn.py +++ b/src/panopticon/sessionservice/spawn.py @@ -4,8 +4,10 @@ it makes the repo's cache clone current (`CloneCache`) and `git clone --local`s it to the per-task path that gets bind-mounted at ``/workspace``. A ``--local`` clone is self-contained (hardlinked objects), so it mounts at any container path; the agent works there the whole task and the slug -later just branches it (`Provisioner`). Submodules are initialized too — recursively, and *after* -``origin`` is repointed, since that's what relative ``.gitmodules`` URLs resolve against. +later just branches it (`Provisioner`). Submodules are filled in too — *after* ``origin`` is +repointed, since that's what relative ``.gitmodules`` URLs resolve against, and **from the repo's +own checkout on this host** when ``git_url`` names one (`hydrate_submodules`), so they're local +hardlink clones rather than a per-task fetch from their forge. Idempotent: skips the clone (and the cache fetch) when the per-task checkout already exists — e.g. a re-created container re-mounts the same dir. LLM-free. @@ -20,7 +22,7 @@ from pathlib import Path from panopticon.client import JsonObj -from panopticon.core.git import SUBMODULE_UNINITIALIZED, GitClones +from panopticon.core.git import SUBMODULE_UNINITIALIZED, GitClones, local_repo_path from panopticon.sessionservice.clones import CloneCache _log = logging.getLogger(__name__) @@ -30,6 +32,10 @@ #: collide with another task's checkout path. QUARANTINE_SUFFIX = ".stale" +#: How deep :func:`hydrate_submodules` follows nested submodules. A guard against a cyclic or +#: pathological nesting, not a real limit — git itself becomes unusable long before this. +MAX_SUBMODULE_DEPTH = 10 + def prepare_workspace( task_id: str, @@ -70,10 +76,99 @@ def prepare_workspace( git.clone_local(cache_path=cache_path, dest=clone) git.set_origin(repo_path=clone, url=repo["git_url"]) if _needs_submodules(git.submodule_status(repo_path=clone)): - git.update_submodules(repo_path=clone) + _fill_in_submodules(clone, repo, git=git, exists=exists) return clone +def _fill_in_submodules( + clone: str, repo: JsonObj, *, git: GitClones, exists: Callable[[str], bool] +) -> None: + """Check the repo's submodules out into the per-task clone — locally when that's possible. + + Prefers :func:`hydrate_submodules` from the repo's own checkout on this host (a local ``git_url`` + — the donor), and otherwise, or if that leaves anything uninitialized, falls back to the plain + fetch-from-their-URLs update. The donor path is **only** an optimisation: any way it can fail + ends in exactly the behaviour a repo without a donor gets. + """ + donor = local_repo_path(str(repo["git_url"])) + if donor and exists(donor): + try: + hydrate_submodules(clone, donor, git=git, exists=exists) + except Exception: # any donor failure falls back to the network path below + _log.warning( + "hydrating %s's submodules from %s failed; fetching them instead", + clone, + donor, + exc_info=True, + ) + else: + if not _needs_submodules(git.submodule_status(repo_path=clone)): + return + _log.info( + "%s still has uninitialized submodules after hydrating from %s; fetching them", + clone, + donor, + ) + git.update_submodules(repo_path=clone) + + +def hydrate_submodules( + clone: str, + donor: str, + *, + git: GitClones, + exists: Callable[[str], bool] = os.path.isdir, + depth: int = MAX_SUBMODULE_DEPTH, +) -> None: + """Check ``clone``'s submodules out by cloning them from ``donor``'s hydrated ones. + + ``donor`` is the repo's own checkout on this host (a local ``git_url``), which already holds + every submodule's objects — so each submodule is a *local* clone (hardlinked object store, + no network) instead of a fetch from its forge, paid once per task. That is the whole point: + the superproject was already near-free (``clone --local``); this makes its submodules so too. + + The donor is matched **by path**, never by URL: the donor resolved its relative ``.gitmodules`` + URLs against *its own* ``origin`` and the per-task clone resolves them against the repo's + ``git_url``, so the two can name the same submodule differently. Per level: + + 1. ``submodule init`` — let git resolve the declared URLs into config; + 2. for each submodule the donor actually has checked out, overwrite that resolved URL with the + donor's path (a submodule the donor lacks keeps its real URL and is simply fetched); + 3. ``submodule update`` for **this level only** — a nested submodule's URL can't be resolved + before its parent exists; + 4. recurse into each submodule with the matching donor level. + + Then, once at the top, ``submodule sync --recursive`` puts the canonical URLs back — in the + config *and* in each submodule's own ``origin`` — so the donor's host paths never reach the + container. Raises whatever ``git`` raises; the caller falls back to the plain update. + """ + _hydrate_level(clone, donor, git=git, exists=exists, depth=depth) + git.sync_submodules(repo_path=clone) + + +def _hydrate_level( + repo_path: str, donor: str, *, git: GitClones, exists: Callable[[str], bool], depth: int +) -> None: + """One superproject level of :func:`hydrate_submodules`, then a recursion per submodule.""" + paths = git.submodule_paths(repo_path=repo_path) if depth > 0 else {} + if not paths: + return + git.init_submodules(repo_path=repo_path) + for name, path in paths.items(): + donor_sub = f"{donor.rstrip('/')}/{path}" + if exists(donor_sub): + git.set_submodule_url(repo_path=repo_path, name=name, url=donor_sub) + git.update_submodules(repo_path=repo_path, recursive=False) + for path in paths.values(): + _hydrate_level( + f"{repo_path.rstrip('/')}/{path}", + f"{donor.rstrip('/')}/{path}", + git=git, + exists=exists, + depth=depth - 1, + ) + + def _needs_submodules(states: Mapping[str, str]) -> bool: """Whether any of the repo's submodules isn't checked out yet. diff --git a/src/panopticon/terminal/quickstart.py b/src/panopticon/terminal/quickstart.py index 34efc6a1..ee7adaed 100644 --- a/src/panopticon/terminal/quickstart.py +++ b/src/panopticon/terminal/quickstart.py @@ -10,11 +10,11 @@ import subprocess from collections.abc import Callable -from pathlib import Path import httpx from panopticon.client import TaskServiceClient +from panopticon.core.git import is_forge_url, local_repo_path from panopticon.terminal.setup_repo_task import SETUP_REPO_WORKFLOW, create_setup_repo_task _FALLBACK_GIT_URL = "https://github.com/Unsupervisedcom/panopticon.git" @@ -28,9 +28,6 @@ _FORGE_WORKFLOW = "github-peer-reviewed" _LOCAL_WORKFLOW = "local-git-self-reviewed" -#: URL schemes that mean a networked (hosted-forge) remote rather than a local path. -_FORGE_SCHEMES = ("https://", "http://", "ssh://", "git://", "ftp://", "ftps://") - def _secrets_template() -> str: """The secrets-file template, read from the packaged ``panopticon.env.template`` data file.""" @@ -80,44 +77,13 @@ def repo_id_from_url(git_url: str) -> str: return tail.lower() or "repo" -def _is_forge_url(git_url: str) -> bool: - """True when ``git_url`` names a hosted-forge remote (network push/PR/CI), not a local path. - - Recognizes URL-scheme remotes (``https://…``, ``ssh://…``, …) and scp-like ``user@host:path`` - remotes; treats a bare filesystem path or a ``file://`` URL as local-only. - """ - url = git_url.strip() - if url.lower().startswith("file://"): - return False - if url.lower().startswith(_FORGE_SCHEMES): - return True - # scp-like syntax: user@host:path — an '@' and a ':' before any '/'. A Windows drive path - # (``C:\…``) has the ':' but no '@', so it stays local. - at, colon, slash = url.find("@"), url.find(":"), url.find("/") - return at != -1 and colon > at and (slash == -1 or colon < slash) - - def choose_enabled_workflow(git_url: str) -> str: """The opt-in workflow quickstart enables for a repo, chosen from its remote URL. A hosted-forge remote gets the forge lifecycle (``github-peer-reviewed``); a local-only repo gets the forge-free ``local-git-self-reviewed``. """ - return _FORGE_WORKFLOW if _is_forge_url(git_url) else _LOCAL_WORKFLOW - - -def local_repo_path(git_url: str) -> str | None: - """The filesystem path ``git_url`` names, or ``None`` when it names a networked remote. - - The counterpart of :func:`_is_forge_url`: a bare path or a ``file://`` URL is somewhere on this - host, which is what makes panopticon's host-side push (and the config below) possible at all. - """ - if _is_forge_url(git_url): - return None - url = git_url.strip() - if url.lower().startswith("file://"): - url = url[len("file://") :] - return str(Path(url).expanduser()) if url else None + return _FORGE_WORKFLOW if is_forge_url(git_url) else _LOCAL_WORKFLOW def allow_pushes_into_local_repo( diff --git a/tests/core/test_git.py b/tests/core/test_git.py index 20ce3c0c..1a4f4cea 100644 --- a/tests/core/test_git.py +++ b/tests/core/test_git.py @@ -20,6 +20,9 @@ GitWorktrees, Worktree, branch_name, + is_forge_url, + local_repo_path, + parse_submodule_paths, parse_submodule_status, worktree_path, ) @@ -160,6 +163,84 @@ def test_update_submodules_is_recursive_and_allows_local_transports() -> None: ] +def test_update_submodules_can_do_one_level_only() -> None: + # The donor hydration walks the tree a level at a time: a nested submodule's URL can't be + # resolved (or redirected at the donor) before its parent has been checked out. + rec = _Recorder() + GitClones(run=rec).update_submodules(repo_path="/tasks/t1", recursive=False) + assert "--recursive" not in rec.calls[0][0] + + +def test_submodule_paths_reads_the_declared_submodules() -> None: + rec = _Recorder() + GitClones(run=rec).submodule_paths(repo_path="/tasks/t1") + # Read from `.gitmodules`, so it answers before `submodule init` and for a submodule that has + # no checkout yet — and tolerant of a repo that declares none (git exits non-zero). + argv, check = rec.calls[0] + assert argv == [ + "git", + "-C", + "/tasks/t1", + "config", + "--file", + ".gitmodules", + "--get-regexp", + "^submodule\\..*\\.path$", + ] + assert check is False + + +def test_parse_submodule_paths_keeps_dotted_names_and_spaced_paths() -> None: + output = ( + "submodule.vendor/lib.path vendor/lib\n" + "submodule.my.lib.path vendor/my lib\n" # a name with a dot, a path with a space + "submodule.vendor/lib.url ../lib.git\n" # not a `.path` line + "junk\n" + ) + assert parse_submodule_paths(output) == { + "vendor/lib": "vendor/lib", + "my.lib": "vendor/my lib", + } + + +def test_init_set_url_and_sync_submodules() -> None: + rec = _Recorder() + clones = GitClones(run=rec) + clones.init_submodules(repo_path="/tasks/t1") + clones.set_submodule_url(repo_path="/tasks/t1", name="vendor/lib", url="/srv/widget/vendor/lib") + clones.sync_submodules(repo_path="/tasks/t1") + assert [argv for argv, _check in rec.calls] == [ + # `init` resolves the declared URLs into config, `config` overrides one with the donor's + # checkout, and `sync` puts the canonical URLs back afterwards (config *and* each + # submodule's own origin). + ["git", "-C", "/tasks/t1", "submodule", "init"], + ["git", "-C", "/tasks/t1", "config", "submodule.vendor/lib.url", "/srv/widget/vendor/lib"], + ["git", "-C", "/tasks/t1", "submodule", "sync", "--recursive"], + ] + + +# -- a repo's URL: hosted forge vs. a checkout on this host ------------------------- + + +@pytest.mark.parametrize( + "git_url", + ["https://github.com/x/y.git", "http://forge/y.git", "ssh://git@forge/y.git", "git@forge:x/y"], +) +def test_forge_urls_have_no_local_path(git_url: str) -> None: + assert is_forge_url(git_url) is True + assert local_repo_path(git_url) is None + + +def test_local_repo_paths_resolve_to_a_filesystem_path() -> None: + # What makes the host-side push — and cloning a task's submodules out of the repo's own + # checkout — possible at all. + assert local_repo_path("/srv/widget") == "/srv/widget" + assert local_repo_path("file:///srv/widget") == "/srv/widget" + assert local_repo_path("~/src/widget") == str(Path("~/src/widget").expanduser()) + assert local_repo_path(" ") is None + assert is_forge_url("C:\\src\\widget") is False # a drive letter isn't a scp-like remote + + def test_push_emits_a_plain_push() -> None: rec = _Recorder() GitClones(run=rec).push(repo_path="/tasks/t1", remote="origin", branch="main") diff --git a/tests/sessionservice/test_spawn.py b/tests/sessionservice/test_spawn.py index 0335f654..057d5fd4 100644 --- a/tests/sessionservice/test_spawn.py +++ b/tests/sessionservice/test_spawn.py @@ -5,7 +5,7 @@ import shutil import subprocess -from collections.abc import Callable +from collections.abc import Callable, Mapping, Sequence from pathlib import Path import pytest @@ -304,9 +304,209 @@ def docker_cleanup_fails(_path: str) -> None: assert renamed == [("/tasks/t1", "/tasks/t1.stale")] +#: A local ``git_url``: the repo's own checkout on this host, which spawn-prep can clone the +#: submodules out of instead of fetching them (the donor). +_LOCAL_REPO = {"id": "r1", "git_url": "/srv/widget"} + +_GITMODULES_REGEXP = ["config", "--file", ".gitmodules", "--get-regexp", "^submodule\\..*\\.path$"] + + +def _gitmodules(repo_path: str) -> list[str]: + return ["git", "-C", repo_path, *_GITMODULES_REGEXP] + + +def _hydration_runner( + *, + declared: Mapping[str, str], + statuses: Sequence[str], + fails: Callable[[list[str]], bool] = lambda _argv: False, +) -> tuple[list[list[str]], Callable[..., str]]: + """A fake ``git`` for the donor-hydration tests. + + ``declared`` maps a repo path to its ``.gitmodules`` ``--get-regexp`` output (so a nested + superproject can declare its own submodules); ``statuses`` answers successive + ``submodule status`` calls, the last one repeating; ``fails`` picks commands that blow up. + """ + calls: list[list[str]] = [] + remaining = list(statuses) + + def run(args: object, *, check: bool = True) -> str: + argv = list(args) # type: ignore[call-overload] + calls.append(argv) + if fails(argv): + raise subprocess.CalledProcessError(1, argv) + repo_path = argv[2] if len(argv) > 2 and argv[1] == "-C" else "" + if "status" in argv: + return remaining.pop(0) if len(remaining) > 1 else remaining[0] + if "--get-regexp" in argv: + return declared.get(repo_path, "") + return "" + + return calls, run + + +def _hydrate( + *, + declared: Mapping[str, str], + statuses: Sequence[str], + fails: Callable[[list[str]], bool] = lambda _argv: False, + exists: Callable[[str], bool] = lambda _p: True, +) -> list[list[str]]: + """Run ``prepare_workspace`` for a repo with a local checkout; return the emitted commands.""" + calls, run = _hydration_runner(declared=declared, statuses=statuses, fails=fails) + cache = CloneCache("/cache", run=run, exists=exists, makedirs=lambda _p: None) + prepare_workspace( + "t1", + _LOCAL_REPO, + cache=cache, + tasks_root="/tasks", + git=GitClones(run=run), + exists=exists, + makedirs=lambda _p: None, + ) + return calls + + +def test_prepare_clones_submodules_from_the_repos_own_checkout() -> None: + # The whole point: each submodule is a *local* clone off the repo's checkout on this host + # (hardlinked objects, no network), not a fetch from its forge paid once per task. + calls = _hydrate( + declared={"/tasks/t1": "submodule.vendor/lib.path vendor/lib\n"}, + statuses=[_UNINITIALIZED, ""], # uninitialized, then hydrated + ) + + assert calls[calls.index(_SUBMODULE_STATUS) :] == [ + _SUBMODULE_STATUS, + _gitmodules("/tasks/t1"), + ["git", "-C", "/tasks/t1", "submodule", "init"], + # …the resolved URL overridden with the donor's checkout of that submodule… + ["git", "-C", "/tasks/t1", "config", "submodule.vendor/lib.url", "/srv/widget/vendor/lib"], + # …and updated one level at a time (a nested submodule can't be resolved before its parent). + [ + "git", + "-C", + "/tasks/t1", + "-c", + "protocol.file.allow=always", + "submodule", + "update", + "--init", + ], + _gitmodules("/tasks/t1/vendor/lib"), # nothing nested below it + # `sync` puts the canonical URLs back, in the config and in each submodule's own origin. + ["git", "-C", "/tasks/t1", "submodule", "sync", "--recursive"], + _SUBMODULE_STATUS, # …and the result is checked, which is what gates the fallback + ] + assert _SUBMODULE_UPDATE not in calls # never fetched them + + +def test_prepare_hydrates_nested_submodules_from_the_matching_donor_level() -> None: + calls = _hydrate( + declared={ + "/tasks/t1": "submodule.vendor/lib.path vendor/lib\n", + "/tasks/t1/vendor/lib": "submodule.nested.path nested\n", + }, + statuses=[_UNINITIALIZED, ""], + ) + + # The donor is matched by *path*, level by level — never by URL, which the donor and the + # per-task clone resolve against different origins. + assert ["git", "-C", "/tasks/t1/vendor/lib", "submodule", "init"] in calls + assert [ + "git", + "-C", + "/tasks/t1/vendor/lib", + "config", + "submodule.nested.url", + "/srv/widget/vendor/lib/nested", + ] in calls + assert calls.count(["git", "-C", "/tasks/t1", "submodule", "sync", "--recursive"]) == 1 + + +def test_prepare_leaves_a_submodule_the_donor_lacks_pointing_at_its_own_url() -> None: + # The donor is an optimisation per submodule, not a precondition: one it hasn't checked out + # keeps the URL git resolved and is fetched as before. + calls = _hydrate( + declared={"/tasks/t1": "submodule.vendor/lib.path vendor/lib\n"}, + statuses=[_UNINITIALIZED, ""], + exists=lambda p: p != "/srv/widget/vendor/lib", + ) + + assert not [c for c in calls if c[3:4] == ["config"] and "submodule.vendor/lib.url" in c] + assert ["git", "-C", "/tasks/t1", "submodule", "init"] in calls + + +def test_prepare_fetches_submodules_when_the_repo_has_no_checkout_on_this_host() -> None: + # A hosted-forge `git_url` has no donor: exactly the previous behaviour, one recursive update. + calls, run = _hydration_runner(declared={}, statuses=[_UNINITIALIZED]) + cache = CloneCache("/cache", run=run, exists=lambda _p: True, makedirs=lambda _p: None) + + prepare_workspace( + "t1", + _REPO, + cache=cache, + tasks_root="/tasks", + git=GitClones(run=run), + exists=lambda _p: True, + makedirs=lambda _p: None, + ) + + assert calls[-1] == _SUBMODULE_UPDATE + assert not [c for c in calls if "--get-regexp" in c] + + +def test_prepare_fetches_submodules_when_the_donor_checkout_is_gone() -> None: + calls = _hydrate( + declared={"/tasks/t1": "submodule.vendor/lib.path vendor/lib\n"}, + statuses=[_UNINITIALIZED], + exists=lambda p: p != "/srv/widget", # the registered repo path isn't there any more + ) + + assert calls[-1] == _SUBMODULE_UPDATE + + +def test_prepare_fetches_submodules_when_hydrating_from_the_donor_fails() -> None: + calls = _hydrate( + declared={"/tasks/t1": "submodule.vendor/lib.path vendor/lib\n"}, + statuses=[_UNINITIALIZED], + fails=lambda argv: argv[3:] == ["submodule", "init"], + ) + + assert calls[-1] == _SUBMODULE_UPDATE # a broken donor is never worse than no donor + + +def test_prepare_fetches_submodules_when_hydration_leaves_one_uninitialized() -> None: + # E.g. the donor is behind and lacks the commit the superproject records: the status gate + # catches it and the plain update fetches what's missing. + calls = _hydrate( + declared={"/tasks/t1": "submodule.vendor/lib.path vendor/lib\n"}, + statuses=[_UNINITIALIZED], + ) + + assert calls[-1] == _SUBMODULE_UPDATE + + # -- integration: a real repo with a real submodule --------------------------------- +def _git(*args: str, cwd: Path) -> str: + return subprocess.run( + ["git", *args], cwd=cwd, check=True, capture_output=True, text=True + ).stdout + + +def _init_repo(path: Path) -> None: + path.mkdir(parents=True) + _git("init", "--initial-branch", "main", cwd=path) + _git("config", "user.email", "t@example.com", cwd=path) + _git("config", "user.name", "t", cwd=path) + + +def _add_submodule(superproject: Path, url: str, path: str) -> None: + _git("-c", "protocol.file.allow=always", "submodule", "add", url, path, cwd=superproject) + _git("commit", "--message", f"add {path}", cwd=superproject) + + @pytest.mark.skipif(not shutil.which("git"), reason="needs git") def test_prepare_fills_in_a_real_submodule_that_survives_relocation(tmp_path: Path) -> None: """The per-task checkout gets the submodule's content — and keeps it when moved. @@ -316,15 +516,7 @@ def test_prepare_fills_in_a_real_submodule_that_survives_relocation(tmp_path: Pa the submodule's gitdir/worktree links have to be relative. They are, but only because ``submodule update`` writes them that way — worth pinning. """ - - def git(*args: str, cwd: Path) -> None: - subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True) - - def init(path: Path) -> None: - path.mkdir() - git("init", "--initial-branch", "main", cwd=path) - git("config", "user.email", "t@example.com", cwd=path) - git("config", "user.name", "t", cwd=path) + git, init = _git, _init_repo lib = tmp_path / "lib" # the submodule's own repo init(lib) @@ -339,16 +531,7 @@ def init(path: Path) -> None: git("commit", "--message", "init", cwd=forge) # A *relative* submodule URL — the common case, and the one that only resolves correctly # because prepare_workspace repoints origin at the forge before initializing submodules. - git( - "-c", - "protocol.file.allow=always", - "submodule", - "add", - "../lib", - "vendor/lib", - cwd=forge, - ) - git("commit", "--message", "add submodule", cwd=forge) + _add_submodule(forge, "../lib", "vendor/lib") cache = CloneCache(str(tmp_path / "cache")) clone = Path( @@ -369,3 +552,87 @@ def init(path: Path) -> None: check=True, capture_output=True, ) # the submodule is still a working repo at its new path + + +@pytest.mark.skipif(not shutil.which("git"), reason="needs git") +def test_prepare_hardlinks_real_submodules_out_of_the_source_repo(tmp_path: Path) -> None: + """Submodules — nested ones included — are cloned out of the repo's own checkout on this host. + + The two properties that make this worth doing at all: the objects are **hardlinked** from the + source repo rather than fetched (that's the cost saving), and once hydration is done nothing + in the checkout points at the donor any more (``submodule sync`` restores the canonical URLs), + so the container gets a repo it can fetch and push normally. + """ + inner = tmp_path / "inner" # a submodule of the submodule + _init_repo(inner) + (inner / "I").write_text("innerfile") + _git("add", "--all", cwd=inner) + _git("commit", "--message", "init", cwd=inner) + + lib = tmp_path / "lib" + _init_repo(lib) + (lib / "L").write_text("libfile") + _git("add", "--all", cwd=lib) + _git("commit", "--message", "init", cwd=lib) + _add_submodule(lib, str(inner), "nested") + + source = tmp_path / "source" # the repo's checkout on this host — registered as its git_url + _init_repo(source) + (source / "T").write_text("top") + _git("add", "--all", cwd=source) + _git("commit", "--message", "init", cwd=source) + _add_submodule(source, "../lib", "vendor/lib") + _git( + "-c", + "protocol.file.allow=always", + "submodule", + "update", + "--init", + "--recursive", + cwd=source, + ) + + # Move the submodules' own repos out of the way: the *only* copy of their objects left on + # this host is the source repo's, so a task that filled its submodules in the old way — by + # fetching each one from its URL — would now fail outright. Hydration has to find them. + lib.rename(tmp_path / "lib.gone") + inner.rename(tmp_path / "inner.gone") + + clone = Path( + prepare_workspace( + "t1", + {"id": "r1", "git_url": str(source)}, + cache=CloneCache(str(tmp_path / "cache")), + tasks_root=str(tmp_path / "tasks"), + ) + ) + + assert (clone / "vendor" / "lib" / "L").read_text() == "libfile" + assert (clone / "vendor" / "lib" / "nested" / "I").read_text() == "innerfile" + + # Cheap: the submodules' object stores are the source repo's, hardlinked — no bytes copied + # and no fetch. (Same filesystem here; a cross-filesystem donor degrades to a copy.) + for module in ("vendor/lib", "vendor/lib/modules/nested"): + objects = clone / ".git" / "modules" / module / "objects" + shared = [ + obj + for obj in objects.rglob("*") + if obj.is_file() + and obj.stat().st_nlink > 1 + and obj.stat().st_ino + == (source / ".git" / "modules" / module / "objects" / obj.relative_to(objects)) + .stat() + .st_ino + ] + assert shared, f"{module} objects were copied or fetched, not hardlinked" + + # …and nothing points at the donor afterwards: the canonical URLs are back in the config and + # in each submodule's own origin, so the container fetches and pushes where it should. + config = _git("config", "--get-regexp", "^submodule\\.", cwd=clone) + assert str(source / "vendor") not in config + assert _git("config", "remote.origin.url", cwd=clone / "vendor" / "lib").strip() == str( + tmp_path / "lib" + ) + assert _git("config", "remote.origin.url", cwd=clone / "vendor" / "lib" / "nested").strip() == ( + str(inner) + )