Tahinli/forge
public0 watching
★ 0
| 1 | # Milestone 4a: forks and pull requests (contract between workers) |
| 2 | |
| 3 | Reviews with line comments come in M4b, not here. The global rules from `docs/m1-interfaces.md` and `docs/m3-interfaces.md` apply: |
| 4 | - The git CLI only gets **full hex OIDs** and validated ref names, placed after `--end-of-options` where git supports it. |
| 5 | - No shell, no unwrap in non-test code. Clippy `-D warnings` and fmt must pass. |
| 6 | - No inline styles and no JS. |
| 7 | - Every page goes through `load_repo` and `authorize`, and times use `viewer_tz`. |
| 8 | - **Never put cargo target dirs or big files in /tmp**, and tests use `tempfile::TempDir`. |
| 9 | |
| 10 | ## Concepts |
| 11 | - A PR is an `issues` row with `kind = 'pull'`, so it shares the per-repo numbering with issues. That gives it comments, labels, assignees, milestones, events and edit history for free. Issue pages already filter `kind = 'issue'`; PR pages filter `kind = 'pull'`. `#N` links go to `/issues/N`, and the issue page redirects (303) to `/pulls/N` when N is a PR. |
| 12 | - The **head** is `(head_repo_id, head_branch)`, either the same repo or a fork of the base. The **base** is `(base repo = issue.repo_id, base_branch)`. |
| 13 | - The base repo keeps `refs/pull/<N>/head` pointing at the head commit. It's updated whenever the head branch moves, so the base repo always has the objects. For a fork, that means a local `git fetch` from the fork's path. |
| 14 | |
| 15 | ## Forks |
| 16 | - `repos` gains `fork_of TEXT NULL REFERENCES repos(id)` (migration 0007). |
| 17 | - Creating a fork: `POST /{o}/{r}/fork` (signed in, Read on the source, CSRF). The new repo is named like the source (a `-1`, `-2` suffix on conflict), owned by the viewer. Its visibility matches the source; a private source forks to private. |
| 18 | - **On disk:** `git clone --bare --local --no-checkout <src_path> <new_path>` via GitCommand. `--local` hardlinks objects, which is cheap and safe because objects are immutable. Copy only branches and tags (`refs/heads/*`, `refs/tags/*`), not `refs/pull/*`. Then apply the managed config the same way `init_bare` does (hooks path and so on). Use alternates? **No**: deleting or trashing the parent would break the fork. |
| 19 | - The fork page header shows "forked from owner/name", linked to the source if the viewer can see it. |
| 20 | - Deleting a parent repo leaves forks working, with `fork_of` set to NULL in the delete transaction. |
| 21 | |
| 22 | ## Schema (migration `0007_pulls.sql`) |
| 23 | ```sql |
| 24 | ALTER TABLE repos ADD COLUMN fork_of TEXT REFERENCES repos(id); |
| 25 | CREATE TABLE pull_requests ( |
| 26 | issue_id TEXT PRIMARY KEY REFERENCES issues(id) ON DELETE CASCADE, |
| 27 | base_branch TEXT NOT NULL, |
| 28 | head_repo_id TEXT REFERENCES repos(id), -- NULL if the head repo was deleted |
| 29 | head_branch TEXT NOT NULL, |
| 30 | head_oid TEXT NOT NULL, -- last known head commit (also refs/pull/N/head) |
| 31 | base_oid TEXT NOT NULL, -- base branch tip when mergeability was last computed |
| 32 | merge_base TEXT, |
| 33 | mergeable TEXT NOT NULL DEFAULT 'unknown', -- 'unknown' | 'clean' | 'conflict' | 'merged' | 'up_to_date' |
| 34 | conflict_files TEXT NOT NULL DEFAULT '[]', -- JSON array of paths |
| 35 | merged_at TEXT, merged_by TEXT REFERENCES users(id), merge_commit TEXT, |
| 36 | allow_maintainer_edits INTEGER NOT NULL DEFAULT 1); |
| 37 | CREATE INDEX pull_requests_head ON pull_requests(head_repo_id, head_branch); |
| 38 | ``` |
| 39 | `Store::delete_repo` (base) must delete pull_requests rows of the repo's issues. Deleting a **head** repo sets `head_repo_id = NULL`; such a PR can't be merged any more, but its stored refs and diff still show. |
| 40 | |
| 41 | ## forge-core (`src/pulls.rs` model plus `store/pulls.rs`, an `impl Store` block) |
| 42 | ```rust |
| 43 | pub enum Mergeable { Unknown, Clean, Conflict, Merged, UpToDate } // as_str/parse |
| 44 | pub struct PullRequest { pub issue: Issue, pub base_branch: String, pub head_repo_id: Option<Id>, |
| 45 | pub head_branch: String, pub head_oid: String, pub base_oid: String, pub merge_base: Option<String>, |
| 46 | pub mergeable: Mergeable, pub conflict_files: Vec<String>, pub merged_at: Option<String>, |
| 47 | pub merged_by: Option<Id>, pub merge_commit: Option<String> } |
| 48 | pub enum MergeStyle { Merge, Squash, FastForward } // "merge" | "squash" | "ff" |
| 49 | |
| 50 | // store/pulls.rs |
| 51 | pub async fn create_pull(&self, repo_id: &str, author_id: &str, title: &str, body: &str, |
| 52 | base_branch: &str, head_repo_id: &str, head_branch: &str, head_oid: &str, base_oid: &str) -> Result<PullRequest>; // allocates a number like create_issue (shared counter), kind='pull' |
| 53 | pub async fn pull_by_number(&self, repo_id: &str, number: i64) -> Result<Option<PullRequest>>; |
| 54 | pub async fn list_pulls(&self, repo_id: &str, f: &IssueFilter) -> Result<(Vec<IssueListItem>, u64)>; // same filters as issues; state 'open'|'closed' (merged PRs are closed) |
| 55 | pub async fn pull_counts(&self, repo_id: &str) -> Result<(u64, u64)>; |
| 56 | pub async fn open_pulls_for_head(&self, head_repo_id: &str, head_branch: &str) -> Result<Vec<(Id /*base repo*/, i64 /*number*/, Id /*issue id*/)>>; |
| 57 | pub async fn open_pulls_for_base(&self, repo_id: &str, base_branch: &str) -> Result<Vec<(i64, Id)>>; |
| 58 | pub async fn update_pull_head(&self, issue_id: &str, head_oid: &str) -> Result<()>; // resets mergeable to 'unknown' |
| 59 | pub async fn set_mergeability(&self, issue_id: &str, base_oid: &str, merge_base: Option<&str>, m: Mergeable, conflicts: &[String]) -> Result<()>; |
| 60 | pub async fn mark_merged(&self, issue_id: &str, actor_id: &str, merge_commit: &str) -> Result<()>; // closes the issue (state_reason completed) + 'merged' event {"commit"} |
| 61 | pub async fn find_open_pull(&self, base_repo: &str, base_branch: &str, head_repo: &str, head_branch: &str) -> Result<Option<i64>>; // prevents duplicates |
| 62 | // forks |
| 63 | pub async fn create_fork(&self, source: &Repo, owner_id: &str, name: &str) -> Result<Repo>; // sets fork_of |
| 64 | pub async fn list_forks(&self, repo_id: &str) -> Result<Vec<Repo>>; |
| 65 | ``` |
| 66 | `add_event` must accept the new kinds: `merged`, `head_updated` (data `{"from","to"}`), `base_changed`, and `head_deleted`. |
| 67 | |
| 68 | ## forge-git (`src/pulls.rs`) |
| 69 | ```rust |
| 70 | pub async fn fetch_into_pull_ref(env: &GitEnv, base_repo: &Path, head_repo: &Path, head_oid: &str, number: i64) -> Result<()>; |
| 71 | // same repo: `update-ref refs/pull/N/head <oid>`; fork: `git fetch --no-tags <head_repo_path> <oid>` then update-ref (uploadpack.allowAnySHA1InWant isn't needed for a local path fetch of a full oid; if it is, fetch the branch ref and verify the oid) |
| 72 | pub async fn merge_base(env, repo, a: &str, b: &str) -> Result<Option<String>>; |
| 73 | pub enum MergeCheck { Clean { tree: String }, Conflict { files: Vec<String> }, UpToDate } |
| 74 | pub async fn check_merge(env, repo, base_oid: &str, head_oid: &str) -> Result<MergeCheck>; |
| 75 | // `git merge-tree --write-tree --name-only --no-messages <base> <head>`; exit 1 = conflict (parse the file list); UpToDate if head is an ancestor of base |
| 76 | pub async fn commit_tree(env, repo, tree: &str, parents: &[&str], message: &str, author: &Signature, committer: &Signature) -> Result<String>; |
| 77 | // `git commit-tree`, with GIT_AUTHOR_*/GIT_COMMITTER_* env set from the signatures; the message goes in via stdin (never argv) |
| 78 | pub async fn update_ref_cas(env, repo, refname: &str, new: &str, old: &str) -> Result<()>; // `update-ref <ref> <new> <old>`, an atomic compare-and-swap |
| 79 | pub async fn range_diff_files(env, repo, base_oid: &str, head_oid: &str, limits: DiffLimits) -> Result<CommitDiff>; // three-dot: merge-base..head, same parsing as commit_diff |
| 80 | pub async fn range_log(env, repo, base_oid: &str, head_oid: &str, max: usize) -> Result<Vec<CommitInfo>>; // `log <head> ^<base>`, oldest first |
| 81 | pub struct Signature { pub name: String, pub email: String, pub time_unix: i64 } |
| 82 | ``` |
| 83 | **Merging (forge-web, `pulls_merge.rs`):** load the PR; require Write on the base repo, not archived, the PR open, and the head present. Recompute `check_merge` against the CURRENT base tip. |
| 84 | - **Merge:** `commit_tree(tree, [base, head], "Merge pull request #N from owner/branch\n\n<title>")`. |
| 85 | - **Squash:** `commit_tree(tree, [base], "<title> (#N)\n\n<body or commit list>")`, with the PR author as author. |
| 86 | - **FastForward:** only if base is an ancestor of head; new = head. |
| 87 | - Then `update_ref_cas(refs/heads/<base>, new, base)`. On a CAS failure, show "base moved, try again". |
| 88 | - **Branch protection:** a merge is never a force push. It runs as Write, so allowed. |
| 89 | - After the merge, call `mark_merged`, then run the same post-push side effects the hook would: closing keywords from the merged commits, the maintenance trigger, and recomputing mergeability of other PRs on that base. Factor the post-receive logic in `internal.rs` into a reusable `fn after_ref_update(state, repo, actor, refname, old, new)`, then call it from both places. |
| 90 | |
| 91 | ## Post-receive integration (forge-web `internal.rs`) |
| 92 | For every updated `refs/heads/<b>` of repo R (both paths, via `after_ref_update`): |
| 93 | - For each open PR whose head is (R, b): `fetch_into_pull_ref`, `update_pull_head`, a `head_updated` event, then recompute mergeability in the background. If the branch was deleted: a `head_deleted` event, and keep the last head. |
| 94 | - For each open PR whose base is (R, b): recompute mergeability in the background. |
| 95 | - If a push to the base branch makes a PR's head an ancestor of base (someone merged manually on the CLI), call `mark_merged` with the merge commit unknown. Use the base tip, and record the `merged` event. |
| 96 | |
| 97 | ## Web routes |
| 98 | - `GET /{o}/{r}/pulls`: list, same filters as issues; state open/closed, where closed also shows merged with a purple badge. `pulls_list.rs`. |
| 99 | - `GET /{o}/{r}/compare/{base}...{head}`, where head is `branch` or `owner:branch` for forks; also `GET /{o}/{r}/compare` with a base/head picker form (plain GET form). `compare.rs`. |
| 100 | - Shows the commits, the diff stat and the file diffs (reuse the commit page's diff rendering: extract it into a shared component in `views/diff.rs`). |
| 101 | - Shows an "Open pull request" form (title prefilled from the first commit summary, body) if the viewer can Read the base and there are commits. |
| 102 | - `POST /{o}/{r}/pulls` creates the PR (CSRF; a duplicate open PR for the same head and base redirects to the existing one). |
| 103 | - `GET /{o}/{r}/pulls/{n}` (Conversation tab), `/pulls/{n}/commits`, `/pulls/{n}/files`. `pull_view.rs`. |
| 104 | - Header: title, state badge (Open / Merged / Closed), "author wants to merge N commits into base from head". |
| 105 | - The Conversation tab reuses the issue timeline, sidebar and comment form. Factor the shared parts of `issue_view.rs` into reusable components; the issue page must keep working and its tests must keep passing. |
| 106 | - **Merge box:** mergeability status ("This branch has conflicts that must be resolved", listing the files; "Ready to merge"; "Checking…" when unknown), a style radio (merge, squash, ff when possible) and a Merge button for Write holders. `POST /{o}/{r}/pulls/{n}/merge`. |
| 107 | - Close and reopen work like issues. |
| 108 | - Repo tabs gain **Pull requests (N)** after Issues. |
| 109 | - Repo pages for a forkable repo show a **Fork** button in the header (`POST /fork`). The fork header shows "forked from". |
| 110 | - A banner on the repo home after a recent push to a non-default branch ("feature/x had recent pushes — Compare & pull request"): optional, skip if complex. |
| 111 | |
| 112 | ## Tests (each worker covers its part; the e2e test file `tests/pulls_e2e.rs` uses real git) |
| 113 | - Same-repo PR: push `feature`, open the PR via compare, see its commits and files, merge-commit merge, then main contains the feature commit, the PR is merged, and a "Fixes #1" in the PR commits closes issue 1. |
| 114 | - Fork PR: bob forks, pushes to his fork, opens a PR to alice's repo. Alice merges with squash, giving a single new commit on main with bob as author. |
| 115 | - A conflict is detected and lists the file; the Merge button is disabled and the POST is rejected. |
| 116 | - Pushing to the head branch updates the PR (a new commit shows, a `head_updated` event). |
| 117 | - A base that moved concurrently gives the CAS failure message (simulate by updating the base between check and update; a unit test in forge-git is fine). |
| 118 | - FF-only when possible; otherwise the FF option is hidden or rejected. |
| 119 | - Permissions: a Read user can open a PR from their fork but can't merge (403). Private base: anonymous gets 404. |
| 120 | - `#N` on the issue page redirects to the PR. |