specs/015-gerrit-style-patchsets/spec.md

Ref: main   Size: 19.6 KiB

# Feature Specification: Patch Revisions

**Feature Branch**: `015-gerrit-style-patchsets`
**Created**: 2026-03-21
**Status**: Draft
**Input**: Patches-as-branches are redundant when users share a git host — branches already exist. The differentiator is revision-aware review (à la Radicle/Gerrit): numbered revisions, comments anchored to specific revisions, and interdiffs between revisions. This is the thing GitHub PRs are actually bad at.

## Motivation

The current patch model wraps a git branch with a review conversation. When collaborators share a remote, the branch already exists — the collab layer adds review metadata but doesn't solve any problem that GitHub/GitLab PRs don't already handle better.

What Radicle and Gerrit get right (and GitHub gets wrong) is **revision-aware review**:

1. A reviewer leaves inline comments on revision 1 of the code
2. The author amends/rebases and pushes revision 2
3. The reviewer can see: what changed between r1 and r2 (interdiff), which comments are on older code, and which lines are untouched
4. Comments stay anchored to the revision they were made on — they don't float to wrong line numbers

This spec adds numbered revisions to git-collab patches, following the Radicle model (patch + revisions) rather than the Gerrit model (change + patch sets).

## User Scenarios & Testing *(mandatory)*

### User Story 1 - Automatic Revision Recording on Write Operations (Priority: P1)

The system automatically detects when the branch tip has changed since the last recorded revision. This detection happens only during **write operations** that touch the patch DAG — `patch comment`, `patch review`, `patch merge`, and `patch create`. Before appending their own event, these commands check the branch tip against the last recorded revision's commit OID. If they differ, a `PatchRevision` event is appended first, then the actual event. Read operations (`patch show`, `patch list`, `patch diff`, TUI) never modify the DAG.

Revision numbers are **not stored in events** — they are derived from DAG order during materialization. The Nth `PatchRevision` event (in topological order) is revision N+1 (revision 1 comes from `PatchCreate`). This avoids conflicts when two collaborators independently detect the same branch change.

**Why this priority**: Without revision recording, nothing else (anchored comments, interdiff) works. Automatic recording on writes means developers don't need to learn a new command — revisions appear naturally as the review conversation progresses.

**Independent Test**: Create a patch (revision 1 from create), push a commit, run `patch comment`, verify a PatchRevision event was auto-inserted before the comment event.

**Acceptance Scenarios**:

1. **Given** a patch exists with revision 1, **When** the developer pushes new commits and runs `patch comment <id>`, **Then** a PatchRevision event is auto-inserted before the comment, and the comment is anchored to revision 2.
2. **Given** the branch hasn't changed since the last revision, **When** the developer runs `patch comment`, **Then** no PatchRevision is inserted — the comment is anchored to the current revision.
3. **Given** a patch is created with `patch create`, **Then** revision 1 is recorded in the PatchCreate event with the branch tip commit OID and tree OID.
4. **Given** a patch has 3 revisions, **When** viewing the patch, **Then** all 3 revisions are listed with their number (derived from DAG order), commit OID, and timestamp.
5. **Given** a developer rebases their branch (force-push), **When** the next write operation touches the patch, **Then** a PatchRevision event is auto-inserted capturing the post-rebase state.
6. **Given** the branch has been deleted, **When** a write operation attempts auto-detection, **Then** no PatchRevision is inserted — the branch is simply unavailable.
7. **Given** two collaborators independently detect the same branch change and create PatchRevision events, **When** their DAGs are synced and merged, **Then** revision numbers are derived from topological order — both events contribute but the numbering remains consistent after materialization.

---

### User Story 2 - Inline Comments Anchored to Revisions (Priority: P1)

A reviewer leaves an inline comment on a specific file and line. The comment is anchored to the current revision number. Comments belong to the revision they were made on — period. There is no attempt to track or map comments across revisions. When viewing a specific revision, you see its comments. When viewing a different revision, you see that revision's comments. This is the Radicle model: each revision is its own self-contained review surface.

**Why this priority**: This is the core value proposition — without revision-anchored comments, we're just a worse GitHub PR.

**Independent Test**: Create a patch, leave an inline comment on revision 1, publish revision 2, leave a different comment on revision 2, verify each revision shows only its own comments.

**Acceptance Scenarios**:

