Stop the server rewriting the repositories it serves, and add git-collab-server migrate #81

Merged

a73x merged 1 commit from worktree-agent-a32970d20c5f1d961 into main

2026-08-13 09:25:26 +00:00

a73x commented

2026-08-13 07:00:10 +00:00

(Migrated from git.a73x.sh)

state::list_patches called migrate_patch_layout, and the server calls
list_patches on every repo page. So rendering wrote to the repository
being rendered: any anonymous visitor mutated it, with no lock, able to
race a concurrent request or an in-flight receive-pack.

The crux is not "stop calling migrate" — it is that the read path had no
way to see an unmigrated patch at all. patch_refs_under globbed
<id>/events, which a bare refs/collab/patches/<id> does not match, so
without the migration the patch was simply invisible and the page would
have rendered an empty list instead of writing. Reads now tolerate both
layouts: the bare ref is the DAG and is walked where it lies. The two
shapes cannot coexist for one id — git will not have <id> be both a ref
and a directory — so there is nothing to disambiguate.

Migration moves to the callers that own their repository. The CLI does it
once at its entry point, because writes genuinely need it:
<id>/rev/<oid> and a bare <id> are a directory/file conflict git
refuses. sync keeps the call it already had. The server has none.

And an operator gets git-collab-server migrate, which is what makes the
sequencing work: once e5096ffc strips legacy support an unmigrated repo
will fail to read, and this is how you convert a roster before the
upgrade rather than from the wreckage after it. It walks repos_dir by
the server's own discovery rules, reports every repository including the
ones with nothing to do, and has a dry run.

It also fixes the diagnostic the old path never gave. Failures were
printed per patch on stderr with no repository name and nothing returned,
so a read-only mount produced noise and no verdict; now each repository is
probed for writability first — by writing, since a read-only mount leaves
the mode bits saying 0755 — and reported by name, with the run exiting
non-zero if anything was left behind.

Locking: each repository is migrated under its own SyncLock, the
advisory lock git-collab sync already takes, so a concurrent sync is
refused rather than raced. That is not a guarantee against git receive-pack, which takes no such lock; what covers a push is narrower
and comes from git itself, since every ref written here goes through git2
and takes the per-ref lockfile. A push landing mid-run can still leave the
report stale, which is what the dry run is for.

Tests: an old-layout patch served over real HTTP across every page that
renders patches, with all of refs/collab/** byte-identical afterwards —
and the same patch asserted present on the list, the overview and its
detail page, because not writing is worthless if it means not reading.
Plus migrate's own behaviour: it migrates, it is idempotent, it walks
nested repos and skips dot-prefixed paths, it names a repository it cannot
write without letting that stop the others, and its dry run changes
nothing.

Migrated from git-collab patche 46c3cdc545932cd8dc80206d8b99c53e977abb62.

Original author: a73x dev@a73x.sh.

Fixes #80

Revision 1: 0086f3c774c2e9e353781b7d30b18ba44763d17a; base 15bc9a3c2f8c1622b6eb5157c6176452ba7569bb

`state::list_patches` called `migrate_patch_layout`, and the server calls `list_patches` on every repo page. So rendering wrote to the repository being rendered: any anonymous visitor mutated it, with no lock, able to race a concurrent request or an in-flight `receive-pack`. The crux is not "stop calling migrate" — it is that the read path had no way to see an unmigrated patch at all. `patch_refs_under` globbed `<id>/events`, which a bare `refs/collab/patches/<id>` does not match, so without the migration the patch was simply invisible and the page would have rendered an empty list instead of writing. Reads now tolerate both layouts: the bare ref *is* the DAG and is walked where it lies. The two shapes cannot coexist for one id — git will not have `<id>` be both a ref and a directory — so there is nothing to disambiguate. Migration moves to the callers that own their repository. The CLI does it once at its entry point, because writes genuinely need it: `<id>/rev/<oid>` and a bare `<id>` are a directory/file conflict git refuses. `sync` keeps the call it already had. The server has none. And an operator gets `git-collab-server migrate`, which is what makes the sequencing work: once e5096ffc strips legacy support an unmigrated repo will fail to read, and this is how you convert a roster *before* the upgrade rather than from the wreckage after it. It walks `repos_dir` by the server's own discovery rules, reports every repository including the ones with nothing to do, and has a dry run. It also fixes the diagnostic the old path never gave. Failures were printed per patch on stderr with no repository name and nothing returned, so a read-only mount produced noise and no verdict; now each repository is probed for writability first — by writing, since a read-only mount leaves the mode bits saying 0755 — and reported by name, with the run exiting non-zero if anything was left behind. Locking: each repository is migrated under its own `SyncLock`, the advisory lock `git-collab sync` already takes, so a concurrent sync is refused rather than raced. That is not a guarantee against `git receive-pack`, which takes no such lock; what covers a push is narrower and comes from git itself, since every ref written here goes through git2 and takes the per-ref lockfile. A push landing mid-run can still leave the report stale, which is what the dry run is for. Tests: an old-layout patch served over real HTTP across every page that renders patches, with all of `refs/collab/**` byte-identical afterwards — and the same patch asserted present on the list, the overview and its detail page, because not writing is worthless if it means not reading. Plus migrate's own behaviour: it migrates, it is idempotent, it walks nested repos and skips dot-prefixed paths, it names a repository it cannot write without letting that stop the others, and its dry run changes nothing. --- Migrated from git-collab patche `46c3cdc545932cd8dc80206d8b99c53e977abb62`. Original author: a73x <dev@a73x.sh>. Fixes #80 Revision 1: `0086f3c774c2e9e353781b7d30b18ba44763d17a`; base `15bc9a3c2f8c1622b6eb5157c6176452ba7569bb`