@@ -0,0 +1,223 @@ |
| 1 | +# Pull requests |
| 2 | + |
| 3 | +S22 ships the PR core: open, view (Conversation / Commits / Files / |
| 4 | +Checks tabs), edit, close/reopen, draft → ready, three merge |
| 5 | +strategies (merge / squash / rebase), and auto-synchronize on push. |
| 6 | + |
| 7 | +## Schema (migration 0023) |
| 8 | + |
| 9 | +``` |
| 10 | +pull_requests — keyed on issue_id (FK PK to issues) |
| 11 | +pull_request_commits — head-side commit list, refreshed on synchronize |
| 12 | +pull_request_files — changed-files list, refreshed on synchronize |
| 13 | +``` |
| 14 | + |
| 15 | +Plus four columns on `repos` that S11 left to S22: |
| 16 | + |
| 17 | +| Column | Default | Notes | |
| 18 | +| ---------------------- | --------- | -------------------------------------------------- | |
| 19 | +| `allow_squash_merge` | `true` | UI hides the option when off | |
| 20 | +| `allow_rebase_merge` | `true` | UI hides the option when off | |
| 21 | +| `allow_merge_commit` | `true` | UI hides the option when off | |
| 22 | +| `default_merge_method` | `'merge'` | preselected in the merge form | |
| 23 | + |
| 24 | +A CHECK constraint enforces at least one method enabled so the UI |
| 25 | +can never end up unable to merge. |
| 26 | + |
| 27 | +A PR's title / body / state / comments / labels / milestones / |
| 28 | +assignees live on the **issues** row (`kind = 'pr'`); the PR-only |
| 29 | +fields (base/head refs, draft, mergeable_state, merge metadata) live |
| 30 | +on `pull_requests`. Single number space, single comments table — |
| 31 | +matches the S21 design. |
| 32 | + |
| 33 | +## Same-repo only (v1) |
| 34 | + |
| 35 | +`head_repo_id` is on the table from day one but always equals |
| 36 | +`repo_id` for v1. A CHECK constraint ensures `base_ref <> head_ref` |
| 37 | +so a self-merge can't be opened. Cross-fork PRs land in S27. |
| 38 | + |
| 39 | +## Routes |
| 40 | + |
| 41 | +| Route | Auth | |
| 42 | +| ----------------------------------------------------- | ------------- | |
| 43 | +| `GET /{owner}/{repo}/pulls?state=open\|closed` | public | |
| 44 | +| `GET /{owner}/{repo}/pulls/{number}` | public | |
| 45 | +| `GET /{owner}/{repo}/pulls/{number}/files` | public | |
| 46 | +| `GET /{owner}/{repo}/pulls/{number}/commits` | public | |
| 47 | +| `GET /{owner}/{repo}/pulls/{number}/checks` | public | |
| 48 | +| `GET /{owner}/{repo}/pulls/new?base=…&head=…` | RequireUser | |
| 49 | +| `POST /{owner}/{repo}/pulls` | RequireUser | |
| 50 | +| `POST /{owner}/{repo}/pulls/{number}/edit` | RequireUser | |
| 51 | +| `POST /{owner}/{repo}/pulls/{number}/state` | RequireUser | |
| 52 | +| `POST /{owner}/{repo}/pulls/{number}/ready` | RequireUser | |
| 53 | +| `POST /{owner}/{repo}/pulls/{number}/merge` | RequireUser | |
| 54 | + |
| 55 | +The compare view (S20) links into `/pulls/new?base=...&head=...` so |
| 56 | +the entry point matches GitHub's flow. |
| 57 | + |
| 58 | +## Auto-synchronize on head push |
| 59 | + |
| 60 | +`internal/worker/jobs/push_process.go` is extended: after a push to |
| 61 | +`refs/heads/<X>`, list every open PR with `head_repo_id = repo_id` |
| 62 | +AND `head_ref = X`, and enqueue `pr:synchronize` for each one. The |
| 63 | +synchronize job: |
| 64 | + |
| 65 | +1. Resolves base + head OIDs via `repogit.ResolveRefOID`. |
| 66 | +2. Refreshes `pull_request_commits` + `pull_request_files`. |
| 67 | +3. Resets `mergeable_state` to `unknown`. |
| 68 | +4. Emits a `synchronized` timeline event with the new head OID. |
| 69 | +5. Chains a `pr:mergeability` job. |
| 70 | + |
| 71 | +Failure-modes are best-effort — push:process logs but doesn't block |
| 72 | +the push pipeline if PR enumeration / enqueue fails. |
| 73 | + |
| 74 | +## Mergeability detection (`pr:mergeability`) |
| 75 | + |
| 76 | +Implemented via `git merge-tree --write-tree --no-messages <base> |
| 77 | +<head>` (Git ≥ 2.38). Git auto-computes the merge base. |
| 78 | + |
| 79 | +| Exit | Meaning | mergeable_state | |
| 80 | +| ---- | ----------------------------------------------------- | --------------- | |
| 81 | +| 0 | Clean merge (TreeOID printed) | `clean` | |
| 82 | +| 1 | Conflicts (first line is tree OID, rest are paths) | `dirty` | |
| 83 | +| ≥ 2 | Real error — wrapped + retried via worker backoff | unchanged | |
| 84 | + |
| 85 | +A separate cheap check runs first: if `git log base..head` is empty |
| 86 | +the head has no commits ahead and we set `behind` without invoking |
| 87 | +merge-tree. |
| 88 | + |
| 89 | +The earlier draft of this code passed `--merge-base=<base>` to |
| 90 | +`merge-tree`, which sets the common ancestor *to base itself* and |
| 91 | +therefore can never produce a conflict. That was wrong — fixed via |
| 92 | +`TestMergeability_Dirty`. |
| 93 | + |
| 94 | +## Merge operation |
| 95 | + |
| 96 | +`internal/repos/git/mergeops.go::PerformMerge` runs entirely |
| 97 | +server-side in a temp worktree on the same volume as the bare repo |
| 98 | +(S04 mandates `/data/repos/` and tempdirs share the volume). The |
| 99 | +worktree is created via `git worktree add --detach <wt> <base_oid>` |
| 100 | +so the bare repo's branch refs aren't polluted; only the final |
| 101 | +`update-ref` writes back. |
| 102 | + |
| 103 | +Strategies: |
| 104 | + |
| 105 | +- **merge** — `git merge --no-ff --no-edit -m <subject>` against the |
| 106 | + worktree, then update-ref. |
| 107 | +- **squash** — `git merge --squash` (stages without committing) then |
| 108 | + `git commit -m <subject>` with author = PR author, committer = merger. |
| 109 | +- **rebase** — `git rebase --onto <base_oid> <base_oid> <head_oid>`. |
| 110 | + Each PR commit's author is preserved; committer becomes the merger. |
| 111 | + |
| 112 | +The orchestrator wraps the merge in a tx that holds `SELECT … FOR |
| 113 | +UPDATE` on `pull_requests` for the whole job. That row lock |
| 114 | +serializes concurrent merges — `TestMerge_RejectsConcurrentDouble` |
| 115 | +fans two goroutines and asserts exactly one wins. |
| 116 | + |
| 117 | +`update-ref <ref> <new> <old>` is gated on the expected `<old>` OID |
| 118 | +so a concurrent push to base between the merge-tree probe and the |
| 119 | +update-ref also fails cleanly. |
| 120 | + |
| 121 | +## Linked-issue auto-close |
| 122 | + |
| 123 | +`Closes/Fixes/Resolves #N` (case-insensitive, all tenses) parsed from: |
| 124 | + |
| 125 | +- The PR body. |
| 126 | +- Each head-side commit's subject + body, fetched from |
| 127 | + `pull_request_commits`. |
| 128 | + |
| 129 | +On merge, every linked still-open issue in the same repo flips to |
| 130 | +`closed`, `state_reason = 'completed'`, and gets a `closed` event |
| 131 | +citing the PR id. Self-references skip silently. The regex lives at |
| 132 | +`internal/pulls/linked_issues.go::reLinkedIssue`. |
| 133 | + |
| 134 | +## Identity propagation |
| 135 | + |
| 136 | +Per the spec: |
| 137 | + |
| 138 | +| Strategy | Author | Committer | |
| 139 | +| -------- | --------------------- | ----------- | |
| 140 | +| merge | merger | merger | |
| 141 | +| squash | PR author | merger | |
| 142 | +| rebase | original commit author | merger | |
| 143 | + |
| 144 | +Email = the user's primary verified email. When unset (rare for v1 |
| 145 | +since repo create requires a verified primary), we synthesize |
| 146 | +`<username>@noreply.shithub.local` rather than fail the merge. Real |
| 147 | +noreply emails are post-MVP. |
| 148 | + |
| 149 | +## Web UI |
| 150 | + |
| 151 | +- Tabbed view at `/pulls/{number}` switches between Conversation, |
| 152 | + Commits, Files, Checks via the `Tab` field on the template data. |
| 153 | +- Files tab uses the existing S19 diff renderer fed from |
| 154 | + `compareSourceMergeBase` (three-dot diff, base...head). |
| 155 | +- The Checks tab is a placeholder — real check runs land in S24. |
| 156 | +- The merge form hides disallowed methods and preselects the repo's |
| 157 | + `default_merge_method`. |
| 158 | + |
| 159 | +## Errors |
| 160 | + |
| 161 | +| Error | UI message | |
| 162 | +| ---------------------- | --------------------------------------------------------------------- | |
| 163 | +| `ErrSameBranch` | "Base and head must differ." | |
| 164 | +| `ErrBaseNotFound` | "Base branch not found." / "base branch no longer exists" on reopen | |
| 165 | +| `ErrHeadNotFound` | "Head branch not found." / "head branch no longer exists" on reopen | |
| 166 | +| `ErrNoCommitsToMerge` | "Head has no commits ahead of base." | |
| 167 | +| `ErrAlreadyMerged` | "already merged" | |
| 168 | +| `ErrAlreadyClosed` | "already closed" | |
| 169 | +| `ErrMergeBlocked` | "merge blocked — branch has conflicts or hasn't been checked yet" | |
| 170 | +| `ErrMergeMethodOff` | "this merge method is disabled on this repo" | |
| 171 | +| `ErrConcurrentMerge` | "PR is being merged by another request" | |
| 172 | + |
| 173 | +## Tests |
| 174 | + |
| 175 | +| File | Covers | |
| 176 | +| --------------------------------------------- | ------------------------------------------------------------ | |
| 177 | +| `internal/pulls/linked_issues_test.go` | Closes/Fixes/Resolves regex, dedup, prose-vs-keyword | |
| 178 | +| `internal/pulls/pulls_test.go::Create…` | Issue/PR row pair + initial commits/files snapshot | |
| 179 | +| `internal/pulls/pulls_test.go::Mergeability_Clean` | Probe sets `clean` on simple fast-forward | |
| 180 | +| `internal/pulls/pulls_test.go::Mergeability_Dirty` | Probe sets `dirty` on real shared-file conflict | |
| 181 | +| `internal/pulls/pulls_test.go::Merge_MergeCommit` | Worktree merge writes commit, PR closes with merged_at | |
| 182 | +| `internal/pulls/pulls_test.go::Merge_RejectsConcurrentDouble` | Row lock + update-ref serialize merges | |
| 183 | +| `internal/pulls/pulls_test.go::LinkedIssueAutoClose` | `Fixes #N` in body closes #N on merge | |
| 184 | + |
| 185 | +## Pitfalls handled |
| 186 | + |
| 187 | +- **`merge-tree --merge-base=<base>` was wrong** in the early draft; |
| 188 | + it sets the ancestor *to base* and reports clean for any input. |
| 189 | + Drop the flag and let git auto-compute. |
| 190 | +- **Non-fast-forward merge commit** — `--no-ff` so we always get a |
| 191 | + merge commit, even when head is strictly ahead of base. |
| 192 | +- **Worktree leak on panic** — `defer cleanup()` in `PerformMerge` |
| 193 | + runs `git worktree remove --force` + `os.RemoveAll`. A periodic |
| 194 | + janitor for `<gitDir>/.tmp-worktrees/*` older than 1h is an S37 |
| 195 | + follow-up. |
| 196 | +- **Squash author mismatch** — author = PR author (looked up via |
| 197 | + `users.id`); committer = merger. Documented in identityFor(). |
| 198 | +- **Reopen with deleted branches** — `pulls.SetState(open)` validates |
| 199 | + base + head still resolve; otherwise returns `ErrBaseNotFound` / |
| 200 | + `ErrHeadNotFound` for a friendly handler message. |
| 201 | +- **Stale mergeability** — synchronize resets to `unknown` so a |
| 202 | + concurrent merge attempt sees the "checking…" banner instead of |
| 203 | + acting on a stale `clean`. |
| 204 | +- **Pre-receive bypass on merge** — the internal `update-ref` skips |
| 205 | + the hook by design. Branch protection is checked at the merge |
| 206 | + policy gate (`policy.ActionPullMerge`) instead of via push. |
| 207 | + |
| 208 | +## Deferred |
| 209 | + |
| 210 | +- **Cross-fork PRs** → S27 (forks). The `head_repo_id` column + |
| 211 | + fan-out path are in place; resolver + UI to follow. |
| 212 | +- **File-level review threads / approve / request-changes** → S23. |
| 213 | +- **Real CI / required-checks gating** → S24. The Checks tab is the |
| 214 | + visual scaffold. |
| 215 | +- **Auto-merge / merge-queue** → post-MVP. |
| 216 | +- **Inline conflict resolver UI** → post-MVP. v1 surfaces a banner |
| 217 | + with manual-resolve instructions. |
| 218 | +- **Auto-delete head branch on merge** → S32 (repo settings) once |
| 219 | + the toggle UI lands. Spec leans opt-in checkbox. |
| 220 | +- **`@team` mentions in PR body** → S31. |
| 221 | +- **Worktree janitor** → S37. |
| 222 | +- **Author-noreply emails** → post-MVP. |
| 223 | +- **N+1 on PR list (author, label fan-out)** → S36. |