markdown · 12071 bytes Raw Blame History

Pull requests

S22 ships the PR core: open, view (Conversation / Commits / Files / Checks tabs), edit, close/reopen, draft → ready, three merge strategies (merge / squash / rebase), and auto-synchronize on push.

Schema (migration 0023)

pull_requests          — keyed on issue_id (FK PK to issues)
pull_request_commits   — head-side commit list, refreshed on synchronize
pull_request_files     — changed-files list, refreshed on synchronize

Plus four columns on repos that S11 left to S22:

Column Default Notes
allow_squash_merge true UI hides the option when off
allow_rebase_merge true UI hides the option when off
allow_merge_commit true UI hides the option when off
default_merge_method 'merge' preselected in the merge form

A CHECK constraint enforces at least one method enabled so the UI can never end up unable to merge.

A PR's title / body / state / comments / labels / milestones / assignees live on the issues row (kind = 'pr'); the PR-only fields (base/head refs, draft, mergeable_state, merge metadata) live on pull_requests. Single number space, single comments table — matches the S21 design.

Same-repo only (v1)

head_repo_id is on the table from day one but always equals repo_id for v1. A CHECK constraint ensures base_ref <> head_ref so a self-merge can't be opened. Cross-fork PRs land in S27.

Routes

Route Auth
GET /{owner}/{repo}/pulls?state=open|closed public
GET /{owner}/{repo}/pulls/{number} public
GET /{owner}/{repo}/pulls/{number}/files public
GET /{owner}/{repo}/pulls/{number}/commits public
GET /{owner}/{repo}/pulls/{number}/checks public
GET /{owner}/{repo}/pulls/new?base=…&head=… RequireUser
POST /{owner}/{repo}/pulls RequireUser
POST /{owner}/{repo}/pulls/{number}/edit RequireUser
POST /{owner}/{repo}/pulls/{number}/state RequireUser
POST /{owner}/{repo}/pulls/{number}/ready RequireUser
POST /{owner}/{repo}/pulls/{number}/merge RequireUser

The pull-request list's "New pull request" button starts at /{owner}/{repo}/compare, where the user picks base/head refs. Once the head is ahead of base, compare links into /pulls/new?base=...&head=.... /pulls/new redirects back to compare when no head ref is supplied so the GitHub-style branch picker remains the canonical entry point.

Auto-synchronize on head push

internal/worker/jobs/push_process.go is extended: after a push to refs/heads/<X>, list every open PR with head_repo_id = repo_id AND head_ref = X, and enqueue pr:synchronize for each one. The synchronize job:

  1. Resolves base + head OIDs via repogit.ResolveRefOID.
  2. Refreshes pull_request_commits + pull_request_files.
  3. Resets mergeable_state to unknown.
  4. Emits a synchronized timeline event with the new head OID.
  5. Chains a pr:mergeability job.

Failure-modes are best-effort — push:process logs but doesn't block the push pipeline if PR enumeration / enqueue fails.

Mergeability detection (pr:mergeability)

Implemented via `git merge-tree --write-tree --no-messages

` (Git ≥ 2.38). Git auto-computes the merge base.
Exit Meaning mergeable_state
0 Clean merge (TreeOID printed) clean
1 Conflicts (first line is tree OID, rest are paths) dirty
≥ 2 Real error — wrapped + retried via worker backoff unchanged

A separate cheap check runs first: if git log base..head is empty the head has no commits ahead and we set behind without invoking merge-tree.

The earlier draft of this code passed --merge-base=<base> to merge-tree, which sets the common ancestor to base itself and therefore can never produce a conflict. That was wrong — fixed via TestMergeability_Dirty.

Merge operation

internal/repos/git/mergeops.go::PerformMerge runs entirely server-side in a temp worktree on the same volume as the bare repo (S04 mandates /data/repos/ and tempdirs share the volume). The worktree is created via git worktree add --detach <wt> <base_oid> so the bare repo's branch refs aren't polluted; only the final update-ref writes back.

Strategies:

  • mergegit merge --no-ff --no-edit -m <subject> against the worktree, then update-ref.
  • squashgit merge --squash (stages without committing) then git commit -m <subject> with author = PR author, committer = merger.
  • rebasegit rebase --onto <base_oid> <base_oid> <head_oid>. Each PR commit's author is preserved; committer becomes the merger.

The orchestrator wraps the merge in a tx that holds SELECT … FOR UPDATE on pull_requests for the whole job. That row lock serializes concurrent merges — TestMerge_RejectsConcurrentDouble fans two goroutines and asserts exactly one wins.

update-ref <ref> <new> <old> is gated on the expected <old> OID so a concurrent push to base between the merge-tree probe and the update-ref also fails cleanly.

Linked-issue auto-close

Closes/Fixes/Resolves #N (case-insensitive, all tenses) parsed from:

  • The PR body.
  • Each head-side commit's subject + body, fetched from pull_request_commits.

On merge, every linked still-open issue in the same repo flips to closed, state_reason = 'completed', and gets a closed event citing the PR id. Self-references skip silently. The regex lives at internal/pulls/linked_issues.go::reLinkedIssue.

Identity propagation

Per the spec:

Strategy Author Committer
merge merger merger
squash PR author merger
rebase original commit author merger

Email = the user's primary verified email. When unset (rare for v1 since repo create requires a verified primary), we synthesize <username>@noreply.shithub.local rather than fail the merge. Real noreply emails are post-MVP.

Web UI

  • Compare/new-PR entry follows GitHub's range editor: base and head dropdowns list branches and tags with live filtering, preserve the opposite side of the comparison, and render compare URLs with the base...head shape.
  • The open-PR page reuses the compare state: ahead/behind counts, mergeability probe, commits, and three-dot diff all render before submission. The form posts the selected refs as hidden fields.
  • The new PR description uses the shared GitHub-like Markdown editor (write/preview, toolbar, mentions/references/saved replies shell). Copilot suggestions are intentionally omitted.
  • Tabbed view at /pulls/{number} switches between Conversation, Commits, Files, Checks via the Tab field on the template data.
  • Conversation follows GitHub's PageHeader + tab strip shape: state pill, branch summary, tab count badges, left timeline, right metadata rail, and a mergeability box below the timeline.
  • Files tab uses the existing S19 diff renderer fed from compareSourceMergeBase (three-dot diff, base...head).
  • The Checks tab is a placeholder — real check runs land in S24.
  • The merge form hides disallowed methods and preselects the repo's default_merge_method.

Errors

Error UI message
ErrSameBranch "Base and head must differ."
ErrBaseNotFound "Base branch not found." / "base branch no longer exists" on reopen
ErrHeadNotFound "Head branch not found." / "head branch no longer exists" on reopen
ErrNoCommitsToMerge "Head has no commits ahead of base."
ErrAlreadyMerged "already merged"
ErrAlreadyClosed "already closed"
ErrMergeBlocked "merge blocked — branch has conflicts or hasn't been checked yet"
ErrMergeMethodOff "this merge method is disabled on this repo"
ErrConcurrentMerge "PR is being merged by another request"

Tests

File Covers
internal/pulls/linked_issues_test.go Closes/Fixes/Resolves regex, dedup, prose-vs-keyword
internal/pulls/pulls_test.go::Create… Issue/PR row pair + initial commits/files snapshot
internal/pulls/pulls_test.go::Mergeability_Clean Probe sets clean on simple fast-forward
internal/pulls/pulls_test.go::Mergeability_Dirty Probe sets dirty on real shared-file conflict
internal/pulls/pulls_test.go::Merge_MergeCommit Worktree merge writes commit, PR closes with merged_at
internal/pulls/pulls_test.go::Merge_RejectsConcurrentDouble Row lock + update-ref serialize merges
internal/pulls/pulls_test.go::LinkedIssueAutoClose Fixes #N in body closes #N on merge

Pitfalls handled

  • merge-tree --merge-base=<base> was wrong in the early draft; it sets the ancestor to base and reports clean for any input. Drop the flag and let git auto-compute.
  • Non-fast-forward merge commit--no-ff so we always get a merge commit, even when head is strictly ahead of base.
  • Worktree leak on panicdefer cleanup() in PerformMerge runs git worktree remove --force + os.RemoveAll. A periodic janitor for <gitDir>/.tmp-worktrees/* older than 1h is an S37 follow-up.
  • Squash author mismatch — author = PR author (looked up via users.id); committer = merger. Documented in identityFor().
  • Reopen with deleted branchespulls.SetState(open) validates base + head still resolve; otherwise returns ErrBaseNotFound / ErrHeadNotFound for a friendly handler message.
  • Stale mergeability — synchronize resets to unknown so a concurrent merge attempt sees the "checking…" banner instead of acting on a stale clean.
  • Pre-receive bypass on merge — the internal update-ref skips the hook by design. Branch protection is checked at the merge policy gate (policy.ActionPullMerge) instead of via push.

Deferred

  • Cross-fork PRs → S27 (forks). The head_repo_id column + fan-out path are in place; resolver + UI to follow.
  • File-level review threads / approve / request-changes → S23.
  • Real CI / required-checks gating → S24. The Checks tab is the visual scaffold.
  • Auto-merge / merge-queue → post-MVP.
  • Inline conflict resolver UI → post-MVP. v1 surfaces a banner with manual-resolve instructions.
  • Auto-delete head branch on merge → S32 (repo settings) once the toggle UI lands. Spec leans opt-in checkbox.
  • @team mentions in PR body → S31.
  • Worktree janitor → S37.
  • Author-noreply emails → post-MVP.
  • N+1 on PR list (author, label fan-out) → S36.
View source
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 pull-request list's "New pull request" button starts at
56 `/{owner}/{repo}/compare`, where the user picks base/head refs. Once
57 the head is ahead of base, compare links into
58 `/pulls/new?base=...&head=...`. `/pulls/new` redirects back to
59 compare when no head ref is supplied so the GitHub-style branch picker
60 remains the canonical entry point.
61
62 ## Auto-synchronize on head push
63
64 `internal/worker/jobs/push_process.go` is extended: after a push to
65 `refs/heads/<X>`, list every open PR with `head_repo_id = repo_id`
66 AND `head_ref = X`, and enqueue `pr:synchronize` for each one. The
67 synchronize job:
68
69 1. Resolves base + head OIDs via `repogit.ResolveRefOID`.
70 2. Refreshes `pull_request_commits` + `pull_request_files`.
71 3. Resets `mergeable_state` to `unknown`.
72 4. Emits a `synchronized` timeline event with the new head OID.
73 5. Chains a `pr:mergeability` job.
74
75 Failure-modes are best-effort — push:process logs but doesn't block
76 the push pipeline if PR enumeration / enqueue fails.
77
78 ## Mergeability detection (`pr:mergeability`)
79
80 Implemented via `git merge-tree --write-tree --no-messages <base>
81 <head>` (Git ≥ 2.38). Git auto-computes the merge base.
82
83 | Exit | Meaning | mergeable_state |
84 | ---- | ----------------------------------------------------- | --------------- |
85 | 0 | Clean merge (TreeOID printed) | `clean` |
86 | 1 | Conflicts (first line is tree OID, rest are paths) | `dirty` |
87 | ≥ 2 | Real error — wrapped + retried via worker backoff | unchanged |
88
89 A separate cheap check runs first: if `git log base..head` is empty
90 the head has no commits ahead and we set `behind` without invoking
91 merge-tree.
92
93 The earlier draft of this code passed `--merge-base=<base>` to
94 `merge-tree`, which sets the common ancestor *to base itself* and
95 therefore can never produce a conflict. That was wrong — fixed via
96 `TestMergeability_Dirty`.
97
98 ## Merge operation
99
100 `internal/repos/git/mergeops.go::PerformMerge` runs entirely
101 server-side in a temp worktree on the same volume as the bare repo
102 (S04 mandates `/data/repos/` and tempdirs share the volume). The
103 worktree is created via `git worktree add --detach <wt> <base_oid>`
104 so the bare repo's branch refs aren't polluted; only the final
105 `update-ref` writes back.
106
107 Strategies:
108
109 - **merge** — `git merge --no-ff --no-edit -m <subject>` against the
110 worktree, then update-ref.
111 - **squash** — `git merge --squash` (stages without committing) then
112 `git commit -m <subject>` with author = PR author, committer = merger.
113 - **rebase** — `git rebase --onto <base_oid> <base_oid> <head_oid>`.
114 Each PR commit's author is preserved; committer becomes the merger.
115
116 The orchestrator wraps the merge in a tx that holds `SELECT … FOR
117 UPDATE` on `pull_requests` for the whole job. That row lock
118 serializes concurrent merges — `TestMerge_RejectsConcurrentDouble`
119 fans two goroutines and asserts exactly one wins.
120
121 `update-ref <ref> <new> <old>` is gated on the expected `<old>` OID
122 so a concurrent push to base between the merge-tree probe and the
123 update-ref also fails cleanly.
124
125 ## Linked-issue auto-close
126
127 `Closes/Fixes/Resolves #N` (case-insensitive, all tenses) parsed from:
128
129 - The PR body.
130 - Each head-side commit's subject + body, fetched from
131 `pull_request_commits`.
132
133 On merge, every linked still-open issue in the same repo flips to
134 `closed`, `state_reason = 'completed'`, and gets a `closed` event
135 citing the PR id. Self-references skip silently. The regex lives at
136 `internal/pulls/linked_issues.go::reLinkedIssue`.
137
138 ## Identity propagation
139
140 Per the spec:
141
142 | Strategy | Author | Committer |
143 | -------- | --------------------- | ----------- |
144 | merge | merger | merger |
145 | squash | PR author | merger |
146 | rebase | original commit author | merger |
147
148 Email = the user's primary verified email. When unset (rare for v1
149 since repo create requires a verified primary), we synthesize
150 `<username>@noreply.shithub.local` rather than fail the merge. Real
151 noreply emails are post-MVP.
152
153 ## Web UI
154
155 - Compare/new-PR entry follows GitHub's range editor: base and head
156 dropdowns list branches and tags with live filtering, preserve the
157 opposite side of the comparison, and render compare URLs with the
158 `base...head` shape.
159 - The open-PR page reuses the compare state: ahead/behind counts,
160 mergeability probe, commits, and three-dot diff all render before
161 submission. The form posts the selected refs as hidden fields.
162 - The new PR description uses the shared GitHub-like Markdown editor
163 (write/preview, toolbar, mentions/references/saved replies shell).
164 Copilot suggestions are intentionally omitted.
165 - Tabbed view at `/pulls/{number}` switches between Conversation,
166 Commits, Files, Checks via the `Tab` field on the template data.
167 - Conversation follows GitHub's PageHeader + tab strip shape: state
168 pill, branch summary, tab count badges, left timeline, right metadata
169 rail, and a mergeability box below the timeline.
170 - Files tab uses the existing S19 diff renderer fed from
171 `compareSourceMergeBase` (three-dot diff, base...head).
172 - The Checks tab is a placeholder — real check runs land in S24.
173 - The merge form hides disallowed methods and preselects the repo's
174 `default_merge_method`.
175
176 ## Errors
177
178 | Error | UI message |
179 | ---------------------- | --------------------------------------------------------------------- |
180 | `ErrSameBranch` | "Base and head must differ." |
181 | `ErrBaseNotFound` | "Base branch not found." / "base branch no longer exists" on reopen |
182 | `ErrHeadNotFound` | "Head branch not found." / "head branch no longer exists" on reopen |
183 | `ErrNoCommitsToMerge` | "Head has no commits ahead of base." |
184 | `ErrAlreadyMerged` | "already merged" |
185 | `ErrAlreadyClosed` | "already closed" |
186 | `ErrMergeBlocked` | "merge blocked — branch has conflicts or hasn't been checked yet" |
187 | `ErrMergeMethodOff` | "this merge method is disabled on this repo" |
188 | `ErrConcurrentMerge` | "PR is being merged by another request" |
189
190 ## Tests
191
192 | File | Covers |
193 | --------------------------------------------- | ------------------------------------------------------------ |
194 | `internal/pulls/linked_issues_test.go` | Closes/Fixes/Resolves regex, dedup, prose-vs-keyword |
195 | `internal/pulls/pulls_test.go::Create…` | Issue/PR row pair + initial commits/files snapshot |
196 | `internal/pulls/pulls_test.go::Mergeability_Clean` | Probe sets `clean` on simple fast-forward |
197 | `internal/pulls/pulls_test.go::Mergeability_Dirty` | Probe sets `dirty` on real shared-file conflict |
198 | `internal/pulls/pulls_test.go::Merge_MergeCommit` | Worktree merge writes commit, PR closes with merged_at |
199 | `internal/pulls/pulls_test.go::Merge_RejectsConcurrentDouble` | Row lock + update-ref serialize merges |
200 | `internal/pulls/pulls_test.go::LinkedIssueAutoClose` | `Fixes #N` in body closes #N on merge |
201
202 ## Pitfalls handled
203
204 - **`merge-tree --merge-base=<base>` was wrong** in the early draft;
205 it sets the ancestor *to base* and reports clean for any input.
206 Drop the flag and let git auto-compute.
207 - **Non-fast-forward merge commit** — `--no-ff` so we always get a
208 merge commit, even when head is strictly ahead of base.
209 - **Worktree leak on panic** — `defer cleanup()` in `PerformMerge`
210 runs `git worktree remove --force` + `os.RemoveAll`. A periodic
211 janitor for `<gitDir>/.tmp-worktrees/*` older than 1h is an S37
212 follow-up.
213 - **Squash author mismatch** — author = PR author (looked up via
214 `users.id`); committer = merger. Documented in identityFor().
215 - **Reopen with deleted branches** — `pulls.SetState(open)` validates
216 base + head still resolve; otherwise returns `ErrBaseNotFound` /
217 `ErrHeadNotFound` for a friendly handler message.
218 - **Stale mergeability** — synchronize resets to `unknown` so a
219 concurrent merge attempt sees the "checking…" banner instead of
220 acting on a stale `clean`.
221 - **Pre-receive bypass on merge** — the internal `update-ref` skips
222 the hook by design. Branch protection is checked at the merge
223 policy gate (`policy.ActionPullMerge`) instead of via push.
224
225 ## Deferred
226
227 - **Cross-fork PRs** → S27 (forks). The `head_repo_id` column +
228 fan-out path are in place; resolver + UI to follow.
229 - **File-level review threads / approve / request-changes** → S23.
230 - **Real CI / required-checks gating** → S24. The Checks tab is the
231 visual scaffold.
232 - **Auto-merge / merge-queue** → post-MVP.
233 - **Inline conflict resolver UI** → post-MVP. v1 surfaces a banner
234 with manual-resolve instructions.
235 - **Auto-delete head branch on merge** → S32 (repo settings) once
236 the toggle UI lands. Spec leans opt-in checkbox.
237 - **`@team` mentions in PR body** → S31.
238 - **Worktree janitor** → S37.
239 - **Author-noreply emails** → post-MVP.
240 - **N+1 on PR list (author, label fan-out)** → S36.