markdown · 11029 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 compare view (S20) links into /pulls/new?base=...&head=... so the entry point matches GitHub's flow.

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

  • Tabbed view at /pulls/{number} switches between Conversation, Commits, Files, Checks via the Tab field on the template data.
  • 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 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.