1. **Given** a patch is on revision 3, **When** a reviewer adds an inline comment, **Then** the comment is recorded with `revision: 3` in the event.
2. **Given** comments exist on revisions 1 and 3, **When** viewing with `patch show <id> --revision 1`, **Then** only revision 1's comments are shown.
3. **Given** `patch show <id>` is run without `--revision`, **Then** comments from all revisions are shown, grouped by revision number.
4. **Given** a reviewer is viewing revision 2, **When** they add an inline comment, **Then** the comment is anchored to revision 2 regardless of what the latest revision is.

---

### User Story 3 - View Interdiff Between Revisions (Priority: P1)

A reviewer wants to see only what changed between two revisions of the patch, not the full diff against the base. They run `patch diff <id> --between 1 2` and see the diff between revision 1's tree and revision 2's tree. This is the critical "what did the author actually change in response to my feedback" view.

**Why this priority**: This is the killer feature that makes revisions useful. Without interdiff, numbered revisions are just bookmarks.

**Independent Test**: Create a patch with 3 revisions where each modifies different files, run `patch diff --between 1 3`, verify the diff shows only the changes between those two snapshots.

**Acceptance Scenarios**:

1. **Given** a patch has revisions 1 and 2, **When** running `patch diff <id> --between 1 2`, **Then** the output is the unified diff between revision 1's tree and revision 2's tree.
2. **Given** a patch has revisions 1, 2, and 3, **When** running `patch diff <id> --between 1 3`, **Then** the diff spans from revision 1 to 3, skipping 2.
3. **Given** a patch has only 1 revision, **When** running `patch diff <id> --between 1 2`, **Then** the system errors with "revision 2 not found".
4. **Given** `patch diff <id>` is run without `--between`, **Then** behavior is unchanged — shows the diff between merge-base and current branch tip (the latest code).
5. **Given** revisions 1 and 2 have identical trees (edge case: metadata-only revision), **Then** interdiff is empty.

---

### User Story 4 - View Revision Log (Priority: P2)

A reviewer or author wants to see the revision history of a patch — all revisions with their numbers, timestamps, commit OIDs, and which files changed between each consecutive pair. This gives a timeline of how the patch evolved.

**Why this priority**: Useful context for reviewers picking up a patch mid-conversation, but not blocking for the core review workflow.

**Independent Test**: Create a patch with 3 revisions, run `patch log <id>`, verify all 3 are listed with timestamps and file-change summaries.

**Acceptance Scenarios**:

1. **Given** a patch with 3 revisions, **When** running `patch log <id>`, **Then** the output lists each revision with number, timestamp, commit short OID, and a summary like "+2 -1 files changed" relative to the previous revision.
2. **Given** `--json` flag, **Then** the output is a JSON array of revision objects.
3. **Given** a patch with only 1 revision, **Then** the log shows just revision 1 with "(initial)" label.

---

### User Story 5 - Review Anchored to Revisions (Priority: P2)

A reviewer submits a review (approve/request-changes). The review is anchored to the current revision. Reviews belong to their revision, same as comments. `patch show` displays the latest review per reviewer with its revision number, so it's clear whether the approval is on the current code or an older version.

**Why this priority**: Important for understanding review state, but the core approve/request-changes mechanics already exist.

**Acceptance Scenarios**:

1. **Given** a reviewer approves on revision 3 and revision 4 is recorded, **When** viewing the patch, **Then** the approval is shown as "approved (revision 3)".
2. **Given** a reviewer requests changes on revision 2, **When** viewing on revision 4, **Then** the request-changes review is shown with its revision number.
3. **Given** `patch show`, **Then** the latest review per reviewer is highlighted, with revision context.
4. **Given** a configurable merge policy exists, **When** merging, **Then** the policy determines whether approval must be on the latest revision. See FR-014.

---

### Edge Cases

- What if the branch is deleted when auto-detection runs? No new revision is created. The last recorded revision remains current.
- What if the base branch has moved significantly between revisions? The interdiff still works — it compares the two revision trees directly, independent of the base.
- What about merge commits in the branch? The revision snapshots the tree at the branch tip, regardless of history shape.
- What if a reviewer comments on a revision that doesn't exist yet (race condition in distributed sync)? The comment is stored with its revision number; if the revision doesn't exist locally, it's displayed with "(revision N — not yet synced)".
- What if the tree hasn't changed but commits were amended (message-only change)? Auto-detection compares commit OID, not tree OID, so a new revision is recorded. The interdiff will show no code changes.
- What about auto-recording requiring a signing key? The auto-detection must have access to the signing key to append a DAG event. If the key is unavailable (e.g., read-only clone), revision detection is skipped silently — the patch still works, just without new revision tracking.
- Thread comments (`patch.comment`) are NOT revision-anchored — they are patch-level conversation that spans the whole lifecycle.

