diff --git a/ROADMAP.md b/ROADMAP.md index 4f28da4b..edaac0eb 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -706,7 +706,8 @@ skeleton, users, organizations, tokens, `admin` CLI), F2a (repositories API, lif and reconciler, collaborators, branches) and F2b (smart-HTTP git transport via `git http-backend`, `pre-receive` ref policy, admission control, threat-model pass #1) and F3a (content reads: refs, tags, commits, trees, contents, archives) and F3b (contents writes: create, update, delete as server-made commits) and F4a (branch protection -enforced on push, contents API and branch delete; commit statuses) are built. Hosted repos are +enforced on push, contents API and branch delete; commit statuses) and F4b (pull requests, reviews, comments, retention +refs) are built. Hosted repos are bare Git and get none of the `.sc/` features. Other follow-up work is tracked as GitHub issues and listed under Deferred diff --git a/crates/forge/conformance/FINDINGS.md b/crates/forge/conformance/FINDINGS.md index 3b208df7..ae067148 100644 --- a/crates/forge/conformance/FINDINGS.md +++ b/crates/forge/conformance/FINDINGS.md @@ -99,7 +99,7 @@ every failure is a difference listed in the last table. An empty repo whose first push is a branch other than the default still reports `empty:true`. **O and P (PR lifecycle, pagination)** -- Force-pushing the head moves `head.sha`. Deleting the head branch leaves the PR open; +- Force-pushing the head moves `head.sha`. Deleting the head branch **closes the PR** (O#6-O#10: `state:"closed"`, `head.ref:"refs/pull/N/head"`); `refs/pull/N/head` stays visible and merge is 404. A merged PR's `head.sha` is **not** frozen: it follows a recreated branch. Pushing the head commit straight into base does **not** mark the PR merged (still open after 15 s). Default page size is 30, max 50; `limit=0`, `-1` and `abc` fall back diff --git a/crates/forge/conformance/forge-check.sh b/crates/forge/conformance/forge-check.sh index f61beb6b..fbc54aae 100755 --- a/crates/forge/conformance/forge-check.sh +++ b/crates/forge/conformance/forge-check.sh @@ -25,4 +25,4 @@ for _ in $(seq 50); do done python3 -I "$here/run.py" check --base-url "http://127.0.0.1:$port" --admin-token "$token" \ --fixtures "$here/fixtures/1.22.6" --departures "$here/departures.json" --pending "$here/slice-pending.json" \ - --only A,B,C,D,E,H,I,J,K,L,M,N,Q,R,V,W "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; } + --only A,B,C,D,E,F,H,I,J,K,L,M,N,O,P,Q,R,V,W "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; } diff --git a/crates/forge/conformance/slice-pending.json b/crates/forge/conformance/slice-pending.json index f4c474b9..c33d16be 100644 --- a/crates/forge/conformance/slice-pending.json +++ b/crates/forge/conformance/slice-pending.json @@ -1,12 +1,4 @@ { - "D": { - "defer": [ - [ - 13, - "GET /api/v1/repos/acme/blank/pulls?state=all" - ] - ] - }, "K": { "defer": [ [ @@ -21,8 +13,54 @@ }, "Q": { "stop": [ - 5, - "expected [201]" + 8, + "origin/main~1" + ], + "defer": [ + [ + 5, + "POST /api/v1/repos/acme/bypass/pulls//merge" + ] + ] + }, + "O": { + "defer": [ + [ + 9, + "POST /api/v1/repos/acme/life/pulls//merge" + ], + [ + 19, + "POST /api/v1/repos/acme/life/pulls//merge" + ], + [ + 20, + "GET /api/v1/repos/acme/life/pulls/" + ], + [ + 21, + "merged_pr_head_sha_frozen" + ], + [ + 22, + "merged_pr_head_sha_follows_new_branch" + ], + [ + 23, + "PATCH /api/v1/repos/acme/life/pulls/" + ], + [ + 25, + "GET /api/v1/repos/acme/life/pulls/" + ], + [ + 26, + "PATCH /api/v1/repos/acme/life/pulls/" + ], + [ + 27, + "GET /api/v1/repos/acme/life/pulls/" + ] ] } } diff --git a/crates/forge/src/api.rs b/crates/forge/src/api.rs index a21a368a..632fe7d1 100644 --- a/crates/forge/src/api.rs +++ b/crates/forge/src/api.rs @@ -44,6 +44,20 @@ pub(crate) fn user_json(base: &str, u: &User) -> Value { }) } +/// The user object Gitea nests inside statuses, reviews and comments: inactive, never a site +/// admin, no login name. `mask_email` replaces the stored address with `@noreply.localhost` +/// (recorded for statuses and comments; reviews show the stored one). +pub(crate) fn nested_user_json(base: &str, u: &User, mask_email: bool) -> Value { + let mut j = user_json(base, u); + j["active"] = json!(false); + j["is_admin"] = json!(false); + j["login_name"] = json!(""); + if mask_email { + j["email"] = json!(format!("{}@noreply.localhost", u.login)); + } + j +} + pub(crate) fn org_json(base: &str, o: &User) -> Value { json!({ "id": o.id, @@ -119,7 +133,7 @@ pub(crate) fn paged( limit: i64, items: Vec, ) -> Resp { - paged_with(ctx, path, total, page, limit, &[], items) + paged_with(ctx, path, total, page, limit, items) } /// Percent-encode everything except the RFC 3986 unreserved characters, so a value can sit inside @@ -136,79 +150,67 @@ pub(crate) fn pct_encode(s: &str) -> String { out } -/// Like [`paged`], carrying `extra` query parameters (already strictly percent-encoded with [`pct_encode`], in the order -/// Gitea emits them: alphabetical after `limit` and `page`) into every `Link` URL, so following -/// a page link repeats the same query. +/// Like [`paged`]; the page links repeat the request's own query (see [`link_header`]). pub(crate) fn paged_with( ctx: &Ctx, path: &str, total: i64, page: i64, limit: i64, - extra: &[(&str, String)], items: Vec, ) -> Resp { let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string()); - if let Some(l) = link_header(ctx, path, total, page, limit, extra) { + if let Some(l) = link_header(ctx, path, total, page, limit) { r = r.with("link", l); } r } /// Gitea's `Link` header value for page `page` of `total` rows, or `None` when there is one page. +/// Each link repeats the request's own query parameters (first value of a repeated one, sorted by +/// name) with `page` replaced, and carries `limit` only when the request did (recorded P#19-32: +/// `?page=2` for a bare request, `?limit=0&page=2` when the client sent `limit=0`). The page is +/// clamped to the last one, as Gitea does when building the links. pub(crate) fn link_header( ctx: &Ctx, path: &str, total: i64, page: i64, limit: i64, - extra: &[(&str, String)], ) -> Option { - let tail: String = extra.iter().map(|(k, v)| format!("&{k}={v}")).collect(); - let last = ((total + limit - 1) / limit).max(1); - let url = |p: i64, rel: &str| { - format!( - "<{}/api/v1{path}?limit={limit}&page={p}{tail}>; rel=\"{rel}\"", - ctx.app.base - ) - }; - let mut links = Vec::new(); - if page < last { - links.push(url(page + 1, "next")); - links.push(url(last, "last")); - } - if page > 1 { - links.push(url(1, "first")); - links.push(url(page - 1, "prev")); - } - (!links.is_empty()).then(|| links.join(",")) + links(ctx, path, total, page, limit, false) } -/// Like [`paged`], but a page link repeats *every* request parameter, alphabetically by name (what -/// Gitea does for the commits listing), with `limit` and `page` set to the link's own values. -pub(crate) fn paged_sorted( +fn links( ctx: &Ctx, path: &str, total: i64, page: i64, limit: i64, - items: Vec, -) -> Resp { + with_limit: bool, +) -> Option { let mut q: std::collections::BTreeMap = ctx .req .query .split('&') - .filter(|kv| !kv.is_empty()) - .map(|kv| { - let (k, v) = kv.split_once('=').unwrap_or((kv, "")); - (k.to_string(), v.to_string()) + // Parsed exactly like `Req::param` (raw names, `name=value` pairs only, first value wins), + // so the next page selects the rows this one did. + .filter_map(|kv| kv.split_once('=')) + .map(|(k, v)| { + // Re-encoded strictly, never echoed raw: a `>` or `"` in the request must not close the + // link target or start another entry. Names stay raw-equivalent (not decoded). + let dec = |t: &str| { + crate::routes::percent_decode(&t.replace('+', " ")) + .map_or_else(|| pct_encode(t), |d| pct_encode(&d)) + }; + (pct_encode(k), dec(v)) }) .rev() // the request parser reads the first value of a repeated parameter; the last insert wins .collect(); - q.insert("limit".into(), limit.to_string()); + if with_limit { + q.insert("limit".into(), limit.to_string()); + } let last = ((total + limit - 1) / limit).max(1); - // Gitea builds the links from the clamped page: asking past the end still points `prev` at the - // page before the last. let page = page.min(last); let mut url = |p: i64, rel: &str| { q.insert("page".into(), p.to_string()); @@ -228,9 +230,21 @@ pub(crate) fn paged_sorted( links.push(url(1, "first")); links.push(url(page - 1, "prev")); } + (!links.is_empty()).then(|| links.join(",")) +} + +/// Like [`paged`], but every link also carries `limit` (what Gitea does for the commits listing). +pub(crate) fn paged_sorted( + ctx: &Ctx, + path: &str, + total: i64, + page: i64, + limit: i64, + items: Vec, +) -> Resp { let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string()); - if !links.is_empty() { - r = r.with("link", links.join(",")); + if let Some(l) = links(ctx, path, total, page, limit, true) { + r = r.with("link", l); } r } diff --git a/crates/forge/src/cgi.rs b/crates/forge/src/cgi.rs index c3a32b6d..2a992d8e 100644 --- a/crates/forge/src/cgi.rs +++ b/crates/forge/src/cgi.rs @@ -811,6 +811,9 @@ impl Supervise { let status = child.wait(); let ok = reason == "exited" && status.as_ref().is_ok_and(|s| s.success()); let _ = verdict.send(ok); + // The child is gone: release the transport registration, so a repository delete waiting + // for it is not also waiting on the sync below (which takes the shared lifecycle lock). + drop(reg); let err = tail.and_then(|t| t.join().ok()).unwrap_or_default(); if !ok { eprintln!( @@ -821,6 +824,7 @@ impl Supervise { } else if receive && marker.exists() { if let Ok(conn) = crate::db::connect(&app.data_dir) { let _ = repos::touch(&conn, repo_id, crate::timefmt::now()); + crate::pulls::sync(&app.life, &conn, repo_id, crate::timefmt::now()); } } let _ = std::fs::remove_file(&marker); diff --git a/crates/forge/src/cli.rs b/crates/forge/src/cli.rs index 6fac5b35..9a7a3592 100644 --- a/crates/forge/src/cli.rs +++ b/crates/forge/src/cli.rs @@ -313,6 +313,8 @@ fn run_serve(a: ServeArgs) -> Result<()> { if report != scl_forge::lifecycle::Report::default() { eprintln!("sc-forge: reconciled {report:?}"); } + // Pull request refs and rows agree again before anything is served. + scl_forge::pulls::reconcile_all(&life, &db::connect(&a.data_dir)?, scl_forge::timefmt::now()); let tcfg = scl_forge::cgi::Cfg { max_transports: a.max_transports, per_repo: a.max_transports_per_repo, diff --git a/crates/forge/src/content_api.rs b/crates/forge/src/content_api.rs index f19562fb..7f1ea4a6 100644 --- a/crates/forge/src/content_api.rs +++ b/crates/forge/src/content_api.rs @@ -72,7 +72,12 @@ fn no_refs(ctx: &Ctx, a: &Access) -> Result { all_refs(ctx, a).map(|rows| rows.is_empty()) } -fn resolve(ctx: &Ctx, a: &Access, rev: &str, peel: Peel) -> Result, Resp> { +pub(crate) fn resolve( + ctx: &Ctx, + a: &Access, + rev: &str, + peel: Peel, +) -> Result, Resp> { gitobj::resolve( &ctx.app.life.git, &dir(ctx, a), @@ -83,20 +88,20 @@ fn resolve(ctx: &Ctx, a: &Access, rev: &str, peel: Peel) -> Result(ctx: &Ctx, a: &Access, r: Result) -> Result { +pub(crate) fn git_err(ctx: &Ctx, a: &Access, r: Result) -> Result { r.map_err(|e| read_err(ctx, a, e)) } // ---- JSON pieces ---- /// Commit authors resolved to accounts by email, once per request. -struct Users<'a> { +pub(crate) struct Users<'a> { ctx: &'a Ctx<'a>, cache: HashMap, } impl<'a> Users<'a> { - fn new(ctx: &'a Ctx<'a>) -> Self { + pub(crate) fn new(ctx: &'a Ctx<'a>) -> Self { Users { ctx, cache: HashMap::new(), @@ -104,7 +109,7 @@ impl<'a> Users<'a> { } /// The account owning `email`, or `null` (recorded: commits by unknown addresses). - fn actor(&mut self, email: &str) -> Result { + pub(crate) fn actor(&mut self, email: &str) -> Result { if let Some(v) = self.cache.get(&email.to_lowercase()) { return Ok(v.clone()); } @@ -127,7 +132,7 @@ pub(crate) fn verification() -> Value { json!({"verified": false, "reason": "gpg.error.not_signed_commit", "signature": "", "signer": null, "payload": ""}) } -fn person_json(p: &gitread::Person) -> Value { +pub(crate) fn person_json(p: &gitread::Person) -> Value { json!({"name": p.name, "email": p.email, "date": rfc3339(p.when)}) } @@ -136,10 +141,10 @@ fn commit_meta(ctx: &Ctx, a: &Access, sha: &str, when: i64) -> Value { } /// Files and line counts a commit response may carry (`None` for each the client did not ask for). -type Changes = (Option>, Option); +pub(crate) type Changes = (Option>, Option); /// The Gitea commit object. `changes` fills `files` and `stats` (`None` leaves them `null`). -fn commit_json( +pub(crate) fn commit_json( ctx: &Ctx, a: &Access, users: &mut Users, @@ -318,7 +323,6 @@ pub fn list_tags(ctx: &mut Ctx) -> Resp { total, page, limit, - &[], items, ) }) @@ -336,7 +340,7 @@ pub fn list_commits(ctx: &mut Ctx) -> Resp { } /// Changed files one `commits` response may carry in total (a few hundred bytes of JSON each). -const MAX_PAGE_FILES: usize = 50_000; +pub(crate) const MAX_PAGE_FILES: usize = 50_000; fn list_commits_inner(ctx: &Ctx, a: &Access) -> Resp { if ctx.req.param("not").is_some() { diff --git a/crates/forge/src/content_write.rs b/crates/forge/src/content_write.rs index 9ee42ab7..00a9a007 100644 --- a/crates/forge/src/content_write.rs +++ b/crates/forge/src/content_write.rs @@ -486,6 +486,8 @@ fn published(ctx: &Ctx, a: &Access, resp: Resp) -> Resp { a.repo.id ); } + // The branch moved: PRs on it follow (the write gate is held). + crate::pulls_api::sync_held(ctx, a); resp } diff --git a/crates/forge/src/db.rs b/crates/forge/src/db.rs index 7119119d..e0a1eab8 100644 --- a/crates/forge/src/db.rs +++ b/crates/forge/src/db.rs @@ -100,6 +100,69 @@ const MIGRATIONS: &[&str] = &[ updated_unix INTEGER NOT NULL ); CREATE INDEX commit_statuses_by_sha ON commit_statuses (repo_id, sha, context, id);", + // 4: pull requests (one number sequence per repository, one open PR per head/base pair), + // the mergeability cache keyed by the two immutable object ids, reviews, and issue comments + // (the number of an issue comment is the PR number). `state` merged is set by F4c. + "CREATE TABLE pulls ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + number INTEGER NOT NULL, + head_ref TEXT NOT NULL, + base_ref TEXT NOT NULL, + base_sha TEXT NOT NULL, + head_sha TEXT NOT NULL, + title TEXT NOT NULL, + body TEXT NOT NULL DEFAULT '', + state TEXT NOT NULL CHECK (state IN ('open', 'closed', 'merged')), + creator_id INTEGER NOT NULL REFERENCES users(id), + created_unix INTEGER NOT NULL, + updated_unix INTEGER NOT NULL, + closed_unix INTEGER, + final_head_sha TEXT, + merge_commit_sha TEXT, + merged_unix INTEGER, + merged_by INTEGER REFERENCES users(id), + merge_base TEXT, + changed_files INTEGER, + additions INTEGER, + deletions INTEGER, + head_gone INTEGER NOT NULL DEFAULT 0, + UNIQUE (repo_id, number) + ); + CREATE UNIQUE INDEX pulls_one_open ON pulls (repo_id, head_ref, base_ref) WHERE state = 'open'; + CREATE TABLE mergeability_cache ( + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + base_sha TEXT NOT NULL, + head_sha TEXT NOT NULL, + mergeable INTEGER NOT NULL, + merge_base TEXT NOT NULL DEFAULT '', + changed_files INTEGER NOT NULL, + additions INTEGER NOT NULL, + deletions INTEGER NOT NULL, + PRIMARY KEY (repo_id, base_sha, head_sha) + ); + CREATE TABLE reviews ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + pull_number INTEGER NOT NULL, + reviewer_id INTEGER NOT NULL REFERENCES users(id), + state TEXT NOT NULL CHECK (state IN ('APPROVED', 'REQUEST_CHANGES', 'COMMENT', 'PENDING')), + body TEXT NOT NULL, + commit_id TEXT NOT NULL, + official INTEGER NOT NULL, + created_unix INTEGER NOT NULL + ); + CREATE INDEX reviews_by_pull ON reviews (repo_id, pull_number, id); + CREATE TABLE issue_comments ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + number INTEGER NOT NULL, + author_id INTEGER NOT NULL REFERENCES users(id), + body TEXT NOT NULL, + created_unix INTEGER NOT NULL, + updated_unix INTEGER NOT NULL + ); + CREATE INDEX issue_comments_by_issue ON issue_comments (repo_id, number, id);", ]; /// Every table the current schema creates; `/healthz` probes each, so a missing or unreadable one @@ -112,6 +175,10 @@ pub const TABLES: &[&str] = &[ "collaborators", "branch_protections", "commit_statuses", + "pulls", + "mergeability_cache", + "reviews", + "issue_comments", ]; /// Database failures a caller reports to the operator. diff --git a/crates/forge/src/gitobj.rs b/crates/forge/src/gitobj.rs index 2acff3aa..31ae9308 100644 --- a/crates/forge/src/gitobj.rs +++ b/crates/forge/src/gitobj.rs @@ -445,6 +445,136 @@ pub fn patch_text(git: &Git, dir: &Path, oid: &str, merge: bool) -> Result, + oid: &str, + skip: usize, + limit: usize, +) -> Result, GitError> { + let (skip, limit) = (format!("--skip={skip}"), format!("--max-count={limit}")); + let not = exclude.map(|e| format!("^{e}")); + let mut args = vec!["log", "-z", LOG_FORMAT, skip.as_str(), limit.as_str()]; + if let Some(n) = ¬ { + args.push(n.as_str()); + } + args.extend(["--end-of-options", oid]); + Ok(parse_log(&must(run(git, dir, &args, Some(BIG))?)?)) +} + +/// How many commits `exclude..oid` holds. +pub fn range_count( + git: &Git, + dir: &Path, + exclude: Option<&str>, + oid: &str, +) -> Result { + let not = exclude.map(|e| format!("^{e}")); + let mut args = vec!["rev-list", "--count"]; + if let Some(n) = ¬ { + args.push(n.as_str()); + } + args.extend(["--end-of-options", oid]); + let out = must(run(git, dir, &args, None)?)?; + Ok(String::from_utf8_lossy(&out).trim().parse().unwrap_or(0)) +} + +/// A file changed between two trees, with its own line counts. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FileStat { + pub name: String, + pub status: &'static str, + pub additions: u64, + pub deletions: u64, +} + +/// Per-file changes from `base` (`None` = the empty tree) to `oid`. +pub fn file_stats( + git: &Git, + dir: &Path, + base: Option<&str>, + oid: &str, +) -> Result, GitError> { + let base = base.unwrap_or(EMPTY_TREE); + let out = must(run( + git, + dir, + &[ + "diff-tree", + "-r", + "-z", + "--no-renames", + "--no-ext-diff", + "--raw", + "--numstat", + "--end-of-options", + base, + oid, + ], + Some(BIG), + )?)?; + let text = String::from_utf8_lossy(&out); + let mut toks = text.split('\0'); + let mut files: Vec<(String, &'static str)> = Vec::new(); + let mut nums: Vec<(u64, u64)> = Vec::new(); + while let Some(t) = toks.next() { + if let Some(meta) = t.strip_prefix(':') { + let status = meta.split_whitespace().last().unwrap_or("M"); + let Some(name) = toks.next() else { break }; + files.push(( + name.to_string(), + match status.chars().next() { + Some('A') => "added", + Some('D') => "removed", + _ => "modified", + }, + )); + } else if let Some((a, rest)) = t.split_once('\t') { + if let Some((d, _path)) = rest.split_once('\t') { + nums.push((a.parse().unwrap_or(0), d.parse().unwrap_or(0))); + } + } + } + Ok(files + .into_iter() + .zip(nums.into_iter().chain(std::iter::repeat((0, 0)))) + .map(|((name, status), (additions, deletions))| FileStat { + name, + status, + additions, + deletions, + }) + .collect()) +} + +/// Mailbox patches of every commit in `exclude..oid` (`format-patch --stdout`), oldest first. +pub fn range_patch( + git: &Git, + dir: &Path, + exclude: Option<&str>, + oid: &str, +) -> Result, GitError> { + let range = exclude.map_or_else(|| oid.to_string(), |e| format!("{e}..{oid}")); + must(run( + git, + dir, + &[ + "format-patch", + "--stdout", + "--root", + "--no-color", + "--no-ext-diff", + "--no-textconv", + "--no-renames", + "--end-of-options", + &range, + ], + Some(BIG), + )?) +} + /// One entry of a tree. #[derive(Debug, Clone, PartialEq, Eq)] pub struct TreeEntry { diff --git a/crates/forge/src/gitread.rs b/crates/forge/src/gitread.rs index 60759712..f65cf64b 100644 --- a/crates/forge/src/gitread.rs +++ b/crates/forge/src/gitread.rs @@ -33,13 +33,15 @@ pub struct CommitInfo { pub committer: Person, } -/// All branches, sorted by name. +/// All branches, newest commit first, ties by name (recorded P#19-22: Gitea lists the branch that was +/// committed to last first). pub fn list_branches(git: &Git, dir: &Path) -> Result, GitError> { let out = git.run(&Spec { repo: Some(dir), args: &[ "for-each-ref", "--sort=refname", + "--sort=-committerdate", "--format=%(refname)%00%(objectname)", "refs/heads", ], @@ -307,4 +309,54 @@ mod tests { ); assert!(person("garbage").is_none()); } + + #[test] + fn branches_list_newest_commit_first_with_ties_by_name() { + let (g, d, dir) = repo("order"); + let id_at = |t: i64| Identity::new("T", "t@example.invalid").unwrap().at(t); + let tree = g + .run(&Spec { + repo: Some(&dir), + args: &["mktree"], + stdin: Some(b""), + ..Default::default() + }) + .unwrap() + .text(); + // alphabetical order (aaa, bbb, mmm, zzz) conflicts with the date order below + for (branch, when) in [ + ("aaa", 1_700_000_100), + ("mmm", 1_700_000_200), + ("zzz", 1_700_000_300), + ("bbb", 1_700_000_300), + ] { + let id = id_at(when); + let oid = g + .run(&Spec { + repo: Some(&dir), + args: &["commit-tree", &tree], + stdin: Some(b"m"), + identity: Some(&id), + ..Default::default() + }) + .unwrap() + .text(); + assert!(g + .run(&Spec { + repo: Some(&dir), + args: &["update-ref", &refname::branch_ref(branch), &oid], + ..Default::default() + }) + .unwrap() + .ok()); + } + let names: Vec = list_branches(&g, &dir) + .unwrap() + .into_iter() + .map(|b| b.name) + .collect(); + assert_eq!(names, ["bbb", "zzz", "mmm", "aaa"]); + std::fs::remove_dir_all(&d).unwrap(); + assert!(!d.exists()); + } } diff --git a/crates/forge/src/hook.rs b/crates/forge/src/hook.rs index 22b7192d..0e7f2e88 100644 --- a/crates/forge/src/hook.rs +++ b/crates/forge/src/hook.rs @@ -158,6 +158,45 @@ pub fn install(data_dir: &Path, exe: &Path) -> std::io::Result<()> { std::fs::rename(&tmp, hooks.join("pre-receive")) } +/// Before a push deletes the head branch of an open pull request, record the branch's tip in the +/// PR's retention ref (`old` is the tip the push deletes). The spawn has no quarantine variables, +/// so git accepts the ref update; `old` already exists in the repository. A failure refuses the +/// push: deleting the branch would otherwise lose the only reference to that commit. +fn retain_deleted_heads( + data_dir: &Path, + repo_id: i64, + git_bin: Option<&Path>, + lines: &[String], +) -> Result<(), String> { + let deletes: Vec<(&str, &str)> = lines + .iter() + .filter_map(|l| { + let mut it = l.splitn(3, ' '); + let (old, new, name) = (it.next()?, it.next()?, it.next()?); + (policy::is_delete(new) && !policy::is_delete(old)) + .then(|| name.strip_prefix("refs/heads/").map(|b| (old, b))) + .flatten() + }) + .collect(); + if deletes.is_empty() { + return Ok(()); + } + let git_bin = git_bin.ok_or("FORGE_GIT_BIN is not set")?; + let conn = db::open_readonly(data_dir).map_err(|e| e.to_string())?; + let open = crate::pulls::open_pulls(&conn, repo_id).map_err(|e| e.to_string())?; + let git = Git::new(data_dir, git_bin.to_path_buf(), 1).map_err(|e| e.to_string())?; + let repo = git.data_dir().join("repos").join(format!("{repo_id}.git")); + for (old, branch) in deletes { + for p in open.iter().filter(|p| p.head_ref == branch) { + if !full_id(old) { + return Err("malformed object id".into()); + } + crate::pulls::retain(&git, &repo, p.number, old).map_err(|e| e.to_string())?; + } + } + Ok(()) +} + /// Entry point: returns the process exit code. pub fn run() -> i32 { let (Some(data), Some(id)) = ( @@ -191,6 +230,16 @@ pub fn run() -> i32 { None => Err("FORGE_GIT_BIN is not set".into()), }; let refusals = decide(&lines, &info.default_branch, info.live, &info.rules, &is_ff); + let refusals = if refusals.is_empty() { + match retain_deleted_heads(Path::new(&data), id, git_bin.as_deref(), &lines) { + Ok(()) => refusals, + Err(e) => vec![format!( + "refusing the push: cannot keep the head of a pull request ({e})" + )], + } + } else { + refusals + }; for r in &refusals { eprintln!("{r}"); } diff --git a/crates/forge/src/lib.rs b/crates/forge/src/lib.rs index d2b2d137..69c71ff5 100644 --- a/crates/forge/src/lib.rs +++ b/crates/forge/src/lib.rs @@ -26,10 +26,13 @@ pub mod limits; pub mod names; pub mod policy; pub mod protection; +pub mod pulls; +pub mod pulls_api; pub mod refname; pub mod registry; pub mod repo_api; pub mod repos; +pub mod reviews; pub mod routes; pub mod scope; pub mod server; diff --git a/crates/forge/src/lifecycle.rs b/crates/forge/src/lifecycle.rs index c36b2263..41d6fa88 100644 --- a/crates/forge/src/lifecycle.rs +++ b/crates/forge/src/lifecycle.rs @@ -514,6 +514,9 @@ impl Lifecycle { )) } }; + // The head of an open PR is retained before its branch can go. + crate::pulls::retain_for_branch(conn, &self.git, &dir, current.id, branch, &tip) + .map_err(|e| LifecycleError::GitFailed(e.to_string()))?; let out = self.git.run(&Spec { repo: Some(&dir), args: &["update-ref", "-d", &full, &tip], diff --git a/crates/forge/src/pulls.rs b/crates/forge/src/pulls.rs new file mode 100644 index 00000000..de449b2f --- /dev/null +++ b/crates/forge/src/pulls.rs @@ -0,0 +1,693 @@ +//! Pull requests: storage, the git computations behind a PR object, and the retention refs. +//! +//! A PR is a row naming a head and a base branch of one repository (forks are a non-goal). Its +//! `head.sha` is the live tip of the head branch; when that branch is gone it is the head the +//! server retained in `refs/forge/pull//head`, and a PR whose head branch is deleted is closed +//! (recorded, both Gitea versions). A merged PR's head and stats are frozen at merge. +//! +//! The retention ref is written only by the server, always under the repository's write gate +//! (`lifecycle`'s lock order), or by the `pre-receive` hook for a push that deletes the head +//! branch of an open PR. [`sync_locked`] makes refs and rows agree after anything that can move a +//! branch (a push, a contents write, a branch delete, startup); reads never write. + +use crate::gitobj; +use crate::gitproc::{Git, GitError, Spec}; +use crate::lifecycle::{Lifecycle, WriteRun}; +use rusqlite::{params, Connection, OptionalExtension, Transaction, TransactionBehavior}; +use std::collections::HashMap; +use std::path::Path; +use std::time::Duration; + +/// Retention ref of PR `n`. +pub fn retention_ref(n: i64) -> String { + format!("refs/forge/pull/{n}/head") +} + +/// A stored pull request. +#[derive(Clone, Debug)] +pub struct Pull { + pub id: i64, + pub repo_id: i64, + pub number: i64, + pub head_ref: String, + pub base_ref: String, + pub base_sha: String, + pub head_sha: String, + pub title: String, + pub body: String, + /// `open`, `closed` or `merged`. + pub state: String, + pub creator_id: i64, + pub created: i64, + pub updated: i64, + pub closed: Option, + pub final_head: Option, + pub merge_commit: Option, + pub merged: Option, + pub merged_by: Option, + pub merge_base: Option, + pub changed_files: Option, + pub additions: Option, + pub deletions: Option, + /// Open with a head branch that no longer exists (reopened after its branch was deleted). + pub head_gone: bool, +} + +const COLS: &str = + "id, repo_id, number, head_ref, base_ref, base_sha, head_sha, title, body, state, \ + creator_id, created_unix, updated_unix, closed_unix, final_head_sha, merge_commit_sha, \ + merged_unix, merged_by, merge_base, changed_files, additions, deletions, head_gone"; + +fn row(r: &rusqlite::Row) -> rusqlite::Result { + Ok(Pull { + id: r.get(0)?, + repo_id: r.get(1)?, + number: r.get(2)?, + head_ref: r.get(3)?, + base_ref: r.get(4)?, + base_sha: r.get(5)?, + head_sha: r.get(6)?, + title: r.get(7)?, + body: r.get(8)?, + state: r.get(9)?, + creator_id: r.get(10)?, + created: r.get(11)?, + updated: r.get(12)?, + closed: r.get(13)?, + final_head: r.get(14)?, + merge_commit: r.get(15)?, + merged: r.get(16)?, + merged_by: r.get(17)?, + merge_base: r.get(18)?, + changed_files: r.get(19)?, + additions: r.get(20)?, + deletions: r.get(21)?, + head_gone: r.get::<_, i64>(22)? != 0, + }) +} + +/// PR `number` of the repository. +pub fn find(conn: &Connection, repo_id: i64, number: i64) -> rusqlite::Result> { + conn.query_row( + &format!("SELECT {COLS} FROM pulls WHERE repo_id = ?1 AND number = ?2"), + params![repo_id, number], + row, + ) + .optional() +} + +/// The open PR with exactly these targets, if any. +pub fn find_open( + conn: &Connection, + repo_id: i64, + head: &str, + base: &str, +) -> rusqlite::Result> { + conn.query_row( + &format!( + "SELECT {COLS} FROM pulls WHERE repo_id = ?1 AND head_ref = ?2 AND base_ref = ?3 AND state = 'open'" + ), + params![repo_id, head, base], + row, + ) + .optional() +} + +/// Every open PR of the repository. +pub fn open_pulls(conn: &Connection, repo_id: i64) -> rusqlite::Result> { + let mut st = conn.prepare(&format!( + "SELECT {COLS} FROM pulls WHERE repo_id = ?1 AND state = 'open' ORDER BY number" + ))?; + let rows = st.query_map([repo_id], row)?.collect(); + rows +} + +/// How a listing is ordered (`sort=`). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Sort { + Newest, + Oldest, + RecentUpdate, +} + +/// PRs of the repository in `states`, a page of them, and the total. +pub fn list( + conn: &Connection, + repo_id: i64, + states: &[&str], + sort: Sort, + offset: i64, + limit: i64, +) -> rusqlite::Result<(Vec, i64)> { + let marks = vec!["?"; states.len()].join(","); + let order = match sort { + Sort::Newest => "number DESC", + Sort::Oldest => "number ASC", + Sort::RecentUpdate => "updated_unix DESC, number DESC", + }; + let mut args: Vec = vec![repo_id.into()]; + args.extend(states.iter().map(|s| (*s).to_string().into())); + let total = conn.query_row( + &format!("SELECT COUNT(*) FROM pulls WHERE repo_id = ? AND state IN ({marks})"), + rusqlite::params_from_iter(args.iter()), + |r| r.get(0), + )?; + args.push(limit.into()); + args.push(offset.into()); + let mut st = conn.prepare(&format!( + "SELECT {COLS} FROM pulls WHERE repo_id = ? AND state IN ({marks}) ORDER BY {order} LIMIT ? OFFSET ?" + ))?; + let rows = st + .query_map(rusqlite::params_from_iter(args.iter()), row)? + .collect::>>()?; + Ok((rows, total)) +} + +/// The number the next PR of the repository gets. +pub fn next_number(conn: &Connection, repo_id: i64) -> rusqlite::Result { + conn.query_row( + "SELECT COALESCE(MAX(number), 0) + 1 FROM pulls WHERE repo_id = ?1", + [repo_id], + |r| r.get(0), + ) +} + +/// Insert a PR with the next number of the repository (PRs and, one day, issues share it). +pub fn insert( + conn: &Connection, + repo_id: i64, + head: &str, + base: &str, + base_sha: &str, + head_sha: &str, + title: &str, + body: &str, + creator: i64, + now: i64, +) -> rusqlite::Result { + let tx = Transaction::new_unchecked(conn, TransactionBehavior::Immediate)?; + let number: i64 = tx.query_row( + "SELECT COALESCE(MAX(number), 0) + 1 FROM pulls WHERE repo_id = ?1", + [repo_id], + |r| r.get(0), + )?; + tx.execute( + "INSERT INTO pulls (repo_id, number, head_ref, base_ref, base_sha, head_sha, title, body, \ + state, creator_id, created_unix, updated_unix) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, 'open', ?9, ?10, ?10)", + params![repo_id, number, head, base, base_sha, head_sha, title, body, creator, now], + )?; + let p = tx.query_row( + &format!("SELECT {COLS} FROM pulls WHERE repo_id = ?1 AND number = ?2"), + params![repo_id, number], + row, + )?; + tx.commit()?; + Ok(p) +} + +/// Write back the editable and lifecycle columns of `p`. +pub fn save(conn: &Connection, p: &Pull) -> rusqlite::Result<()> { + conn.execute( + "UPDATE pulls SET base_ref = ?1, base_sha = ?2, head_sha = ?3, title = ?4, body = ?5, state = ?6, \ + updated_unix = ?7, closed_unix = ?8, final_head_sha = ?9, head_gone = ?10 WHERE id = ?11", + params![ + p.base_ref, + p.base_sha, + p.head_sha, + p.title, + p.body, + p.state, + p.updated, + p.closed, + p.final_head, + p.head_gone, + p.id + ], + )?; + Ok(()) +} + +/// Comments on the PR (the issue-comment count the PR object reports). +pub fn comment_count(conn: &Connection, repo_id: i64, number: i64) -> rusqlite::Result { + conn.query_row( + "SELECT COUNT(*) FROM issue_comments WHERE repo_id = ?1 AND number = ?2", + params![repo_id, number], + |r| r.get(0), + ) +} + +// ---- git: what a PR is made of ---- + +/// What `base...head` is: cached by the two object ids, which are immutable. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Compute { + pub mergeable: bool, + pub merge_base: Option, + pub changed_files: i64, + pub additions: i64, + pub deletions: i64, +} + +fn cached( + conn: &Connection, + repo_id: i64, + base: &str, + head: &str, +) -> rusqlite::Result> { + conn.query_row( + "SELECT mergeable, merge_base, changed_files, additions, deletions FROM mergeability_cache \ + WHERE repo_id = ?1 AND base_sha = ?2 AND head_sha = ?3", + params![repo_id, base, head], + |r| { + Ok(Compute { + mergeable: r.get::<_, i64>(0)? != 0, + merge_base: r.get::<_, Option>(1)?.filter(|s| !s.is_empty()), + changed_files: r.get(2)?, + additions: r.get(3)?, + deletions: r.get(4)?, + }) + }, + ) + .optional() +} + +/// The merge base of two commits, or `None` for unrelated histories. +pub fn merge_base(git: &Git, dir: &Path, a: &str, b: &str) -> Result, GitError> { + let out = git.run(&Spec { + repo: Some(dir), + args: &["merge-base", "--end-of-options", a, b], + ..Default::default() + })?; + match out.status.code() { + Some(0) => Ok(Some(out.text())), + Some(1) => Ok(None), + _ => Err(GitError::Failed( + String::from_utf8_lossy(&out.stderr).trim().to_string(), + )), + } +} + +/// Does merging `head` into `base` conflict? `merge-tree --write-tree`: 0 clean, 1 conflicts, +/// anything else is an error (never inferred from output). +pub fn merges_cleanly(git: &Git, dir: &Path, base: &str, head: &str) -> Result { + let out = git.run(&Spec { + repo: Some(dir), + args: &[ + "merge-tree", + "--write-tree", + "--no-messages", + "--end-of-options", + base, + head, + ], + ..Default::default() + })?; + match out.status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => Err(GitError::Failed( + String::from_utf8_lossy(&out.stderr).trim().to_string(), + )), + } +} + +/// Compute (or read from the cache) the facts of `base...head`. +pub fn compute( + conn: &Connection, + git: &Git, + dir: &Path, + repo_id: i64, + base: &str, + head: &str, +) -> Result { + if let Some(c) = cached(conn, repo_id, base, head)? { + return Ok(c); + } + let mb = merge_base(git, dir, base, head)?; + let (files, stats) = gitobj::changes(git, dir, mb.as_deref(), head)?; + // Unrelated histories cannot merge; `merge-tree` would die on them. + let mergeable = mb.is_some() && merges_cleanly(git, dir, base, head)?; + let c = Compute { + mergeable, + merge_base: mb, + changed_files: files.len() as i64, + additions: stats.additions as i64, + deletions: stats.deletions as i64, + }; + conn.execute( + "INSERT OR REPLACE INTO mergeability_cache (repo_id, base_sha, head_sha, mergeable, merge_base, \ + changed_files, additions, deletions) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", + params![ + repo_id, + base, + head, + c.mergeable, + c.merge_base.clone().unwrap_or_default(), + c.changed_files, + c.additions, + c.deletions + ], + )?; + // Bounded per repository: obsolete (base, head) pairs of branches that moved on go oldest first. + conn.execute( + "DELETE FROM mergeability_cache WHERE repo_id = ?1 AND rowid NOT IN \ + (SELECT rowid FROM mergeability_cache WHERE repo_id = ?1 ORDER BY rowid DESC LIMIT ?2)", + params![repo_id, CACHE_PER_REPO], + )?; + Ok(c) +} + +/// Mergeability rows kept per repository. +const CACHE_PER_REPO: i64 = 2_000; + +/// Why [`compute`] failed. +#[derive(Debug, thiserror::Error)] +pub enum ComputeError { + #[error("database error: {0}")] + Db(#[from] rusqlite::Error), + #[error("{0}")] + Git(#[from] GitError), +} + +// ---- retention refs ---- + +/// `(full ref name, object id)` of every ref under `prefix`. +pub fn refs_under( + git: &Git, + dir: &Path, + prefix: &str, +) -> Result, GitError> { + let out = git.run(&Spec { + repo: Some(dir), + args: &["for-each-ref", "--format=%(refname) %(objectname)", prefix], + ..Default::default() + })?; + if !out.ok() { + return Err(GitError::Failed( + String::from_utf8_lossy(&out.stderr).trim().to_string(), + )); + } + Ok(String::from_utf8_lossy(&out.stdout) + .lines() + .filter_map(|l| l.split_once(' ')) + .map(|(n, o)| (n.to_string(), o.to_string())) + .collect()) +} + +/// Point PR `n`'s retention ref at `oid` (a commit that exists), whatever it holds now. For the +/// authoritative writers: PR creation, close, and the record of a head that is about to be deleted. +pub fn retain(git: &Git, dir: &Path, n: i64, oid: &str) -> Result<(), GitError> { + let r = retention_ref(n); + let out = git.run(&Spec { + repo: Some(dir), + args: &["update-ref", &r, oid], + ..Default::default() + })?; + if out.ok() { + Ok(()) + } else { + Err(GitError::Failed( + String::from_utf8_lossy(&out.stderr).trim().to_string(), + )) + } +} + +/// Move PR `n`'s retention ref from `expected` (`None` = it must not exist) to `oid`, and only +/// then. `Ok(false)` means someone else changed it in the meantime (a push's hook records the tip +/// of a deleted head without the write gate): the caller leaves it alone, because a newer value +/// must never be overwritten by a stale snapshot. +pub fn retain_cas( + git: &Git, + dir: &Path, + n: i64, + oid: &str, + expected: Option<&str>, +) -> Result { + let r = retention_ref(n); + let zero = "0".repeat(oid.len()); + let out = git.run(&Spec { + repo: Some(dir), + args: &["update-ref", &r, oid, expected.unwrap_or(&zero)], + ..Default::default() + })?; + Ok(out.ok()) +} + +/// Retain `oid` for every open PR whose head branch is `branch` (called with the exclusive +/// lifecycle lock held, just before the branch is deleted). +pub fn retain_for_branch( + conn: &Connection, + git: &Git, + dir: &Path, + repo_id: i64, + branch: &str, + oid: &str, +) -> Result<(), ComputeError> { + for p in open_pulls(conn, repo_id)? { + if p.head_ref == branch { + retain(git, dir, p.number, oid)?; + } + } + Ok(()) +} + +/// The tip of branch `name` in a `refs/heads` listing. Exact: on a case-sensitive filesystem +/// `feature` and `Feature` are two branches. (On a case-insensitive development filesystem a PR +/// opened with a differently cased name reads as vanished; production is Linux.) +fn head_tip(heads: &HashMap, name: &str) -> Option { + heads.get(&format!("refs/heads/{name}")).cloned() +} + +/// Does an open PR of the repository have a head branch that is gone without the PR knowing +/// (not reopened that way)? Reads use it to converge after a push that deleted a head, whose +/// own sync may not have run yet. +pub fn has_vanished_head(conn: &Connection, git: &Git, dir: &Path, repo_id: i64) -> bool { + let Ok(open) = open_pulls(conn, repo_id) else { + return false; + }; + let watched: Vec<&Pull> = open.iter().filter(|p| !p.head_gone).collect(); + if watched.is_empty() { + return false; + } + match refs_under(git, dir, "refs/heads") { + Ok(heads) => watched + .iter() + .any(|p| head_tip(&heads, &p.head_ref).is_none()), + Err(_) => false, + } +} + +/// [`sync_locked`] for one PR; `true` when it closed it. +fn sync_one( + conn: &Connection, + git: &Git, + dir: &Path, + mut p: Pull, + kept: &HashMap, + heads: &HashMap, + now: i64, +) -> Result { + let tip = head_tip(heads, &p.head_ref); + let have = kept.get(&retention_ref(p.number)).cloned(); + match tip { + Some(t) => { + if have.as_deref() != Some(t.as_str()) { + retain_cas(git, dir, p.number, &t, have.as_deref())?; + } + if p.head_sha != t || p.head_gone { + p.head_sha = t; + p.head_gone = false; + save(conn, &p)?; + } + Ok(false) + } + // Reopened without its branch: stays open on the retained head. + None if p.head_gone => Ok(false), + None => { + // The branch is gone, so whoever deleted it has recorded its tip: read that now. + let head = refs_under(git, dir, &retention_ref(p.number))? + .remove(&retention_ref(p.number)) + .unwrap_or_else(|| p.head_sha.clone()); + retain(git, dir, p.number, &head)?; + p.head_sha = head.clone(); + p.final_head = Some(head); + p.state = "closed".into(); + p.closed = Some(now); + p.updated = now; + save(conn, &p)?; + Ok(true) + } + } +} + +/// Bring refs and rows into agreement, with the write gate held: an open PR's retention ref +/// follows its head branch; a PR whose head branch is gone keeps the retained head and is +/// closed. Returns how many PRs it closed. +pub fn sync_locked( + conn: &Connection, + git: &Git, + dir: &Path, + repo_id: i64, + now: i64, +) -> Result { + let open = open_pulls(conn, repo_id)?; + if open.is_empty() { + return Ok(0); + } + // The retention refs are read before the branches: a head deleted by a push in between has + // had its tip recorded by the hook already, and every write below is a compare-and-swap on + // what was read, so a stale snapshot can never overwrite a newer retained head. + let kept = refs_under(git, dir, "refs/forge/pull")?; + let heads = refs_under(git, dir, "refs/heads")?; + let mut closed = 0; + for p in open { + let number = p.number; + // One PR that cannot be synced (a git failure on its ref) must not keep the others from it. + match sync_one(conn, git, dir, p, &kept, &heads, now) { + Ok(c) => closed += usize::from(c), + Err(e) => eprintln!("sc-forge: could not sync pull request {number}: {e}"), + } + } + Ok(closed) +} + +/// The startup repair of one repository: [`sync_locked`], retention refs of closed and merged +/// PRs recreated from their stored final head, and `refs/forge/pull/*` refs with no PR row +/// removed (only that namespace's `head` refs). +pub fn reconcile_repo( + conn: &Connection, + git: &Git, + dir: &Path, + repo_id: i64, + now: i64, +) -> Result<(), ComputeError> { + sync_locked(conn, git, dir, repo_id, now)?; + let kept = refs_under(git, dir, "refs/forge/pull")?; + let mut st = + conn.prepare("SELECT number, state, final_head_sha FROM pulls WHERE repo_id = ?1")?; + let rows: Vec<(i64, String, Option)> = st + .query_map([repo_id], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)))? + .collect::>()?; + let known: HashMap)> = + rows.into_iter().map(|(n, s, h)| (n, (s, h))).collect(); + for p in open_pulls(conn, repo_id)? { + // Reopened without its branch: the stored head is the only anchor left. + if p.head_gone && kept.get(&retention_ref(p.number)) != Some(&p.head_sha) { + retain(git, dir, p.number, &p.head_sha)?; + } + } + for (n, (state, final_head)) in &known { + if state != "open" { + if let Some(h) = final_head { + if kept.get(&retention_ref(*n)) != Some(h) { + retain(git, dir, *n, h)?; + } + } + } + } + for name in kept.keys() { + let n = name + .strip_prefix("refs/forge/pull/") + .and_then(|r| r.strip_suffix("/head")) + .and_then(|n| n.parse::().ok()); + if n.is_some_and(|n| !known.contains_key(&n)) { + let _ = git.run(&Spec { + repo: Some(dir), + args: &["update-ref", "-d", name], + ..Default::default() + }); + } + } + Ok(()) +} + +/// Run [`sync_locked`] under the repository's write gate (for callers that do not hold it, such +/// as the git transport after an accepted push). Failures are logged: the next sync repairs them. +pub fn sync(life: &Lifecycle, conn: &Connection, repo_id: i64, now: i64) { + match life.with_write_row(conn, repo_id, Duration::from_secs(10), |_| { + sync_locked(conn, &life.git, &life.repo_dir(repo_id), repo_id, now) + }) { + Ok(WriteRun::Ran(Ok(_))) | Ok(WriteRun::Gone) => {} + Ok(WriteRun::Ran(Err(e))) => { + eprintln!("sc-forge: pull request sync of repository {repo_id} failed: {e}") + } + Ok(WriteRun::Busy) => { + eprintln!( + "sc-forge: pull request sync of repository {repo_id} skipped: repository busy" + ) + } + Err(e) => eprintln!("sc-forge: pull request sync of repository {repo_id} failed: {e}"), + } +} + +/// Startup: repair every live repository that has pull requests (before the listener is bound, +/// so nothing else is running). +pub fn reconcile_all(life: &Lifecycle, conn: &Connection, now: i64) { + // Every live repository, not only those with pull requests: an orphan ref from a crash during + // a repository's first PR creation has no row to find it by. + let repos = match crate::repos::list_in_state(conn, "live") { + Ok(v) => v, + Err(e) => { + eprintln!("sc-forge: pull request reconcile could not list repositories: {e}"); + return; + } + }; + for r in repos { + if let Err(e) = reconcile_repo(conn, &life.git, &life.repo_dir(r.id), r.id, now) { + eprintln!( + "sc-forge: pull request reconcile of repository {} failed: {e}", + r.id + ); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn conn() -> (Connection, std::path::PathBuf, i64, i64) { + let d = std::env::temp_dir().join(format!( + "scl-forge-pulls-{}-{}", + std::process::id(), + crate::timefmt::now() + )); + let _ = std::fs::remove_dir_all(&d); + let conn = crate::db::open_for_admin(&d).unwrap(); + let owner = crate::store::create_account( + &conn, + &crate::store::NewAccount { + login: "o".into(), + visibility: "public".into(), + ..Default::default() + }, + 1, + ) + .unwrap(); + let repo = crate::repos::insert_creating(&conn, owner.id, "r", "main", "", "", 1).unwrap(); + (conn, d, repo.id, owner.id) + } + + #[test] + fn numbers_are_sequential_and_one_open_pull_per_target_pair() { + let (c, d, repo, user) = conn(); + let a = insert(&c, repo, "f", "main", "b1", "h1", "t", "", user, 5).unwrap(); + let b = insert(&c, repo, "g", "main", "b1", "h2", "t", "", user, 6).unwrap(); + assert_eq!((a.number, b.number), (1, 2)); + assert!( + insert(&c, repo, "f", "main", "b1", "h3", "t", "", user, 7).is_err(), + "the partial unique index refuses a second open PR with the same targets" + ); + let mut a2 = a.clone(); + a2.state = "closed".into(); + save(&c, &a2).unwrap(); + assert!(insert(&c, repo, "f", "main", "b1", "h3", "t", "", user, 8).is_ok()); + let (rows, total) = list(&c, repo, &["open", "closed"], Sort::Newest, 0, 2).unwrap(); + assert_eq!((rows.len(), total), (2, 3)); + assert_eq!(rows[0].number, 3); + let (rows, _) = list(&c, repo, &["closed"], Sort::Oldest, 0, 10).unwrap(); + assert_eq!(rows.len(), 1); + std::fs::remove_dir_all(&d).unwrap(); + assert!(!d.exists()); + } +} diff --git a/crates/forge/src/pulls_api.rs b/crates/forge/src/pulls_api.rs new file mode 100644 index 00000000..d4599f41 --- /dev/null +++ b/crates/forge/src/pulls_api.rs @@ -0,0 +1,804 @@ +//! The pull-request routes: create, list, get (with `.diff` and `.patch`), PATCH, commits, files and +//! the is-merged probe. The merge itself is F4c. +//! +//! Locking follows `lifecycle`'s order: writes take the repository's write gate (and with it the +//! shared lifecycle lock); reads take the shared lock once (`read_locked`). Nothing here calls +//! `repo_json` (it locks); the repository object is built with `repo_json_known`. + +use crate::api::{body, page_params, paged_with, str_field, user_json}; +use crate::content_api::{api_base, commit_json, dir, git_err, read_err, web_base, Users}; +use crate::gitobj; +use crate::gitwrite as gw; +use crate::http::{error, not_found, Resp}; +use crate::lifecycle::WriteRun; +use crate::pulls::{self, Pull, Sort}; +use crate::refname; +use crate::repo_api::{acc, read_locked, repo_json_known}; +use crate::repos::{Access, Perm}; +use crate::routes::Ctx; +use crate::store; +use crate::timefmt::{now, rfc3339}; +use serde_json::{json, Value}; +use std::time::Duration; + +const GATE_WAIT: Duration = Duration::from_secs(10); +const MAX_TITLE: usize = 255; +const MAX_BODY: usize = 65_536; +/// Changed files one `files` page may carry. +const MAX_FILES: usize = 3_000; + +/// A repository path for a URL: every segment percent-encoded, the separators kept. +fn enc_path(p: &str) -> String { + p.split('/') + .map(crate::api::pct_encode) + .collect::>() + .join("/") +} + +fn missing(ctx: &Ctx) -> Resp { + not_found(&ctx.app.base, "The target couldn't be found.", None) +} + +fn needs_write(ctx: &Ctx) -> Resp { + error( + &ctx.app.base, + 403, + "user should have a permission to write to a repo", + ) +} + +/// The PR number out of a path parameter (`12`, or `12.diff` / `12.patch` where allowed). +fn number(raw: &str) -> Option { + raw.parse::().ok().filter(|n| *n >= 1) +} + +fn tip(ctx: &Ctx, a: &Access, branch: &str) -> Result, Resp> { + if !refname::valid_branch(branch) { + return Ok(None); + } + gw::read_ref( + &ctx.app.life.git, + &dir(ctx, a), + &refname::branch_ref(branch), + ) + .map_err(|e| read_err(ctx, a, e)) +} + +/// What the PR object shows now, as opposed to what is stored. +struct Live { + base_sha: String, + head_sha: String, + head_ref: String, + mergeable: bool, + merge_base: Option, + changed_files: i64, + additions: i64, + deletions: i64, +} + +/// The current facts of `p`: live tips while the branches exist, retained or frozen values after. +fn live(ctx: &Ctx, a: &Access, p: &Pull) -> Result { + if p.state == "merged" { + return Ok(Live { + base_sha: p.merge_commit.clone().unwrap_or_else(|| p.base_sha.clone()), + head_sha: p.final_head.clone().unwrap_or_else(|| p.head_sha.clone()), + head_ref: p.head_ref.clone(), + mergeable: false, + merge_base: p.merge_base.clone(), + changed_files: p.changed_files.unwrap_or(0), + additions: p.additions.unwrap_or(0), + deletions: p.deletions.unwrap_or(0), + }); + } + let base_sha = tip(ctx, a, &p.base_ref)?.unwrap_or_else(|| p.base_sha.clone()); + let kept = || -> Result { + let kept = gw::read_ref( + &ctx.app.life.git, + &dir(ctx, a), + &pulls::retention_ref(p.number), + ) + .map_err(|e| read_err(ctx, a, e))?; + Ok(kept + .or_else(|| p.final_head.clone()) + .unwrap_or_else(|| p.head_sha.clone())) + }; + let branch = tip(ctx, a, &p.head_ref)?; + let (head_sha, head_ref) = match (&p.state[..], branch) { + // A closed PR keeps the head it was closed with, however its branch moves afterwards. + ("closed", b) => ( + p.final_head.clone().map_or_else(kept, Ok)?, + if b.is_some() { + p.head_ref.clone() + } else { + format!("refs/pull/{}/head", p.number) + }, + ), + (_, Some(t)) => (t, p.head_ref.clone()), + (_, None) => (kept()?, format!("refs/pull/{}/head", p.number)), + }; + let c = pulls::compute( + ctx.conn, + &ctx.app.life.git, + &dir(ctx, a), + a.repo.id, + &base_sha, + &head_sha, + ) + .map_err(|e| match e { + pulls::ComputeError::Db(e) => ctx.app.internal(&e), + pulls::ComputeError::Git(e) => read_err(ctx, a, e), + })?; + Ok(Live { + base_sha, + head_sha, + head_ref, + // Recorded: a closed PR keeps reading mergeable. + mergeable: p.state != "open" || c.mergeable, + merge_base: c.merge_base, + changed_files: c.changed_files, + additions: c.additions, + deletions: c.deletions, + }) +} + +fn pull_json( + ctx: &Ctx, + a: &Access, + p: &Pull, + l: &Live, + repo: &Value, + users: &mut std::collections::HashMap, +) -> Result { + let mut user = |id: i64| -> Result { + if let Some(v) = users.get(&id) { + return Ok(v.clone()); + } + let u = store::find_by_id(ctx.conn, id) + .map_err(|e| ctx.app.internal(&e))? + .ok_or_else(|| ctx.app.internal(&"pull request without user"))?; + let v = user_json(&ctx.app.base, &u); + users.insert(id, v.clone()); + Ok(v) + }; + let web = format!("{}/pulls/{}", web_base(ctx, a), p.number); + let comments = + pulls::comment_count(ctx.conn, a.repo.id, p.number).map_err(|e| ctx.app.internal(&e))?; + let merged = p.state == "merged"; + Ok(json!({ + "id": p.id, + "url": web, + "number": p.number, + "user": user(p.creator_id)?, + "title": p.title, + "body": p.body, + "labels": [], + "milestone": null, + "assignee": null, + "assignees": null, + "requested_reviewers": null, + "state": if p.state == "open" { "open" } else { "closed" }, + "draft": false, + "is_locked": false, + "comments": comments, + "review_comments": 0, + "additions": l.additions, + "deletions": l.deletions, + "changed_files": l.changed_files, + "html_url": web, + "diff_url": format!("{web}.diff"), + "patch_url": format!("{web}.patch"), + "mergeable": l.mergeable, + "merged": merged, + "merged_at": p.merged.filter(|_| merged).map(rfc3339), + "merge_commit_sha": if merged { json!(p.merge_commit) } else { Value::Null }, + "merged_by": match p.merged_by.filter(|_| merged) { Some(id) => user(id)?, None => Value::Null }, + "allow_maintainer_edit": false, + "base": {"label": p.base_ref, "ref": p.base_ref, "sha": l.base_sha, "repo_id": a.repo.id, "repo": repo}, + "head": {"label": p.head_ref, "ref": l.head_ref, "sha": l.head_sha, "repo_id": a.repo.id, "repo": repo}, + "merge_base": l.merge_base.clone().unwrap_or_default(), + "due_date": null, + "created_at": rfc3339(p.created), + "updated_at": rfc3339(p.updated), + "closed_at": p.closed.map(rfc3339), + "pin_order": 0, + })) +} + +fn repo_value(ctx: &Ctx, a: &Access) -> Result { + // A repository with a pull request has branches, so it is not empty; `repo_json` would take + // the shared lifecycle lock a second time. + repo_json_known(ctx.app, ctx.conn, &a.repo, a.perm, false) +} + +fn render(ctx: &Ctx, a: &Access, p: &Pull) -> Result { + let l = live(ctx, a, p)?; + pull_json(ctx, a, p, &l, &repo_value(ctx, a)?, &mut Default::default()) +} + +/// The head commit a review or comment refers to now. +pub(crate) fn head_of(ctx: &Ctx, a: &Access, p: &Pull) -> Result { + live(ctx, a, p).map(|l| l.head_sha) +} + +fn find(ctx: &Ctx, a: &Access, n: i64) -> Result { + match pulls::find(ctx.conn, a.repo.id, n) { + Ok(Some(p)) => Ok(p), + Ok(None) => Err(missing(ctx)), + Err(e) => Err(ctx.app.internal(&e)), + } +} + +/// Run `f` under the write gate. +fn gated(ctx: &Ctx, a: &Access, f: impl FnOnce() -> Resp) -> Resp { + match ctx + .app + .life + .with_write_row(ctx.conn, a.repo.id, GATE_WAIT, |_| f()) + { + Ok(WriteRun::Ran(r)) => r, + Ok(WriteRun::Busy) => error( + &ctx.app.base, + 503, + "this repository is being written to, try again", + ), + Ok(WriteRun::Gone) => missing(ctx), + Err(e) => ctx.app.internal(&e), + } +} + +/// Before a read: if a push deleted the head of an open PR and its own sync has not run yet, +/// run it now (outside any lifecycle lock; the sync takes the write gate). +fn converge(ctx: &Ctx, a: &Access) { + if pulls::has_vanished_head(ctx.conn, &ctx.app.life.git, &dir(ctx, a), a.repo.id) { + pulls::sync(&ctx.app.life, ctx.conn, a.repo.id, now()); + } +} + +/// Close the PRs whose head branch disappeared and refresh the retention refs, with the gate held. +pub(crate) fn sync_held(ctx: &Ctx, a: &Access) { + if let Err(e) = pulls::sync_locked(ctx.conn, &ctx.app.life.git, &dir(ctx, a), a.repo.id, now()) + { + eprintln!( + "sc-forge: pull request sync of repository {} failed: {e}", + a.repo.id + ); + } +} + +/// `owner:branch` or `branch` to the branch of this repository, or `None` for another repository. +fn local_branch(a: &Access, v: &str) -> Option { + match v.split_once(':') { + Some((owner, branch)) => owner + .eq_ignore_ascii_case(&a.repo.owner) + .then(|| branch.to_string()), + None => Some(v.to_string()), + } +} + +/// `POST /repos/{o}/{r}/pulls`. +pub fn create_pull(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + if a.perm < Perm::Write { + return needs_write(ctx); + } + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + let title = str_field(&b, "title") + .unwrap_or_default() + .trim() + .to_string(); + if title.is_empty() { + return error(&ctx.app.base, 422, "[Title]: Required"); + } + let text = str_field(&b, "body").unwrap_or_default(); + if title.chars().count() > MAX_TITLE || text.len() > MAX_BODY { + return error(&ctx.app.base, 422, "[Title/Body]: too long"); + } + let (Some(head), Some(base)) = (str_field(&b, "head"), str_field(&b, "base")) else { + return error(&ctx.app.base, 422, "[Head/Base]: Required"); + }; + // Forks are not supported: a head in another repository does not exist here. + let Some(head) = local_branch(&a, &head) else { + return missing(ctx); + }; + gated(ctx, &a, || { + let Some(head_tip) = (match tip(ctx, &a, &head) { + Ok(t) => t, + Err(r) => return r, + }) else { + return missing(ctx); + }; + let Some(base_tip) = (match tip(ctx, &a, &base) { + Ok(t) => t, + Err(r) => return r, + }) else { + return not_found(&ctx.app.base, "BaseNotExist", None); + }; + match pulls::find_open(ctx.conn, a.repo.id, &head, &base) { + Ok(Some(p)) => { + return error( + &ctx.app.base, + 409, + &format!( + "pull request already exists for these targets [id: {}, issue_id: {}, head_repo_id: {}, base_repo_id: {}, head_branch: {head}, base_branch: {base}]", + p.id, p.id, a.repo.id, a.repo.id + ), + ) + } + Ok(None) => {} + Err(e) => return ctx.app.internal(&e), + } + if head_tip == base_tip { + return error( + &ctx.app.base, + 422, + "Invalid PullRequest: There are no changes between the head and the base", + ); + } + let t = now(); + // The retention ref comes first: a crash leaves at worst an orphan ref (the startup + // reconcile removes it), never a PR whose head could be pruned. Numbers are only handed + // out under the write gate, so the next one is known. + let number = match pulls::next_number(ctx.conn, a.repo.id) { + Ok(n) => n, + Err(e) => return ctx.app.internal(&e), + }; + if let Err(e) = pulls::retain(&ctx.app.life.git, &dir(ctx, &a), number, &head_tip) { + return read_err(ctx, &a, e); + } + let p = match pulls::insert( + ctx.conn, + a.repo.id, + &head, + &base, + &base_tip, + &head_tip, + &title, + &text, + ctx.who.user.id, + t, + ) { + Ok(p) => p, + Err(e) => return ctx.app.internal(&e), + }; + match render(ctx, &a, &p) { + Ok(v) => Resp::json(201, &v), + Err(r) => r, + } + }) +} + +/// `GET /repos/{o}/{r}/pulls`. +pub fn list_pulls(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + converge(ctx, &a); + read_locked(ctx, &a, |ctx| list_inner(ctx, &a)) +} + +fn list_inner(ctx: &Ctx, a: &Access) -> Resp { + // Recorded (D#13): Gitea answers 404 for the pull requests of a repository with no commits. + match crate::gitread::is_empty(&ctx.app.life.git, &dir(ctx, a)) { + Ok(true) => return missing(ctx), + Ok(false) => {} + Err(e) => return read_err(ctx, a, e), + } + let (state, sort) = (ctx.req.param("state"), ctx.req.param("sort")); + let states: &[&str] = match state { + None | Some("open") | Some("") => &["open"], + Some("closed") => &["closed", "merged"], + Some("all") => &["open", "closed", "merged"], + Some(_) => &["open"], + }; + let order = match sort { + Some("oldest") => Sort::Oldest, + Some("recentupdate") => Sort::RecentUpdate, + _ => Sort::Newest, + }; + let (page, limit) = page_params(ctx); + let (rows, total) = match pulls::list( + ctx.conn, + a.repo.id, + states, + order, + (page - 1) * limit, + limit, + ) { + Ok(x) => x, + Err(e) => return ctx.app.internal(&e), + }; + let repo = match repo_value(ctx, a) { + Ok(v) => v, + Err(r) => return r, + }; + let mut users = Default::default(); + let mut items = Vec::with_capacity(rows.len()); + for p in &rows { + let l = match live(ctx, a, p) { + Ok(l) => l, + Err(r) => return r, + }; + match pull_json(ctx, a, p, &l, &repo, &mut users) { + Ok(v) => items.push(v), + Err(r) => return r, + } + } + paged_with( + ctx, + &format!("/repos/{}/{}/pulls", a.repo.owner, a.repo.name), + total, + page, + limit, + items, + ) +} + +fn text_response(body: Vec) -> Resp { + Resp { + status: 200, + headers: vec![("content-type", "text/plain;charset=utf-8".into())], + body, + delay: Duration::ZERO, + hold: None, + } +} + +/// `GET /repos/{o}/{r}/pulls/{n}` and its `.diff` / `.patch` forms. +pub fn get_pull(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let raw = ctx.param("n").to_string(); + let (num, ext) = match raw.rsplit_once('.') { + Some((n, e)) if matches!(e, "diff" | "patch") => (n.to_string(), Some(e.to_string())), + _ => (raw, None), + }; + let Some(n) = number(&num) else { + return missing(ctx); + }; + converge(ctx, &a); + read_locked(ctx, &a, |ctx| { + let p = match find(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + match ext.as_deref() { + None => match render(ctx, &a, &p) { + Ok(v) => Resp::json(200, &v), + Err(r) => r, + }, + Some(e) => { + let l = match live(ctx, &a, &p) { + Ok(l) => l, + Err(r) => return r, + }; + let (git, d) = (&ctx.app.life.git, dir(ctx, &a)); + let out = if e == "diff" { + gitobj::diff_text(git, &d, l.merge_base.as_deref(), &l.head_sha) + } else { + gitobj::range_patch(git, &d, l.merge_base.as_deref(), &l.head_sha) + }; + match git_err(ctx, &a, out) { + Ok(bytes) => text_response(bytes), + Err(r) => r, + } + } + } + }) +} + +/// `PATCH /repos/{o}/{r}/pulls/{n}`: title, body, state, base. Answers 201 (recorded). +pub fn patch_pull(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + gated(ctx, &a, || { + let mut p = match find(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + if a.perm < Perm::Write && p.creator_id != ctx.who.user.id { + return needs_write(ctx); + } + if let Some(t) = str_field(&b, "title") { + let t = t.trim().to_string(); + if t.is_empty() || t.chars().count() > MAX_TITLE { + return error( + &ctx.app.base, + 422, + "[Title]: Required, at most 255 characters", + ); + } + p.title = t; + } + if let Some(t) = str_field(&b, "body") { + if t.len() > MAX_BODY { + return error(&ctx.app.base, 422, "[Body]: too long"); + } + p.body = t; + } + if let Some(new_base) = str_field(&b, "base").filter(|s| !s.is_empty()) { + if p.state == "merged" { + return error(&ctx.app.base, 422, "the pull request is already merged"); + } + if new_base != p.base_ref { + let Some(t) = (match tip(ctx, &a, &new_base) { + Ok(t) => t, + Err(r) => return r, + }) else { + return not_found(&ctx.app.base, "BaseNotExist", None); + }; + if new_base == p.head_ref { + return error(&ctx.app.base, 422, "the base cannot be the head branch"); + } + if p.state == "open" + && matches!( + pulls::find_open(ctx.conn, a.repo.id, &p.head_ref, &new_base), + Ok(Some(_)) + ) + { + return error( + &ctx.app.base, + 409, + "pull request already exists for these targets", + ); + } + p.base_ref = new_base; + p.base_sha = t; + } + } + let t = now(); + match str_field(&b, "state").as_deref() { + None | Some("") => {} + Some("closed") if p.state == "open" => { + let head = match tip(ctx, &a, &p.head_ref) { + Ok(Some(h)) => h, + Ok(None) => gw::read_ref( + &ctx.app.life.git, + &dir(ctx, &a), + &pulls::retention_ref(p.number), + ) + .ok() + .flatten() + .unwrap_or_else(|| p.head_sha.clone()), + Err(r) => return r, + }; + if let Err(e) = pulls::retain(&ctx.app.life.git, &dir(ctx, &a), p.number, &head) { + return read_err(ctx, &a, e); + } + p.head_sha = head.clone(); + p.final_head = Some(head); + p.state = "closed".into(); + p.closed = Some(t); + } + Some("open") if p.state == "closed" => { + match pulls::find_open(ctx.conn, a.repo.id, &p.head_ref, &p.base_ref) { + Ok(None) => {} + Ok(Some(_)) => { + return error( + &ctx.app.base, + 409, + "pull request already exists for these targets", + ) + } + Err(e) => return ctx.app.internal(&e), + } + // Recorded: reopening works even when the head branch is gone. + p.head_gone = !matches!(tip(ctx, &a, &p.head_ref), Ok(Some(_))); + p.state = "open".into(); + p.closed = None; + p.final_head = None; + } + Some("open") | Some("closed") if p.state == "merged" => { + return error(&ctx.app.base, 422, "the pull request is already merged") + } + Some("open") | Some("closed") => {} + Some(_) => return error(&ctx.app.base, 422, "[State]: must be open or closed"), + } + p.updated = t; + if let Err(e) = pulls::save(ctx.conn, &p) { + return match e { + rusqlite::Error::SqliteFailure(f, _) + if f.code == rusqlite::ErrorCode::ConstraintViolation => + { + error( + &ctx.app.base, + 409, + "pull request already exists for these targets", + ) + } + e => ctx.app.internal(&e), + }; + } + // A reopened PR follows its branch again: bring its retention ref along (gate held). + sync_held(ctx, &a); + let p = match find(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + match render(ctx, &a, &p) { + Ok(v) => Resp::json(201, &v), + Err(r) => r, + } + }) +} + +/// `GET /repos/{o}/{r}/pulls/{n}/merge`: 204 once merged, 404 otherwise. +pub fn is_merged(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + match find(ctx, &a, n) { + Ok(p) if p.state == "merged" => Resp::no_content(), + Ok(_) => missing(ctx), + Err(r) => r, + } +} + +/// `GET /repos/{o}/{r}/pulls/{n}/commits`: `merge_base..head`, newest first, with files and stats. +pub fn pull_commits(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + converge(ctx, &a); + read_locked(ctx, &a, |ctx| { + let p = match find(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + let l = match live(ctx, &a, &p) { + Ok(l) => l, + Err(r) => return r, + }; + let (git, d) = (&ctx.app.life.git, dir(ctx, &a)); + let (page, limit) = page_params(ctx); + let mb = l.merge_base.as_deref(); + let total = match git_err(ctx, &a, gitobj::range_count(git, &d, mb, &l.head_sha)) { + Ok(t) => t as i64, + Err(r) => return r, + }; + let commits = match git_err( + ctx, + &a, + gitobj::range_log( + git, + &d, + mb, + &l.head_sha, + ((page - 1) * limit) as usize, + limit as usize, + ), + ) { + Ok(c) => c, + Err(r) => return r, + }; + let mut users = Users::new(ctx); + let mut items = Vec::new(); + // The per-command output cap does not bound a page of up to 50 commits that each list a huge + // change set, so the files kept for one response are bounded as a whole. + let mut files_left = crate::content_api::MAX_PAGE_FILES; + for c in &commits { + let (files, stats) = match git_err( + ctx, + &a, + gitobj::changes(git, &d, c.parents.first().map(String::as_str), &c.oid), + ) { + Ok(x) => x, + Err(r) => return r, + }; + if files.len() > files_left { + return error( + &ctx.app.base, + 413, + "the changed files of this page are too many", + ); + } + files_left -= files.len(); + match commit_json( + ctx, + &a, + &mut users, + c, + true, + Some((Some(files), Some(stats))), + ) { + Ok(v) => items.push(v), + Err(r) => return r, + } + } + paged_with( + ctx, + &format!("/repos/{}/{}/pulls/{n}/commits", a.repo.owner, a.repo.name), + total, + page, + limit, + items, + ) + }) +} + +/// `GET /repos/{o}/{r}/pulls/{n}/files`: the files `merge_base...head` changes. +pub fn pull_files(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + converge(ctx, &a); + read_locked(ctx, &a, |ctx| { + let p = match find(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + let l = match live(ctx, &a, &p) { + Ok(l) => l, + Err(r) => return r, + }; + let all = match git_err( + ctx, + &a, + gitobj::file_stats( + &ctx.app.life.git, + &dir(ctx, &a), + l.merge_base.as_deref(), + &l.head_sha, + ), + ) { + Ok(f) => f, + Err(r) => return r, + }; + let (page, limit) = page_params(ctx); + let (api, web) = (api_base(ctx, &a), web_base(ctx, &a)); + let items: Vec = all + .iter() + .skip(((page - 1) * limit) as usize) + .take((limit as usize).min(MAX_FILES)) + .map(|f| { + json!({ + "filename": f.name, + "status": f.status, + "additions": f.additions, + "deletions": f.deletions, + "changes": f.additions + f.deletions, + "html_url": format!("{web}/src/commit/{}/{}", l.head_sha, enc_path(&f.name)), + "contents_url": format!("{api}/contents/{}?ref={}", enc_path(&f.name), l.head_sha), + "raw_url": format!("{web}/raw/commit/{}/{}", l.head_sha, enc_path(&f.name)), + }) + }) + .collect(); + paged_with( + ctx, + &format!("/repos/{}/{}/pulls/{n}/files", a.repo.owner, a.repo.name), + all.len() as i64, + page, + limit, + items, + ) + }) +} diff --git a/crates/forge/src/repo_api.rs b/crates/forge/src/repo_api.rs index 54bafaf0..fc250d57 100644 --- a/crates/forge/src/repo_api.rs +++ b/crates/forge/src/repo_api.rs @@ -96,7 +96,7 @@ pub(crate) fn repo_json( /// The ~70-key repository object recorded from Gitea, with `empty` already known (a create or /// PATCH that has committed must not be reported as failed by a later git read). `perm` is the /// *caller's* standing. -fn repo_json_known( +pub(crate) fn repo_json_known( app: &App, conn: &rusqlite::Connection, r: &Repo, @@ -467,7 +467,7 @@ pub fn search(ctx: &mut Ctx) -> Resp { Ok(i) => i, Err(r) => return r, }; - let links = paged_with(ctx, "/repos/search", total, page, limit, &extra, vec![]); + let links = paged_with(ctx, "/repos/search", total, page, limit, vec![]); let mut r = Resp::json(200, &json!({"ok": true, "data": items})) .with("x-total-count", total.to_string()); if let Some((_, link)) = links.headers.iter().find(|(k, _)| *k == "link") { @@ -764,7 +764,11 @@ pub fn delete_branch(ctx: &mut Ctx) -> Resp { return error(&ctx.app.base, 403, "can not delete default branch"); } match ctx.app.life.delete_branch(ctx.conn, &a.repo, &name) { - Ok(()) => Resp::no_content(), + Ok(()) => { + // Open PRs on the branch close (the head was retained before the delete). + crate::pulls::sync(&ctx.app.life, ctx.conn, a.repo.id, now()); + Resp::no_content() + } Err(LifecycleError::NoSuchBranch) => no_branch(ctx, &name), Err(e) => life_err(ctx.app, e), } diff --git a/crates/forge/src/reviews.rs b/crates/forge/src/reviews.rs new file mode 100644 index 00000000..38fd797e --- /dev/null +++ b/crates/forge/src/reviews.rs @@ -0,0 +1,419 @@ +//! Reviews of a pull request and issue comments (an issue number is the PR number). +//! +//! Recorded Gitea behaviour kept: an unknown review `event` is stored as `PENDING`; approving or +//! requesting changes on your own PR is 422; a reviewer without write access can approve but the +//! review is not `official`; a reviewer's later approval or change request dismisses their +//! earlier ones; a `PENDING` review is visible only to its author, yet still counted in +//! `X-Total-Count`. Inline review comments are not supported (a non-empty `comments` is 422, never +//! silently dropped), so a review's comment list is always empty. + +use crate::api::{body, nested_user_json, page_params, paged, str_field, user_json}; +use crate::content_api::web_base; +use crate::http::{error, not_found, Resp}; +use crate::pulls; +use crate::repo_api::acc; +use crate::repos::{Access, Perm}; +use crate::routes::Ctx; +use crate::store; +use crate::timefmt::{now, rfc3339}; +use rusqlite::{params, Connection}; +use serde_json::{json, Value}; + +const MAX_TEXT: usize = 65_536; +/// Reviews per PR and comments per PR; a bound on what one repository can pile up. +const MAX_REVIEWS: i64 = 1_000; +const MAX_COMMENTS: i64 = 5_000; + +struct Review { + id: i64, + reviewer: i64, + state: String, + body: String, + commit_id: String, + official: bool, + created: i64, +} + +fn missing(ctx: &Ctx) -> Resp { + not_found(&ctx.app.base, "The target couldn't be found.", None) +} + +fn number(raw: &str) -> Option { + raw.parse::().ok().filter(|n| *n >= 1) +} + +fn all_reviews(conn: &Connection, repo_id: i64, n: i64) -> rusqlite::Result> { + let mut st = conn.prepare( + "SELECT id, reviewer_id, state, body, commit_id, official, created_unix FROM reviews \ + WHERE repo_id = ?1 AND pull_number = ?2 ORDER BY id", + )?; + let rows = st + .query_map(params![repo_id, n], |r| { + Ok(Review { + id: r.get(0)?, + reviewer: r.get(1)?, + state: r.get(2)?, + body: r.get(3)?, + commit_id: r.get(4)?, + official: r.get::<_, i64>(5)? != 0, + created: r.get(6)?, + }) + })? + .collect(); + rows +} + +fn decides(state: &str) -> bool { + matches!(state, "APPROVED" | "REQUEST_CHANGES") +} + +/// `created` is the POST answer, which (recorded) shows the full account; later reads show the nested form. +fn review_json( + ctx: &Ctx, + a: &Access, + n: i64, + r: &Review, + all: &[Review], + created: bool, +) -> Result { + let u = store::find_by_id(ctx.conn, r.reviewer) + .map_err(|e| ctx.app.internal(&e))? + .ok_or_else(|| ctx.app.internal(&"review without reviewer"))?; + // Superseded by a later decision of the same reviewer. + let dismissed = decides(&r.state) + && all + .iter() + .any(|o| o.id > r.id && o.reviewer == r.reviewer && decides(&o.state)); + let web = format!("{}/pulls/{n}", web_base(ctx, a)); + Ok(json!({ + "id": r.id, + "user": if created { user_json(&ctx.app.base, &u) } else { nested_user_json(&ctx.app.base, &u, false) }, + "team": null, + "state": r.state, + "body": r.body, + "commit_id": r.commit_id, + "stale": false, + "official": r.official && !dismissed, + "dismissed": dismissed, + "comments_count": 0, + "submitted_at": rfc3339(r.created), + "updated_at": rfc3339(r.created), + "html_url": format!("{web}#issuecomment-{}", r.id), + "pull_request_url": web, + })) +} + +fn pull_of(ctx: &Ctx, a: &Access, n: i64) -> Result { + match pulls::find(ctx.conn, a.repo.id, n) { + Ok(Some(p)) => Ok(p), + Ok(None) => Err(missing(ctx)), + Err(e) => Err(ctx.app.internal(&e)), + } +} + +/// A review the caller may see: not somebody else's `PENDING` one. +fn visible(ctx: &Ctx, r: &Review) -> bool { + r.state != "PENDING" || r.reviewer == ctx.who.user.id +} + +/// `GET /repos/{o}/{r}/pulls/{n}/reviews`. +pub fn list_reviews(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + if let Err(r) = pull_of(ctx, &a, n) { + return r; + } + let all = match all_reviews(ctx.conn, a.repo.id, n) { + Ok(v) => v, + Err(e) => return ctx.app.internal(&e), + }; + let (page, limit) = page_params(ctx); + let mut items = Vec::new(); + for r in all + .iter() + .filter(|r| visible(ctx, r)) + .skip(((page - 1) * limit) as usize) + .take(limit as usize) + { + match review_json(ctx, &a, n, r, &all, false) { + Ok(v) => items.push(v), + Err(r) => return r, + } + } + // Recorded quirk: the total counts reviews the caller cannot see. + paged( + ctx, + &format!("/repos/{}/{}/pulls/{n}/reviews", a.repo.owner, a.repo.name), + all.len() as i64, + page, + limit, + items, + ) +} + +/// The review the path names (and all the PR's reviews, which dismissal needs), if the caller may see it. +fn one(ctx: &Ctx, a: &Access) -> Result<(i64, usize, Vec), Resp> { + let (Some(n), Some(id)) = (number(ctx.param("n")), number(ctx.param("id"))) else { + return Err(missing(ctx)); + }; + pull_of(ctx, a, n)?; + let all = all_reviews(ctx.conn, a.repo.id, n).map_err(|e| ctx.app.internal(&e))?; + match all.iter().position(|r| r.id == id) { + Some(pos) if visible(ctx, &all[pos]) => Ok((n, pos, all)), + _ => Err(missing(ctx)), + } +} + +/// `GET /repos/{o}/{r}/pulls/{n}/reviews/{id}`. +pub fn get_review(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + match one(ctx, &a).and_then(|(n, pos, all)| review_json(ctx, &a, n, &all[pos], &all, false)) { + Ok(v) => Resp::json(200, &v), + Err(r) => r, + } +} + +/// `GET /repos/{o}/{r}/pulls/{n}/reviews/{id}/comments`: always empty (no inline comments). +pub fn review_comments(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + match one(ctx, &a) { + Ok(_) => Resp::json(200, &json!([])), + Err(r) => r, + } +} + +/// `POST /repos/{o}/{r}/pulls/{n}/reviews`. +pub fn create_review(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + let p = match pull_of(ctx, &a, n) { + Ok(p) => p, + Err(r) => return r, + }; + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + if b.get("comments") + .is_some_and(|c| c.as_array().is_none_or(|a| !a.is_empty()) && !c.is_null()) + { + return error( + &ctx.app.base, + 422, + "inline review comments are not supported", + ); + } + let text = str_field(&b, "body").unwrap_or_default(); + if text.len() > MAX_TEXT { + return error(&ctx.app.base, 422, "[Body]: too long"); + } + // Anything but the three known events is a pending review (recorded). + let state = match str_field(&b, "event") + .unwrap_or_default() + .to_uppercase() + .as_str() + { + "APPROVED" | "APPROVE" => "APPROVED", + "REQUEST_CHANGES" => "REQUEST_CHANGES", + "COMMENT" => "COMMENT", + _ => "PENDING", + }; + let me = ctx.who.user.id; + if p.creator_id == me && decides(state) { + return error( + &ctx.app.base, + 422, + if state == "APPROVED" { + "approve your own pull is not allowed" + } else { + "reject your own pull is not allowed" + }, + ); + } + match all_reviews(ctx.conn, a.repo.id, n) { + Ok(v) if v.len() as i64 >= MAX_REVIEWS => { + return error(&ctx.app.base, 422, "this pull request has too many reviews") + } + Ok(_) => {} + Err(e) => return ctx.app.internal(&e), + } + // Git runs under the shared lifecycle lock, so a delete in flight answers 404, not a git error. + let commit_id = match ctx.app.life.with_read(ctx.conn, a.repo.id, || { + crate::pulls_api::head_of(ctx, &a, &p) + }) { + Ok(Some(Ok(h))) => h, + Ok(Some(Err(r))) => return r, + Ok(None) => return missing(ctx), + Err(e) => return ctx.app.internal(&e), + }; + let official = a.perm >= Perm::Write && decides(state); + let t = now(); + if let Err(e) = ctx.conn.execute( + "INSERT INTO reviews (repo_id, pull_number, reviewer_id, state, body, commit_id, official, created_unix) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", + params![a.repo.id, n, me, state, text, commit_id, official, t], + ) { + return ctx.app.internal(&e); + } + let id = ctx.conn.last_insert_rowid(); + let all = match all_reviews(ctx.conn, a.repo.id, n) { + Ok(v) => v, + Err(e) => return ctx.app.internal(&e), + }; + let Some(r) = all.iter().find(|r| r.id == id) else { + return ctx.app.internal(&"review vanished"); + }; + match review_json(ctx, &a, n, r, &all, true) { + Ok(v) => Resp::json(200, &v), + Err(r) => r, + } +} + +// ---- issue comments ---- + +fn comment_json( + ctx: &Ctx, + a: &Access, + n: i64, + id: i64, + author: i64, + text: &str, + created: i64, + updated: i64, +) -> Result { + let u = store::find_by_id(ctx.conn, author) + .map_err(|e| ctx.app.internal(&e))? + .ok_or_else(|| ctx.app.internal(&"comment without author"))?; + let web = format!("{}/pulls/{n}", web_base(ctx, a)); + Ok(json!({ + "id": id, + "html_url": format!("{web}#issuecomment-{id}"), + "pull_request_url": web, + "issue_url": "", + "user": nested_user_json(&ctx.app.base, &u, true), + "original_author": "", + "original_author_id": 0, + "body": text, + "assets": [], + "created_at": rfc3339(created), + "updated_at": rfc3339(updated), + })) +} + +/// `GET /repos/{o}/{r}/issues/{n}/comments`: oldest first. +pub fn list_comments(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + if let Err(r) = pull_of(ctx, &a, n) { + return r; + } + let (page, limit) = page_params(ctx); + let total = match pulls::comment_count(ctx.conn, a.repo.id, n) { + Ok(t) => t, + Err(e) => return ctx.app.internal(&e), + }; + type Row = (i64, i64, String, i64, i64); + let rows: Result, _> = ctx + .conn + .prepare( + "SELECT id, author_id, body, created_unix, updated_unix FROM issue_comments \ + WHERE repo_id = ?1 AND number = ?2 ORDER BY id LIMIT ?3 OFFSET ?4", + ) + .and_then(|mut s| { + s.query_map(params![a.repo.id, n, limit, (page - 1) * limit], |r| { + Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?, r.get(4)?)) + })? + .collect() + }); + let rows = match rows { + Ok(r) => r, + Err(e) => return ctx.app.internal(&e), + }; + let mut items = Vec::new(); + for (id, author, text, c, u) in rows { + match comment_json(ctx, &a, n, id, author, &text, c, u) { + Ok(v) => items.push(v), + Err(r) => return r, + } + } + paged( + ctx, + &format!( + "/repos/{}/{}/issues/{n}/comments", + a.repo.owner, a.repo.name + ), + total, + page, + limit, + items, + ) +} + +/// `POST /repos/{o}/{r}/issues/{n}/comments`. +pub fn create_comment(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let Some(n) = number(ctx.param("n")) else { + return missing(ctx); + }; + if let Err(r) = pull_of(ctx, &a, n) { + return r; + } + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + let text = str_field(&b, "body").unwrap_or_default(); + if text.trim().is_empty() { + return error(&ctx.app.base, 422, "[Body]: Required"); + } + if text.len() > MAX_TEXT { + return error(&ctx.app.base, 422, "[Body]: too long"); + } + match pulls::comment_count(ctx.conn, a.repo.id, n) { + Ok(c) if c >= MAX_COMMENTS => { + return error( + &ctx.app.base, + 422, + "this pull request has too many comments", + ) + } + Ok(_) => {} + Err(e) => return ctx.app.internal(&e), + } + let t = now(); + if let Err(e) = ctx.conn.execute( + "INSERT INTO issue_comments (repo_id, number, author_id, body, created_unix, updated_unix) VALUES (?1, ?2, ?3, ?4, ?5, ?5)", + params![a.repo.id, n, ctx.who.user.id, text, t], + ) { + return ctx.app.internal(&e); + } + let id = ctx.conn.last_insert_rowid(); + match comment_json(ctx, &a, n, id, ctx.who.user.id, &text, t, t) { + Ok(v) => Resp::json(201, &v), + Err(r) => r, + } +} diff --git a/crates/forge/src/routes.rs b/crates/forge/src/routes.rs index c8373856..7cee9a5a 100644 --- a/crates/forge/src/routes.rs +++ b/crates/forge/src/routes.rs @@ -9,7 +9,9 @@ use crate::lifecycle::Lifecycle; use crate::limits::{Backoff, Gate, Proxies}; use crate::repos::{self, Access, Perm}; use crate::scope::{self, Cat, Level, Need}; -use crate::{api, content_api, content_write, db, protection, repo_api, statuses, store}; +use crate::{ + api, content_api, content_write, db, protection, pulls_api, repo_api, reviews, statuses, store, +}; use rusqlite::Connection; use std::path::PathBuf; @@ -524,6 +526,110 @@ pub static ROUTES: &[Route] = &[ Repo(Perm::Read), statuses::combined_status, ), + r( + "GET", + "/repos/{o}/{r}/pulls", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + pulls_api::list_pulls, + ), + r( + "POST", + "/repos/{o}/{r}/pulls", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + pulls_api::create_pull, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + pulls_api::get_pull, + ), + r( + "PATCH", + "/repos/{o}/{r}/pulls/{n}", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + pulls_api::patch_pull, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/merge", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + pulls_api::is_merged, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/commits", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + pulls_api::pull_commits, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/files", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + pulls_api::pull_files, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/reviews", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + reviews::list_reviews, + ), + r( + "POST", + "/repos/{o}/{r}/pulls/{n}/reviews", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + reviews::create_review, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/reviews/{id}", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + reviews::get_review, + ), + r( + "GET", + "/repos/{o}/{r}/pulls/{n}/reviews/{id}/comments", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + reviews::review_comments, + ), + r( + "GET", + "/repos/{o}/{r}/issues/{n}/comments", + Token, + n(Cat::Issue, Level::Read), + Repo(Perm::Read), + reviews::list_comments, + ), + r( + "POST", + "/repos/{o}/{r}/issues/{n}/comments", + Token, + n(Cat::Issue, Level::Write), + Repo(Perm::Read), + reviews::create_comment, + ), ]; /// Captured path parameters, in pattern order. diff --git a/crates/forge/src/statuses.rs b/crates/forge/src/statuses.rs index 4fe70efa..a1ba21ae 100644 --- a/crates/forge/src/statuses.rs +++ b/crates/forge/src/statuses.rs @@ -6,7 +6,7 @@ //! context (the oldest rows go), a departure from Gitea, which keeps everything. The merge gate //! reads [`latest`] directly, never the paged combined view. -use crate::api::{body, link_header, page_params, paged, user_json}; +use crate::api::{body, link_header, page_params, paged}; use crate::content_api::{api_base, dir}; use crate::gitobj::{self, Peel}; use crate::http::{error, not_found, Resp}; @@ -186,13 +186,7 @@ fn status_json( let u = store::find_by_id(ctx.conn, s.creator_id) .map_err(|e| ctx.app.internal(&e))? .ok_or_else(|| ctx.app.internal(&"status without creator"))?; - let mut j = user_json(&ctx.app.base, &u); - // Recorded: the nested creator is the public view of the account. The stored address is - // never shown (a placeholder derived from the login stands in), nor the site-admin flag. - j["active"] = json!(false); - j["is_admin"] = json!(false); - j["login_name"] = json!(""); - j["email"] = json!(format!("{}@noreply.localhost", u.login)); + let j = crate::api::nested_user_json(&ctx.app.base, &u, true); slot.insert(j); } Ok(json!({ @@ -208,6 +202,14 @@ fn status_json( })) } +/// The path of this request below `/api/v1`, as the client sent it: page links repeat it. +fn request_path<'a>(ctx: &'a Ctx) -> &'a str { + ctx.req + .path + .strip_prefix("/api/v1") + .unwrap_or(&ctx.req.path) +} + fn no_commit(ctx: &Ctx) -> Resp { not_found(&ctx.app.base, "The target couldn't be found.", None) } @@ -342,19 +344,7 @@ pub fn list_statuses(ctx: &mut Ctx) -> Resp { .map(|s| status_json(ctx, &a, s, &mut creators)) .collect(); match items { - Ok(items) => paged( - ctx, - &format!( - "/repos/{}/{}/commits/{}/statuses", - a.repo.owner, - a.repo.name, - crate::api::pct_encode(&rev) - ), - all.len() as i64, - page, - limit, - items, - ), + Ok(items) => paged(ctx, request_path(ctx), all.len() as i64, page, limit, items), Err(r) => r, } }) @@ -388,10 +378,15 @@ pub fn combined_status(ctx: &mut Ctx) -> Resp { }; let (page, limit) = page_params(ctx); let mut creators = HashMap::new(); + // Recorded (P#40-41): the page is cut from the newest end and then listed oldest first. let shown: Result, Resp> = all .iter() + .rev() .skip(((page - 1) * limit) as usize) .take(limit as usize) + .collect::>() + .into_iter() + .rev() .map(|s| status_json(ctx, &a, s, &mut creators)) .collect(); let shown = match shown { @@ -421,13 +416,8 @@ pub fn combined_status(ctx: &mut Ctx) -> Resp { if total > 0 { r = r.with("x-total-count", total.to_string()); } - let path = format!( - "/repos/{}/{}/commits/{}/status", - a.repo.owner, - a.repo.name, - crate::api::pct_encode(&rev) - ); - if let Some(l) = link_header(ctx, &path, total, page, limit, &[]) { + let path = request_path(ctx); + if let Some(l) = link_header(ctx, path, total, page, limit) { r = r.with("link", l); } r diff --git a/crates/forge/tests/common/mod.rs b/crates/forge/tests/common/mod.rs index a35781b1..ed79e0f7 100644 --- a/crates/forge/tests/common/mod.rs +++ b/crates/forge/tests/common/mod.rs @@ -45,6 +45,14 @@ impl Resp { pub fn msg(&self) -> String { self.json()["message"].as_str().unwrap_or("").to_string() } + /// A response header by lowercase name. + pub fn header(&self, name: &str) -> Option { + self.headers.get(name).cloned() + } + /// The body as text. + pub fn s_text(&self) -> String { + String::from_utf8_lossy(&self.raw).to_string() + } } pub fn b64(s: &str) -> String { @@ -272,6 +280,25 @@ impl Srv { out.text() } + /// Like [`Srv::git`], but `None` when git exits non-zero. + pub fn git_try(&self, id: i64, args: &[&str]) -> Option { + let g = scl_forge::gitproc::Git::new( + &self.dir, + scl_forge::gitproc::find_git().expect("git"), + 4, + ) + .unwrap(); + let dir = self.dir.join("repos").join(format!("{id}.git")); + let out = g + .run(&scl_forge::gitproc::Spec { + repo: Some(&dir), + args, + ..Default::default() + }) + .unwrap(); + out.ok().then(|| out.text()) + } + /// Make branch `name` in repository `id` point at a new empty commit on top of `parent` (or a root). pub fn commit_on(&self, id: i64, name: &str, parent: Option<&str>) -> String { let tree = self.git(id, &["mktree"], Some(b"")); diff --git a/crates/forge/tests/e2e_pulls.rs b/crates/forge/tests/e2e_pulls.rs new file mode 100644 index 00000000..69f57c16 --- /dev/null +++ b/crates/forge/tests/e2e_pulls.rs @@ -0,0 +1,774 @@ +//! F4b: pull requests, reviews and comments, with real git where refs matter. +//! Branches are made through the contents API (a commit per file) so each test reads as a story. +#![cfg(unix)] + +mod common; +use common::*; +use serde_json::{json, Value}; + +struct W { + s: Srv, + auth: String, + id: i64, +} + +fn world() -> W { + let s = Srv::start(); + let auth = token(&s.admin); + let r = s.post( + "/api/v1/user/repos", + &auth, + json!({"name": "w", "default_branch": "main", "auto_init": true}), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + let id = r.json()["id"].as_i64().unwrap(); + W { s, auth, id } +} + +impl W { + fn api(&self, method: &str, path: &str, body: Option) -> Resp { + self.as_(&self.auth, method, path, body) + } + fn as_(&self, auth: &str, method: &str, path: &str, body: Option) -> Resp { + self.s.send( + method, + &format!("/api/v1/repos/siteadmin/w{path}"), + Some(auth), + body.as_ref(), + ) + } + fn git(&self, args: &[&str]) -> String { + self.s.git(self.id, args, None) + } + /// One more commit on `branch` (created from `from` when `new`). + fn commit(&self, branch: &str, file: &str, new_from: Option<&str>) -> String { + let mut body = json!({"content": b64("x\n"), "message": format!("add {file}"), "branch": new_from.unwrap_or(branch)}); + if new_from.is_some() { + body["new_branch"] = json!(branch); + } + let r = self.api("POST", &format!("/contents/{file}"), Some(body)); + assert_eq!(r.status, 201, "{}", r.msg()); + self.git(&["rev-parse", &format!("refs/heads/{branch}")]) + } + fn open(&self, head: &str) -> Value { + let r = self.api( + "POST", + "/pulls", + Some(json!({"title": format!("PR {head}"), "body": "b", "head": head, "base": "main"})), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + r.json() + } + fn retained(&self, n: i64) -> Option { + self.s.git_try( + self.id, + &[ + "rev-parse", + "--verify", + "-q", + &format!("refs/forge/pull/{n}/head"), + ], + ) + } +} + +#[test] +fn create_validates_and_the_object_has_the_recorded_shape() { + let w = world(); + let f1 = w.commit("f1", "f1.txt", Some("main")); + let base = w.git(&["rev-parse", "refs/heads/main"]); + let pr = w.open("f1"); + assert_eq!(pr["number"], 1); + assert_eq!(pr["state"], "open"); + assert_eq!(pr["merged"], false); + assert_eq!(pr["mergeable"], true); + assert_eq!(pr["changed_files"], 1); + assert_eq!(pr["additions"], 1); + assert_eq!(pr["head"]["sha"], f1.as_str()); + assert_eq!(pr["base"]["sha"], base.as_str()); + assert_eq!(pr["merge_base"], base.as_str()); + assert_eq!(pr["head"]["repo"]["full_name"], "siteadmin/w"); + assert_eq!(pr["user"]["login"], "siteadmin"); + assert_eq!( + w.retained(1).as_deref(), + Some(f1.as_str()), + "the head is retained at creation" + ); + + for (name, body, status, text) in [ + ( + "duplicate", + json!({"title": "t", "head": "f1", "base": "main"}), + 409, + "pull request already exists", + ), + ( + "same branch", + json!({"title": "t", "head": "main", "base": "main"}), + 422, + "no changes between the head and the base", + ), + ( + "no head", + json!({"title": "t", "head": "nope", "base": "main"}), + 404, + "couldn't be found", + ), + ( + "no base", + json!({"title": "t", "head": "f1", "base": "nope"}), + 404, + "BaseNotExist", + ), + ( + "no title", + json!({"head": "f1", "base": "main"}), + 422, + "[Title]: Required", + ), + ( + "other repository", + json!({"title": "t", "head": "someone:f1", "base": "main"}), + 404, + "couldn't be found", + ), + ( + "bad branch", + json!({"title": "t", "head": "a..b", "base": "main"}), + 404, + "couldn't be found", + ), + ] { + let r = w.api("POST", "/pulls", Some(body)); + assert!( + r.status == status && r.msg().contains(text), + "{name}: {} {}", + r.status, + r.msg() + ); + } + // `owner:branch` naming this repository is the same branch + w.commit("f2", "f2.txt", Some("main")); + let r = w.api( + "POST", + "/pulls", + Some(json!({"title": "t", "head": "siteadmin:f2", "base": "main"})), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + assert_eq!(r.json()["number"], 2); +} + +#[test] +fn the_head_follows_the_branch_and_lists_are_ordered_and_paged() { + let w = world(); + w.commit("f1", "a.txt", Some("main")); + w.open("f1"); + let f1b = w.commit("f1", "b.txt", None); + let got = w.api("GET", "/pulls/1", None).json(); + assert_eq!(got["head"]["sha"], f1b.as_str(), "a push shows immediately"); + assert_eq!(got["changed_files"], 2); + assert_eq!( + w.retained(1).as_deref(), + Some(f1b.as_str()), + "and is retained" + ); + w.commit("f2", "c.txt", Some("main")); + w.open("f2"); + w.commit("f3", "d.txt", Some("main")); + w.open("f3"); + let list = |q: &str| -> Vec { + w.api("GET", &format!("/pulls?{q}"), None) + .json() + .as_array() + .unwrap() + .iter() + .map(|p| p["number"].as_i64().unwrap()) + .collect() + }; + assert_eq!(list(""), vec![3, 2, 1]); + assert_eq!(list("sort=oldest"), vec![1, 2, 3]); + assert_eq!(list("state=closed"), Vec::::new()); + assert_eq!(list("limit=2&page=2"), vec![1]); + let r = w.api("GET", "/pulls?limit=2&state=open", None); + assert_eq!(r.header("x-total-count").as_deref(), Some("3")); + assert!( + r.header("link") + .unwrap() + .contains("limit=2&page=2&state=open"), + "{:?}", + r.header("link") + ); + // closing moves it between the lists, and a closed PR keeps the head it had + let c = w.api("PATCH", "/pulls/2", Some(json!({"state": "closed"}))); + assert_eq!(c.status, 201, "{}", c.msg()); + assert_eq!(c.json()["state"], "closed"); + assert!(c.json()["closed_at"].is_string()); + assert_eq!(list("state=closed"), vec![2]); + assert_eq!(list("state=all"), vec![3, 2, 1]); + let r = w.api( + "PATCH", + "/pulls/2", + Some(json!({"state": "open", "title": "renamed"})), + ); + assert_eq!( + (r.status, r.json()["title"].as_str().unwrap().to_string()), + (201, "renamed".to_string()) + ); + assert_eq!(w.api("GET", "/pulls/9", None).status, 404); + assert_eq!( + w.api("PATCH", "/pulls/2", Some(json!({"state": "bogus"}))) + .status, + 422 + ); +} + +#[test] +fn deleting_the_head_branch_closes_the_pull_and_keeps_its_head() { + let w = world(); + let tip = w.commit("gone", "g.txt", Some("main")); + w.open("gone"); + let d = w.api("DELETE", "/branches/gone", None); + assert_eq!(d.status, 204, "{}", d.msg()); + let pr = w.api("GET", "/pulls/1", None).json(); + assert_eq!( + pr["state"], "closed", + "recorded: deleting the head closes the PR" + ); + assert_eq!(pr["head"]["ref"], "refs/pull/1/head"); + assert_eq!(pr["head"]["label"], "gone"); + assert_eq!(pr["head"]["sha"], tip.as_str()); + assert_eq!(w.retained(1).as_deref(), Some(tip.as_str())); + assert!( + w.git(&["cat-file", "-t", &tip]).contains("commit"), + "the commit is still there" + ); + // diff and commits still work from the retained head + assert!(w.api("GET", "/pulls/1.diff", None).s_text().contains("+x")); + assert_eq!( + w.api("GET", "/pulls/1/commits", None) + .json() + .as_array() + .unwrap() + .len(), + 1 + ); + // reopening works without the branch, and a later sync does not close it again + let re = w.api("PATCH", "/pulls/1", Some(json!({"state": "open"}))); + assert_eq!(re.status, 201); + assert_eq!(re.json()["state"], "open"); + w.commit("other", "o.txt", Some("main")); + w.api("DELETE", "/branches/other", None); + assert_eq!(w.api("GET", "/pulls/1", None).json()["state"], "open"); +} + +#[test] +fn a_push_that_deletes_or_rewrites_the_head_keeps_it_reachable() { + use std::process::Command; + let w = world(); + /// Removes the client directory when the test ends, however it ends. + struct Tmp(std::path::PathBuf); + impl Drop for Tmp { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + assert!(std::thread::panicking() || !self.0.exists()); + } + } + let c = std::env::temp_dir().join(format!("scl-forge-pulls-push-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&c); + std::fs::create_dir_all(&c).unwrap(); + let _guard = Tmp(c.clone()); + let run = |args: &[&str]| { + let out = Command::new("git") + .env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &c) + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_AUTHOR_NAME", "T") + .env("GIT_AUTHOR_EMAIL", "t@example.invalid") + .env("GIT_COMMITTER_NAME", "T") + .env("GIT_COMMITTER_EMAIL", "t@example.invalid") + .arg("-c") + .arg(format!("http.extraHeader=Authorization: {}", w.auth)) + .args(args) + .current_dir(&c) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() + }; + let url = format!("{}/siteadmin/w.git", w.s.base); + run(&["clone", "-q", &url, "work"]); + let work = c.join("work"); + let in_work = |args: &[&str]| { + let out = Command::new("git") + .env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &c) + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_AUTHOR_NAME", "T") + .env("GIT_AUTHOR_EMAIL", "t@example.invalid") + .env("GIT_COMMITTER_NAME", "T") + .env("GIT_COMMITTER_EMAIL", "t@example.invalid") + .arg("-c") + .arg(format!("http.extraHeader=Authorization: {}", w.auth)) + .args(args) + .current_dir(&work) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() + }; + in_work(&["checkout", "-q", "-b", "pb"]); + std::fs::write(work.join("p.txt"), "p\n").unwrap(); + in_work(&["add", "."]); + in_work(&["commit", "-q", "-m", "p1"]); + in_work(&["push", "-q", "origin", "pb"]); + w.open("pb"); + let first = in_work(&["rev-parse", "HEAD"]); + // a forced rewrite moves the PR head and the retention ref + std::fs::write(work.join("p.txt"), "p2\n").unwrap(); + in_work(&["commit", "-q", "--amend", "-a", "-m", "p1 again"]); + in_work(&["push", "-q", "-f", "origin", "pb"]); + let second = in_work(&["rev-parse", "HEAD"]); + assert_ne!(first, second); + assert_eq!( + w.api("GET", "/pulls/1", None).json()["head"]["sha"], + second.as_str() + ); + assert_eq!(w.retained(1).as_deref(), Some(second.as_str())); + // a push that deletes the branch closes the PR on the last head: the hook records the tip itself + // (the ref is pointed elsewhere first, so only the hook can put it right) + w.git(&["update-ref", "refs/forge/pull/1/head", &first]); + in_work(&["push", "-q", "origin", ":pb"]); + let pr = w.api("GET", "/pulls/1", None).json(); + assert_eq!(pr["state"], "closed"); + assert_eq!(pr["head"]["sha"], second.as_str()); + assert_eq!(w.retained(1).as_deref(), Some(second.as_str())); +} + +#[test] +fn diff_patch_commits_and_files_match_git() { + let w = world(); + let base = w.git(&["rev-parse", "refs/heads/main"]); + w.commit("d", "one.txt", Some("main")); + let head = w.commit("d", "two.txt", None); + w.open("d"); + let diff = w.api("GET", "/pulls/1.diff", None); + assert_eq!(diff.status, 200); + assert_eq!( + diff.s_text(), + w.git(&["diff-tree", "-p", "-r", "--no-renames", &base, &head]) + "\n" + ); + let patch = w.api("GET", "/pulls/1.patch", None).s_text(); + assert!( + patch.contains("[PATCH 1/2]") && patch.contains("[PATCH 2/2]"), + "{patch}" + ); + let commits = w.api("GET", "/pulls/1/commits", None); + assert_eq!(commits.header("x-total-count").as_deref(), Some("2")); + assert_eq!(commits.json()[0]["sha"], head.as_str(), "newest first"); + assert_eq!(commits.json()[0]["files"][0]["filename"], "two.txt"); + let files = w.api("GET", "/pulls/1/files", None).json(); + let names: Vec<&str> = files + .as_array() + .unwrap() + .iter() + .map(|f| f["filename"].as_str().unwrap()) + .collect(); + assert_eq!(names, vec!["one.txt", "two.txt"]); + assert_eq!(files[0]["status"], "added"); + assert_eq!(files[0]["additions"], 1); + assert!(files[0]["raw_url"] + .as_str() + .unwrap() + .ends_with(&format!("/raw/commit/{head}/one.txt"))); + assert_eq!( + w.api("GET", "/pulls/1/merge", None).status, + 404, + "not merged" + ); +} + +#[test] +fn reviews_follow_the_recorded_rules() { + let w = world(); + w.commit("r", "r.txt", Some("main")); + // bot opens the PR with write access; alice (write) and carol (read) review + let mk = |login: &str, perm: &str| { + let pw = w.s.mk_user(login); + let t = token(&w.s.mk_token(login, &pw, "t", &["all"])); + let r = w.api( + "PUT", + &format!("/collaborators/{login}"), + Some(json!({"permission": perm})), + ); + assert_eq!(r.status, 204); + t + }; + let bot = mk("bot", "write"); + let alice = mk("alice", "write"); + let carol = mk("carol", "read"); + let r = w.as_( + &bot, + "POST", + "/pulls", + Some(json!({"title": "t", "head": "r", "base": "main"})), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + let post = |who: &str, ev: &str| { + w.as_( + who, + "POST", + "/pulls/1/reviews", + Some(json!({"body": ev, "event": ev})), + ) + }; + assert_eq!(post(&alice, "REQUEST_CHANGES").json()["official"], true); + let own = post(&bot, "APPROVED"); + assert_eq!( + (own.status, own.msg().as_str()), + (422, "approve your own pull is not allowed") + ); + assert_eq!(post(&bot, "REQUEST_CHANGES").status, 422); + assert_eq!(post(&bot, "COMMENT").status, 200, "the author may comment"); + assert_eq!(post(&alice, "APPROVED").json()["official"], true); + assert_eq!( + post(&carol, "APPROVED").json()["official"], + false, + "no write access" + ); + let pending = post(&alice, "BOGUS"); + assert_eq!(pending.json()["state"], "PENDING"); + let inline = w.as_(&alice, "POST", "/pulls/1/reviews", Some(json!({"event": "COMMENT", "comments": [{"path": "r.txt", "body": "x", "new_position": 1}]}))); + assert_eq!( + inline.status, 422, + "inline comments are refused, not dropped" + ); + + let list = w.as_(&bot, "GET", "/pulls/1/reviews", None); + let rows = list.json(); + let states: Vec<(String, bool, bool)> = rows + .as_array() + .unwrap() + .iter() + .map(|r| { + ( + r["state"].as_str().unwrap().to_string(), + r["official"].as_bool().unwrap(), + r["dismissed"].as_bool().unwrap(), + ) + }) + .collect(); + assert_eq!( + states, + vec![ + ("REQUEST_CHANGES".into(), false, true), + ("COMMENT".into(), false, false), + ("APPROVED".into(), true, false), + ("APPROVED".into(), false, false), + ], + "alice's pending review is hidden from bot; her change request was superseded" + ); + assert_eq!( + list.header("x-total-count").as_deref(), + Some("5"), + "recorded: the total counts the hidden one" + ); + let mine = w.as_(&alice, "GET", "/pulls/1/reviews", None).json(); + assert_eq!(mine.as_array().unwrap().len(), 5); + let pid = pending.json()["id"].as_i64().unwrap(); + assert_eq!( + w.as_(&bot, "GET", &format!("/pulls/1/reviews/{pid}"), None) + .status, + 404 + ); + assert_eq!( + w.as_(&alice, "GET", &format!("/pulls/1/reviews/{pid}"), None) + .status, + 200 + ); + assert_eq!( + w.as_( + &alice, + "GET", + &format!("/pulls/1/reviews/{pid}/comments"), + None + ) + .json(), + json!([]) + ); + assert_eq!(w.as_(&alice, "GET", "/pulls/7/reviews", None).status, 404); +} + +#[test] +fn issue_comments_belong_to_the_pull_and_need_the_issue_scope() { + let w = world(); + w.commit("c", "c.txt", Some("main")); + w.open("c"); + let r = w.api("POST", "/issues/1/comments", Some(json!({"body": "hello"}))); + assert_eq!(r.status, 201, "{}", r.msg()); + assert_eq!(r.json()["body"], "hello"); + assert_eq!(r.json()["user"]["email"], "siteadmin@noreply.localhost"); + w.api( + "POST", + "/issues/1/comments", + Some(json!({"body": "second"})), + ); + let l = w.api("GET", "/issues/1/comments", None); + assert_eq!(l.header("x-total-count").as_deref(), Some("2")); + assert_eq!(l.json()[0]["body"], "hello", "oldest first"); + assert_eq!(w.api("GET", "/pulls/1", None).json()["comments"], 2); + assert_eq!( + w.api("POST", "/issues/1/comments", Some(json!({"body": " "}))) + .status, + 422 + ); + assert_eq!( + w.api( + "POST", + "/issues/1/comments", + Some(json!({"body": "x".repeat(70_000)})) + ) + .status, + 422 + ); + assert_eq!(w.api("GET", "/issues/5/comments", None).status, 404); + // a token without the issue scope is refused + let pw = w.s.mk_user("scoped"); + let t = token(&w.s.mk_token("scoped", &pw, "t", &["read:repository", "write:repository"])); + w.api( + "PUT", + "/collaborators/scoped", + Some(json!({"permission": "write"})), + ); + assert_eq!(w.as_(&t, "GET", "/issues/1/comments", None).status, 403); + assert_eq!( + w.as_(&t, "POST", "/issues/1/comments", Some(json!({"body": "x"}))) + .status, + 403 + ); + assert_eq!(w.as_(&t, "GET", "/pulls/1", None).status, 200); +} + +#[test] +fn reads_do_not_deadlock_against_exclusive_repository_operations() { + let w = world(); + w.commit("s", "s.txt", Some("main")); + let tip = w.git(&["rev-parse", "refs/heads/s"]); + w.open("s"); + let stop = std::sync::atomic::AtomicBool::new(false); + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::scope(|sc| { + for _ in 0..3 { + sc.spawn(|| { + while !stop.load(std::sync::atomic::Ordering::SeqCst) { + for p in ["/pulls", "/pulls/1", "/pulls/1/files", "/pulls/1/commits"] { + let r = w.api("GET", p, None); + assert_eq!(r.status, 200, "{p}: {}", r.msg()); + } + } + }); + } + let (wr, tipr, stopr) = (&w, &tip, &stop); + sc.spawn(move || { + for i in 0..30 { + wr.git(&["update-ref", &format!("refs/heads/t{i}"), tipr]); + let r = wr.api("DELETE", &format!("/branches/t{i}"), None); + assert_eq!(r.status, 204, "{}", r.msg()); + } + stopr.store(true, std::sync::atomic::Ordering::SeqCst); + tx.send(()).unwrap(); + }); + assert!( + rx.recv_timeout(std::time::Duration::from_secs(90)).is_ok(), + "no reader wedged the exclusive lock" + ); + }); +} + +#[test] +fn startup_reconcile_restores_retention_refs_and_closes_vanished_heads() { + let w = world(); + let tip = w.commit("rc", "rc.txt", Some("main")); + w.open("rc"); + w.api("PATCH", "/pulls/1", Some(json!({"state": "closed"}))); + // a lost retention ref, a stray one, and a branch removed behind the server's back + w.git(&["update-ref", "-d", "refs/forge/pull/1/head"]); + w.git(&["update-ref", "refs/forge/pull/9/head", &tip]); + w.commit("rc2", "rc2.txt", Some("main")); + w.open("rc2"); + w.git(&["update-ref", "-d", "refs/heads/rc2"]); + let conn = scl_forge::db::connect(&w.s.dir).unwrap(); + let git = + scl_forge::gitproc::Git::new(&w.s.dir, scl_forge::gitproc::find_git().unwrap(), 2).unwrap(); + let dir = w.s.dir.join("repos").join(format!("{}.git", w.id)); + scl_forge::pulls::reconcile_repo(&conn, &git, &dir, w.id, 100).unwrap(); + assert_eq!( + w.retained(1).as_deref(), + Some(tip.as_str()), + "a closed PR gets its ref back" + ); + assert_eq!(w.retained(9), None, "a ref with no PR is removed"); + assert_eq!( + w.api("GET", "/pulls/2", None).json()["state"], + "closed", + "the vanished head closed it" + ); + assert!(w.retained(2).is_some()); +} + +#[test] +fn a_closed_pull_keeps_the_head_it_was_closed_with() { + let w = world(); + let a = w.commit("k", "k1.txt", Some("main")); + w.open("k"); + w.api("PATCH", "/pulls/1", Some(json!({"state": "closed"}))); + let b = w.commit("k", "k2.txt", None); + assert_ne!(a, b); + let pr = w.api("GET", "/pulls/1", None).json(); + assert_eq!( + pr["head"]["sha"], + a.as_str(), + "the branch moved, the closed PR did not" + ); + assert_eq!(pr["head"]["ref"], "k"); + assert_eq!( + w.api("GET", "/pulls/1/commits", None) + .json() + .as_array() + .unwrap() + .len(), + 1 + ); + assert_eq!(w.retained(1).as_deref(), Some(a.as_str())); + // reopening follows the branch again + let re = w + .api("PATCH", "/pulls/1", Some(json!({"state": "open"}))) + .json(); + assert_eq!(re["head"]["sha"], b.as_str()); + assert_eq!( + w.retained(1).as_deref(), + Some(b.as_str()), + "sync re-points the ref" + ); +} + +#[test] +fn a_stale_sync_never_overwrites_a_newer_retained_head() { + let w = world(); + let a = w.commit("cas", "c1.txt", Some("main")); + w.open("cas"); + let git = &w.s.runner; + let dir = w.s.dir.join("repos").join(format!("{}.git", w.id)); + // someone (the pre-receive hook) recorded a newer tip after the snapshot a sync took + let b = w.commit("other", "c2.txt", Some("main")); + assert!( + !scl_forge::pulls::retain_cas(git, &dir, 1, &b, Some(&"1".repeat(40))).unwrap(), + "wrong expectation: refused" + ); + assert_eq!(w.retained(1).as_deref(), Some(a.as_str()), "untouched"); + assert!(scl_forge::pulls::retain_cas(git, &dir, 1, &b, Some(&a)).unwrap()); + assert_eq!(w.retained(1).as_deref(), Some(b.as_str())); + assert!( + !scl_forge::pulls::retain_cas(git, &dir, 1, &a, None).unwrap(), + "create-only refuses an existing ref" + ); +} + +#[test] +fn page_links_select_the_rows_the_page_did() { + let w = world(); + for i in 0..3 { + w.commit(&format!("p{i}"), &format!("p{i}.txt"), Some("main")); + w.open(&format!("p{i}")); + } + // an encoded name is not `limit` for the server, so it is not for the next page either + let r = w.api("GET", "/pulls?%6cimit=2&limit=1&sort=oldest", None); + assert_eq!(r.json().as_array().unwrap().len(), 1); + let link = r.header("link").unwrap(); + assert!( + link.contains("limit=1&page=2") && !link.contains("limit=2"), + "{link}" + ); + // a bare key is ignored by the server and not repeated + let r = w.api("GET", "/pulls?sort&sort=oldest&limit=1", None); + assert_eq!(r.json()[0]["number"], 1); + assert!(r.header("link").unwrap().contains("sort=oldest")); + // a hostile query cannot break out of the link target + let r = w.api("GET", "/pulls?limit=1&x=%3E%2C%3Chttp://evil%3E", None); + let link = r.header("link").unwrap(); + assert!( + !link.contains("evil>") && link.matches('<').count() == link.matches("; rel=").count(), + "{link}" + ); +} + +#[test] +fn startup_reconcile_covers_repositories_without_any_pull() { + let w = world(); + let tip = w.git(&["rev-parse", "refs/heads/main"]); + w.git(&["update-ref", "refs/forge/pull/1/head", &tip]); // an orphan of a crashed first creation + let conn = scl_forge::db::connect(&w.s.dir).unwrap(); + scl_forge::pulls::reconcile_all(&w.s.app.life, &conn, 100); + assert_eq!(w.retained(1), None); +} + +#[test] +fn branches_differing_only_in_case_are_two_heads_and_file_urls_are_encoded() { + let w = world(); + let up = w.git(&["rev-parse", "refs/heads/main"]); + // two branches that a case-insensitive match would conflate (skipped where the filesystem folds case) + let lower = { + let r = w.api("POST", "/contents/a%23b.txt", Some(json!({"content": b64("x\n"), "message": "m", "branch": "main", "new_branch": "feature"}))); + assert_eq!(r.status, 201, "{}", r.msg()); + w.git(&["rev-parse", "refs/heads/feature"]) + }; + if w.s + .git_try(w.id, &["rev-parse", "--verify", "-q", "refs/heads/Feature"]) + .is_some() + { + return; // case-folding filesystem + } + w.git(&["update-ref", "refs/heads/Feature", &up]); + let upper = { + w.api("POST", "/contents/other.txt", Some(json!({"content": b64("y\n"), "message": "m", "branch": "main", "new_branch": "Feature2"}))); + w.git(&["rev-parse", "refs/heads/Feature2"]) + }; + w.git(&["update-ref", "refs/heads/Feature", &upper]); + w.open("feature"); + assert_eq!( + w.retained(1).as_deref(), + Some(lower.as_str()), + "the PR follows `feature`, not `Feature`" + ); + w.git(&["update-ref", "-d", "refs/heads/feature"]); + assert_eq!( + w.api("GET", "/pulls/1", None).json()["state"], + "closed", + "`Feature` does not keep it open" + ); + assert_eq!(w.retained(1).as_deref(), Some(lower.as_str())); + // URLs name the file, not a fragment + let r = w.api( + "POST", + "/contents/a%23b.txt", + Some(json!({"content": b64("x\n"), "message": "m", "branch": "main", "new_branch": "u"})), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + w.open("u"); + let f = w.api("GET", "/pulls/2/files", None).json(); + assert_eq!(f[0]["filename"], "a#b.txt"); + assert!(f[0]["contents_url"] + .as_str() + .unwrap() + .contains("/contents/a%23b.txt?ref=")); + assert!(f[0]["raw_url"].as_str().unwrap().ends_with("/a%23b.txt")); +} diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index 029a1706..e495f7ed 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -1023,6 +1023,33 @@ Tests: `crates/forge/tests/e2e_protection.rs`, the `policy`, `hook`, `protection - **Branch delete.** The API refuses a protected branch and deletes with compare-and-swap on the observed tip, so a concurrent push is not erased by a stale delete. +### Pull requests, reviews and retention refs (F4b) + +Threat-model pass #2, third part; merge races and the protection bypass matrix come with F4c. Tests: `crates/forge/tests/e2e_pulls.rs` +and the `pulls` unit tests. + +- **Who can do what.** Opening a PR needs write access (Gitea allows readers); changing one needs write access or being its + creator; reviews and comments need read access, and a reviewer without write access cannot make a review `official`. Reviews + and comments are bounded (64 KiB each; 1000 reviews and 5000 comments per PR). Inline review comments are refused rather than + dropped. +- **Retention refs.** `refs/forge/pull//head` is server-only: pushes to the namespace are refused (F2b), reads hide it. Every + write to it happens under the repository's write gate, except the `pre-receive` hook's record of a deleted head's tip, which + runs without git's quarantine variables, validates the object id as hex, and refuses the push if it fails. The ref is written + before the PR row, so a crash leaves an orphan ref (removed at startup), never a PR whose head can be pruned. Deleting a PR head + branch (API or push) closes the PR and keeps the commit reachable (`deleting_the_head_branch_closes_the_pull_and_keeps_its_head`, + `a_push_that_deletes_or_rewrites_the_head_keeps_it_reachable`); the startup reconcile repairs any disagreement + (`startup_reconcile_restores_retention_refs_and_closes_vanished_heads`). Unreferenced objects are not collected until F5's gc. +- **Fetch by id.** `uploadpack.allowAnySHA1InWant` stays on, so a commit kept only by a hidden PR ref is fetchable by anyone who + can read the repository and knows its id (as for any object over protocol v2); a PR head is no more secret than a branch. +- **Locks.** PR reads take the shared lifecycle lock once and never call `repo_json` (which takes it again); a read that finds a + vanished head runs the sync before taking the lock (`reads_do_not_deadlock_against_exclusive_repository_operations`). +- **Account objects.** A PR's `user` and `merged_by` carry the full account object (stored email, admin flag), as recorded + from Gitea and as `GET /users/{u}` already shows to any signed-in caller; only the objects Gitea itself nests in statuses, + comments are masked (login-derived email, no admin flag), and a review list shows the stored email, as recorded (F#33). Anyone with read access to a repository can + therefore see the stored email of its PR authors. Accounts that must not be identifiable should use a placeholder email. +- **Cost.** Mergeability and diff statistics run `git` per PR on a cache miss; a page lists at most 50 PRs. A repository with + many open PRs and a huge history makes a list slow, not wrong; the runner's concurrency and timeouts bound it. + ## Reporting Security reports go to **toni@git-agentic.com** or GitHub private vulnerability diff --git a/docs/superpowers/specs/2026-10-22-p38-forge-design.md b/docs/superpowers/specs/2026-10-22-p38-forge-design.md index 4b2f278e..22623a0b 100644 --- a/docs/superpowers/specs/2026-10-22-p38-forge-design.md +++ b/docs/superpowers/specs/2026-10-22-p38-forge-design.md @@ -430,7 +430,11 @@ use the cache and compute misses under a small concurrency limit. **PR head lifecycle.** An open PR's `head.sha` is the live branch tip (a force-push shows immediately); a merged PR's `head.sha` is frozen at merge; a -closed PR keeps its last head through `refs/forge/pull//head`. Pushing the +closed PR keeps its last head through `refs/forge/pull//head`. **Deleting the head +branch closes the PR** (recorded O#6-O#10 and O#24-O#25, both Gitea versions; an earlier +version of this paragraph and of `FINDINGS.md` said the PR stays open, which the fixtures +contradict); the PR then reads `head.ref: refs/pull//head` with `head.label` the branch +name, and reopening it (201) works without the branch. Pushing the head commit straight into base does not mark the PR merged (recorded; the agent runtime's landing check handles a trunk that contains the commits). @@ -901,6 +905,44 @@ F3b is the write half of the content API (plan: `docs/superpowers/plans/2026-10- deterministic interfering writer); the F4 `refs/forge/pull//head` refresh for a contents write to a PR branch; portable-name collision rules; a disk reservation sized to the exact rebuilt trees. +### F4b decisions (as built) + +F4b adds migration 4 (`pulls`, `mergeability_cache`, `reviews`, `issue_comments`) and gates F, O (the merge-dependent exchanges +stay `defer`red until F4c), P and D#13. + +- **PR object.** The recorded 1.22.6 shape (`id` is the row id, `number` the per-repository index; `user` is the creator's full + account object; `base.repo`/`head.repo` are the repository object). Open PRs show the live tips; `mergeable`, `merge_base`, + `changed_files`, `additions`, `deletions` come from `merge-base`, `diff-tree --numstat` and `merge-tree --write-tree`, cached + by `(base_sha, head_sha)`; a closed PR reads `mergeable: true`; a merged PR is frozen (F4c). Create needs write access (Gitea + lets any reader open one; stricter here); another repository as head is 404 (forks are a non-goal). `head == base` as commits + is 422 `Invalid PullRequest: There are no changes between the head and the base`; a head contained in base is accepted. + PATCH answers 201 and takes `title`, `body`, `state` and `base`; the PR's creator or write access may use it. +- **Lists.** `state` open (default), closed (closed and merged) or all; `sort` newest (default), oldest or recentupdate; the + `head` filter is ignored like Gitea's; 404 on a repository with no commits (D#13). `X-Total-Count` and `Link` follow the + request's own query (see below). +- **Retention (differs from the F0 text).** No `post-receive` database write and no `freezing` state: refresh and freeze both + run in the server under the write gate. `pulls::sync_locked` follows every head branch with `refs/forge/pull//head` and + closes an open PR whose head branch is gone (keeping the retained head as `final_head_sha`); it runs after an accepted push, + a contents write, a branch DELETE and at startup, and reads converge by running it when an open PR's head is missing. The + ref is written before the PR row exists. Branch DELETE retains the tip (under the exclusive lock) before the CAS delete, and + the `pre-receive` hook records `old` into the ref of an open PR whose head branch a push deletes (no quarantine variables + in that spawn; any failure refuses the push). `head_gone` marks a PR reopened without its branch so a later sync leaves it + open (recorded O#26-O#27). The startup reconcile also restores the ref of closed and merged PRs from `final_head_sha` and + removes `refs/forge/pull//head` refs with no row. +- **Reviews.** `event` APPROVED/APPROVE, REQUEST_CHANGES, COMMENT, anything else PENDING; approving or requesting changes on + your own PR is 422; `official` is stored at creation (write access and a deciding state) and a later deciding review by the + same reviewer marks the earlier one `dismissed` and not official; PENDING reviews are visible only to their author but + counted in `X-Total-Count` (recorded quirk); the POST answer carries the full reviewer account, reads the nested form. + Inline review `comments` are refused (422) and a review's comment list is always empty. Issue comments live under + `/issues/{n}/comments` where `n` is a PR number (`issue` scope), oldest first. Caps: 64 KiB of text, 1000 reviews and 5000 + comments per PR. +- **Links.** All paged lists repeat the request's own query parameters (sorted by name, first value of a repeated one, + re-encoded strictly), with `page` replaced and clamped to the last page, and `limit` only when the request sent one (recorded + P#19-P#32). The commits list keeps its always-`limit` form. Branches are listed newest commit first, ties by name (P#19-22). + The combined status pages from the newest end and lists each page oldest first (P#40-41); history links name the request path. +- **Not built / departures.** Forks, labels, milestones, assignees, requested reviewers and inline review comments; a + list of up to 50 PRs computes its cache misses one after another. + ### F4a decisions (as built) F4 ships as three PRs (F4a protection and statuses, F4b pulls, reviews and comments, F4c merge and recovery; plan: