c6de5ea1
Name revision refs by commit OID instead of revision number
a73x 2026-08-09 19:12
Commit message
docs/superpowers/specs/2026-08-09-patch-revision-refs-design.md
| Old | New | ||
|---|---|---|---|
| @@ -28,13 +28,32 @@ they are client-side. | |||
| 28 | ## Ref layout | 28 | ## Ref layout |
| 29 | 29 | ||
| 30 | ``` | 30 | ``` |
| 31 | refs/collab/patches/<id>/events the event DAG (today's refs/collab/patches/<id>) | 31 | refs/collab/patches/<id>/events the event DAG (today's refs/collab/patches/<id>) |
| 32 | refs/collab/patches/<id>/r/<n> revision n's commit — immutable, write-once | 32 | refs/collab/patches/<id>/rev/<oid> a revision's commit, pinned |
| 33 | ``` | 33 | ``` |
| 34 | 34 | ||
| 35 | The `events` rename is forced: git will not let `<id>` be both a ref and a | 35 | The `events` rename is forced: git will not let `<id>` be both a ref and a |
| 36 | directory. | 36 | directory. |
| 37 | 37 | ||
| 38 | **Revision refs are named by commit OID, not by revision number.** An earlier | ||
| 39 | draft used `r/<n>`, and it does not survive contact with concurrency: two clones | ||
| 40 | revising offline each claim `r/2`, and the loser's push is rejected as a | ||
| 41 | non-fast-forward *permanently* — a number the remote has already published cannot | ||
| 42 | be renegotiated, so sync wedges on every retry. Reconciling the numbering across | ||
| 43 | clones is possible but makes `r/<n>` stop meaning "revision n": three clones were | ||
| 44 | observed converging on `r/2`, `r/3`, `r/4` pointing at the DAG's revisions 4, 3 | ||
| 45 | and 2, consistently and permanently. | ||
| 46 | |||
| 47 | Naming by OID dissolves the problem rather than managing it. The name is derived | ||
| 48 | from the content, so a ref never moves, two clones can never disagree about it, | ||
| 49 | and every push is additive by construction. Revision *numbering* stays purely a | ||
| 50 | property of the DAG, which is where it already lives and where it is already | ||
| 51 | authoritative — "what is revision 2" is answered by walking events, exactly as | ||
| 52 | today. | ||
| 53 | |||
| 54 | The refs' only job is to keep revision commits reachable. That job needs a set, | ||
| 55 | not a sequence. | ||
| 56 | |||
| 38 | `sync` already pushes and fetches `refs/collab/patches/*` (`sync.rs:230-233`, | 57 | `sync` already pushes and fetches `refs/collab/patches/*` (`sync.rs:230-233`, |
| 39 | `sync.rs:344-347`). `*` crosses `/` in both git refspecs and git2's | 58 | `sync.rs:344-347`). `*` crosses `/` in both git refspecs and git2's |
| 40 | `references_glob`, so push, fetch and enumeration genuinely need no change — this | 59 | `references_glob`, so push, fetch and enumeration genuinely need no change — this |
| @@ -46,13 +65,20 @@ workflow becomes `git-collab sync`, where it is currently `git push origin | |||
| 46 | fetched ref under the prefix as an event DAG. Revision refs point at *source* | 65 | fetched ref under the prefix as an event DAG. Revision refs point at *source* |
| 47 | commits, which carry no event signature, so it would run `signing::verify_ref` | 66 | commits, which carry no event signature, so it would run `signing::verify_ref` |
| 48 | over them and reject every patch on sync. Reconciliation must classify by ref | 67 | over them and reject every patch on sync. Reconciliation must classify by ref |
| 49 | shape: `<id>/events` reconciles as a DAG, `<id>/r/<n>` is adopted write-once | 68 | shape: `<id>/events` reconciles as a DAG, revision refs are handled separately. |
| 50 | without verification. Stating that the refspecs are unchanged is true and | 69 | Stating that the refspecs are unchanged is true and insufficient; the code behind |
| 51 | insufficient; the code behind them is not. | 70 | them is not. |
| 52 | 71 | ||
| 53 | Revision refs are write-once. A revision is never rewritten; a changed patch adds | 72 | Revision refs are never adopted from a remote. They are derived from the |
| 54 | `r/<n+1>`. This makes the objects behind every revision permanently reachable — | 73 | reconciled, signed DAG — a ref is written for each commit the DAG lists, and |
| 55 | today they survive only while the branch happens to still reach them, so an | 74 | nothing outside the DAG can be referenced. A remote cannot plant a reachable |
| 75 | object by pushing a ref the local DAG does not vouch for, so the boundary is | ||
| 76 | closed by construction rather than by validation. | ||
| 77 | |||
| 78 | Revision refs are write-once, and with OID naming this is trivially true: a ref | ||
| 79 | whose name is its content has no reason to move. This makes the objects behind | ||
| 80 | every revision permanently reachable — today they survive only while the branch | ||
| 81 | happens to still reach them, so an | ||
| 56 | author who rebases and force-pushes silently strips earlier revisions of their | 82 | author who rebases and force-pushes silently strips earlier revisions of their |
| 57 | blobs and leaves interdiff and revision-anchored inline comments | 83 | blobs and leaves interdiff and revision-anchored inline comments |
| 58 | (`state.rs:133`, `state.rs:197`) pointing at objects `git gc` is entitled to | 84 | (`state.rs:133`, `state.rs:197`) pointing at objects `git gc` is entitled to |
| @@ -146,10 +172,10 @@ Four things depend on `PatchState.branch` today and each needs a replacement. | |||
| 146 | 172 | ||
| 147 | | Today | After | | 173 | | Today | After | |
| 148 | |---|---| | 174 | |---|---| |
| 149 | | `resolve_head` looks up `refs/heads/<branch>` (`state.rs:433-446`) | Latest `r/<n>` | | 175 | | `resolve_head` looks up `refs/heads/<branch>` (`state.rs:433-446`) | Latest revision's commit, from the DAG | |
| 150 | | Staleness, ahead/behind against base tip (`state.rs:448-456`) | Unchanged; anchored to latest `r/<n>` | | 176 | | Staleness, ahead/behind against base tip (`state.rs:448-456`) | Unchanged; anchored to the latest revision | |
| 151 | | Merged detection (`check_auto_merge`, `state.rs:464-486`) | See below — not unchanged | | 177 | | Merged detection (`check_auto_merge`, `state.rs:464-486`) | See below — not unchanged | |
| 152 | | `patch checkout` creates a local branch (`patch.rs:663-668`) | Unchanged; branches from `r/<n>` | | 178 | | `patch checkout` creates a local branch (`patch.rs:663-668`) | Unchanged; branches from the latest revision | |
| 153 | 179 | ||
| 154 | `resolve_head` already accepts an OID before falling back to a name lookup | 180 | `resolve_head` already accepts an OID before falling back to a name lookup |
| 155 | (`state.rs:436`), so the OID path is partly built. | 181 | (`state.rs:436`), so the OID path is partly built. |
| @@ -168,7 +194,7 @@ It also calls `resolve_head`, so today it silently no-ops when | |||
| 168 | returns, and the patch stays `Open` with no diagnostic. This was confirmed | 194 | returns, and the patch stays `Open` with no diagnostic. This was confirmed |
| 169 | behaviourally while writing server tests: merge detection requires the source | 195 | behaviourally while writing server tests: merge detection requires the source |
| 170 | branch to have been pushed to the server, not merely the base branch. Anchoring | 196 | branch to have been pushed to the server, not merely the base branch. Anchoring |
| 171 | to `r/<n>` removes that requirement, since the revision ref is pushed by `sync` | 197 | to the revision refs removes that requirement, since they are pushed by `sync` |
| 172 | whether or not any branch was. | 198 | whether or not any branch was. |
| 173 | 199 | ||
| 174 | Both are reasons the "Merged detection" row is not the no-op it first appears to | 200 | Both are reasons the "Merged detection" row is not the no-op it first appears to |
| @@ -184,7 +210,7 @@ same operation Gerrit performs on a push to `refs/for`, moved to the client. | |||
| 184 | 210 | ||
| 185 | `delete` removes a single reference (`patch.rs:677-681`) and `close` archives one | 211 | `delete` removes a single reference (`patch.rs:677-681`) and `close` archives one |
| 186 | via `state::archive_patch_ref` (`patch.rs:694-698`). Both now operate on a | 212 | via `state::archive_patch_ref` (`patch.rs:694-698`). Both now operate on a |
| 187 | subtree: `<id>/events` plus every `<id>/r/<n>`. Archiving must move the whole | 213 | subtree: `<id>/events` plus every `<id>/rev/<oid>`. Archiving must move the whole |
| 188 | subtree under `refs/collab/archive/patches/<id>/` so that closed patches keep | 214 | subtree under `refs/collab/archive/patches/<id>/` so that closed patches keep |
| 189 | their revisions and remain reviewable. | 215 | their revisions and remain reviewable. |
| 190 | 216 | ||
| @@ -194,7 +220,7 @@ Pre-release; no compatibility guarantee. Patches in the old layout are migrated | |||
| 194 | on first use rather than supported indefinitely: | 220 | on first use rather than supported indefinitely: |
| 195 | 221 | ||
| 196 | - `refs/collab/patches/<id>` becomes `<id>/events`. | 222 | - `refs/collab/patches/<id>` becomes `<id>/events`. |
| 197 | - Each recorded revision with a non-empty `commit` gets an `r/<n>` ref, provided | 223 | - Each recorded revision with a non-empty `commit` gets a `rev/<oid>` ref, provided |
| 198 | the object is still present. Revisions whose objects were already lost to a | 224 | the object is still present. Revisions whose objects were already lost to a |
| 199 | force-push cannot be recovered and keep today's `""`-means-unknown convention | 225 | force-push cannot be recovered and keep today's `""`-means-unknown convention |
| 200 | (`state.rs:164-167`). | 226 | (`state.rs:164-167`). |