## Requirements *(mandatory)*

### Functional Requirements

- **FR-001**: `patch create` MUST record revision 1 in the PatchCreate event, capturing the branch tip commit OID and tree OID.
- **FR-002**: Write operations (`patch comment`, `patch review`, `patch merge`) MUST automatically detect when the branch tip differs from the last recorded revision and prepend a `PatchRevision` event before their own event.
- **FR-003**: Auto-detection MUST compare commit OIDs (not tree OIDs) to decide whether a new revision is needed.
- **FR-003a**: Revision numbers MUST be derived from topological DAG order during materialization, NOT stored in events. The Nth `PatchRevision` in topological order is revision N+1.
- **FR-003b**: During materialization, `PatchRevision` events with the same commit OID MUST be deduplicated — only the first (in topological order) contributes a revision boundary. This prevents duplicate revisions when collaborators independently detect the same branch change.
- **FR-004**: Inline comments (`patch.inline_comment` events) MUST include a `revision` field recording the current revision number at the time of commenting. The `--revision N` flag on `patch comment` allows targeting a specific revision; without it, the latest revision is used.
- **FR-005**: Reviews (`patch.review` events) MUST include a `revision` field recording the current revision number at the time of review. The `--revision N` flag on `patch review` allows targeting a specific revision; without it, the latest revision is used.
- **FR-006**: `patch diff <id> --between N M` MUST compute the diff between revision N's tree and revision M's tree. `--between N` (single argument) is shorthand for "revision N to latest revision".
- **FR-007**: `patch diff <id>` without `--between` or `--revision` MUST continue to show the merge-base diff against the current branch tip (existing behavior).
- **FR-007a**: `patch diff <id> --revision N` MUST show the diff between the merge-base and revision N's tree (historical full diff).
- **FR-008**: `patch show` MUST display the current revision number, list all revisions, and annotate inline comments/reviews with their revision context.
- **FR-009**: `patch show --revision N` MUST filter to show only inline comments and reviews from revision N.
- **FR-010**: `patch log <id>` MUST list all revisions with number, timestamp, commit OID, and file-change summary.
- **FR-011**: Revision data MUST be stored in the collab DAG (not external state), so it syncs via the existing sync mechanism.
- **FR-012**: Merge policy MUST be configurable via `refs/collab/config` (project-level, synced between collaborators): `merge.require_approval_on_latest` (boolean, default false). When true, `patch merge` requires at least one approval on the latest revision. When false, any prior approval suffices (current behavior).

### Key Entities

- **Revision**: A numbered snapshot of the branch state at a point in time. Contains: revision number (1-indexed, monotonically increasing), commit OID (branch tip), tree OID (for fast interdiff), and timestamp. Stored as `PatchRevision` events in the review DAG. Revision 1 is embedded in the `PatchCreate` event.
- **Anchored Comment**: An inline comment or review that references a specific revision number.
- **Interdiff**: The diff between two revision trees. Computed on-the-fly from stored tree OIDs, not pre-computed.

### Event Schema Changes

New event type (revision number omitted — derived from DAG order):
```json
{
  "type": "patch.revision",
  "commit": "abc123...",
  "tree": "def456...",
  "body": "addressed review feedback on error handling"
}
```

The `body` field is optional — omitted when auto-detected on write operations, provided when the author runs `patch revise --body "..."` explicitly.

Modified `patch.create` — add required snapshot fields:
```json
{
  "type": "patch.create",
  "title": "...",
  "body": "...",
  "base_ref": "main",
  "branch": "feature-foo",
  "commit": "abc123...",
  "tree": "def456..."
}
```

Modified `patch.inline_comment` — add revision field (derived number at time of event creation):
```json
{
  "type": "patch.inline_comment",
  "file": "src/lib.rs",
  "line": 42,
  "body": "...",
  "revision": 3
}
```

Modified `patch.review` — add revision field (derived number at time of event creation):
```json
{
  "type": "patch.review",
  "verdict": "approve",
  "body": "...",
  "revision": 3
}
```

Note: The `revision` field on inline comments and reviews stores the derived revision number at the time the event was created. This is a convenience for display — the authoritative revision boundaries are always the PatchCreate + PatchRevision events in topological order. Thread comments (`patch.comment`) do not get a revision field.

## Success Criteria *(mandatory)*

### Measurable Outcomes

- **SC-001**: `patch revise` correctly records revision snapshots; `patch log` shows all revisions in order.
- **SC-002**: Inline comments reference their revision number; `patch show` displays revision context for every inline comment.
- **SC-003**: `patch diff --between N M` produces the correct interdiff between any two revisions.
- **SC-004**: Comments and reviews display their revision number; interdiff between revisions is the mechanism for reviewing what changed (no cross-revision staleness tracking).
- **SC-005**: All revision data survives sync (push/fetch) between collaborators.
- **SC-007**: The TUI dashboard displays revision count and current revision number for each patch.

## Assumptions

- Revision recording is automatic on write operations only. Reads never modify the DAG. This means a revision boundary is only recorded when someone actually interacts with the patch (comments, reviews, merges), not on passive viewing.
- Tree OIDs are stored alongside commit OIDs for fast interdiff computation (avoids re-walking commit history to find the tree).
- Inline comments and reviews belong to their revision. Thread comments are patch-level. No cross-revision tracking, no line-mapping, no staleness detection. Each revision is a self-contained review surface. The interdiff is how you see what changed.
- Revision numbers are derived from topological DAG order during materialization, not stored in events. This makes concurrent auto-detection safe — duplicate PatchRevision events for the same branch change are deduplicated by commit OID.
- The existing `patch revise` command is repurposed: it now appends a `PatchRevision` event (with commit + tree OID) if the branch has changed. It serves as the explicit "I've pushed new commits, please re-review" signal.
- No backward compatibility needed — the project is pre-release with no existing patches to migrate. The old `PatchRevise` event type is removed entirely. `commit`/`tree` on `PatchCreate` and `revision` on inline comments/reviews are required fields, not optional.
- Auto-detection requires a signing key to append events. If unavailable, the write operation proceeds without inserting a PatchRevision (the comment/review is anchored to the last known revision).
- Merge policy is configurable, not hardcoded. Default is permissive (any approval suffices).

## Clarifications

### Session 2026-03-21

- Q: Should duplicate PatchRevision events (same commit OID from concurrent detection) produce one or two revisions? → A: Deduplicate by commit OID during materialization.
- Q: Should thread comments (`patch.comment`) be revision-anchored? → A: No — thread comments are patch-level conversation, not revision-scoped.
- Q: How does a reviewer target an older revision for inline comments? → A: `patch comment <id> --revision N` flag to explicitly target a revision.
- Q: Should `patch revise` allow creating a revision when branch is unchanged? → A: No — reject with "no changes since revision N".
- Q: Should `--between` require both endpoints or allow single-arg shorthand? → A: Single arg = "N to latest" shorthand; two args = explicit range.

### Session 2026-03-21 (round 2)

- Q: Should `patch review` support `--revision N` to target a specific revision? → A: Yes, symmetric with `patch comment --revision N`.
- Q: Should `patch revise` support an optional `--body` message? → A: Yes, stored in PatchRevision event alongside commit/tree. Omitted on auto-detected revisions.
- Q: Should `patch diff --revision N` show a historical full diff against the base? → A: Yes, completes the trifecta: no flags (latest vs base), `--between` (interdiff), `--revision N` (historical diff).
- Q: Where does merge policy config live? → A: `refs/collab/config` — project-level, synced between collaborators.

## Resolved Questions

1. **Gap between push and first write**: `patch revise <id>` serves as the explicit "I've pushed new commits, please re-review" signal. It appends a `PatchRevision` event if the branch has changed. If the branch hasn't changed, it rejects with "no changes since revision N". Auto-detection on write operations (`comment`, `review`, `merge`) catches cases where the developer forgets. Both paths produce the same event.
2. **Duplicate revision deduplication**: `PatchRevision` events with the same commit OID are deduplicated during materialization — only the first in topological order contributes a revision boundary.
3. **Thread comments**: `patch.comment` (thread comments) are NOT revision-anchored. They are patch-level conversation. Only inline comments and reviews get a `revision` field.