diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 85e652b9..37f674de 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -480,7 +480,7 @@ PR-scoped commands: `pr ` @@ -671,6 +671,7 @@ Edit the PR's title / body / state / milestone. Fields you don't pass keep their Possible values: `open`, `closed` * `--milestone ` — Milestone id (0 to unset) +* `--base ` — Retarget the PR onto this base branch (omit to leave unchanged) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index caacb662..efdefd7d 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -67,6 +67,7 @@ hive-forge issue create --title "..." --body "..." hive-forge issue create --title "..." --body "..." --label area/ops --label type/bug # repeatable hive-forge issue edit 42 --title "new title" hive-forge pr edit 42 --title "new title" # same edit, PR-kind-validated +hive-forge pr edit 42 --base main # retarget a stacked PR onto main hive-forge pr 42 # PR metadata as JSON hive-forge pr create --title "..." --head my-branch --push # also `git push forge my-branch` hive-forge pr create --title "..." --head my-branch --label area/ops # repeatable, same as issue create diff --git a/hive-forge/src/verbs/issue_edit.rs b/hive-forge/src/verbs/issue_edit.rs index f9a10fb2..9b7cf9ae 100644 --- a/hive-forge/src/verbs/issue_edit.rs +++ b/hive-forge/src/verbs/issue_edit.rs @@ -1,9 +1,9 @@ //! `issue edit [--title ] [body sources] [--state s] //! [--milestone id] [repo]` — partial update of an issue. Omitted -//! fields keep their current value. Also backs `pr edit`: Forgejo serves -//! both kinds off the same `/issues/` endpoint, so this is shared -//! as-is — `pr_cmd.rs` wires it in with a `Kind::Pr` check, the same -//! pattern `close`/`reopen`/`labels` already use. +//! fields keep their current value. Also backs `pr edit` without `--base`: +//! Forgejo serves both kinds off the same `/issues/` endpoint, so this +//! is shared as-is — `pr_edit.rs` wraps it, and `pr_cmd.rs` adds the +//! `Kind::Pr` check, the same pattern `close`/`reopen`/`labels` already use. use anyhow::Result; use clap::{Args as ClapArgs, ValueEnum}; @@ -27,19 +27,36 @@ pub struct Args { pub(crate) number: u64, /// New title (omit to leave unchanged). #[arg(long)] - title: Option, + pub(crate) title: Option, /// Inline body text (omit to leave unchanged). #[arg(long, conflicts_with = "body_file")] - body: Option, + pub(crate) body: Option, /// Read body from a file. `-` means stdin. #[arg(long = "body-file")] - body_file: Option, + pub(crate) body_file: Option, /// New state. #[arg(long, value_enum)] - state: Option, + pub(crate) state: Option, /// Milestone id (0 to unset). #[arg(long)] - milestone: Option, + pub(crate) milestone: Option, +} + +/// Body is partial: only update the body field if a source was +/// actually given. Piped stdin without --body/--body-file leaves +/// body alone (the partial-update contract). Absent fields ride as +/// JSON `null`, which Forgejo's partial-update binding treats as +/// "leave unchanged". +/// +/// # Errors +/// +/// Propagates any I/O error from the body input (`--body-file`, stdin). +pub(crate) fn resolve_body(args: &Args) -> Result> { + if args.body.is_some() || args.body_file.is_some() { + body::resolve(args.body.as_deref(), args.body_file.as_deref()) + } else { + Ok(None) + } } /// # Errors @@ -49,18 +66,18 @@ pub struct Args { /// unreachable, 4xx/5xx response, token missing/invalid), and any /// I/O error from writing the response to stdout. pub fn run(client: &Client, args: Args) -> Result<()> { - // Body is partial: only update the body field if a source was - // actually given. Piped stdin without --body/--body-file leaves - // body alone (the partial-update contract). Absent fields ride as - // JSON `null`, which Forgejo's partial-update binding treats as - // "leave unchanged". - let body_explicit = args.body.is_some() || args.body_file.is_some(); - let body = if body_explicit { - body::resolve(args.body.as_deref(), args.body_file.as_deref())? - } else { - None - }; + let body = resolve_body(&args)?; + run_with_body(client, args, body) +} +/// [`run`] with the body already resolved, for a caller that has consumed the +/// body source (stdin can be read once) before deciding to edit via the +/// issue endpoint. `args.body` / `args.body_file` are ignored. +/// +/// # Errors +/// +/// Same transport and output errors as [`run`]. +pub(crate) fn run_with_body(client: &Client, args: Args, body: Option) -> Result<()> { let payload = EditIssueOption { assignee: None, assignees: None, diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index ae80ef5e..61b993fa 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -34,6 +34,7 @@ pub mod pr_assign_reviewer; pub mod pr_cmd; pub mod pr_commits; pub mod pr_create; +pub mod pr_edit; pub mod pr_merge; pub mod pr_reviews; pub mod pr_status; diff --git a/hive-forge/src/verbs/pr_cmd.rs b/hive-forge/src/verbs/pr_cmd.rs index 1a369c7e..5b2ef040 100644 --- a/hive-forge/src/verbs/pr_cmd.rs +++ b/hive-forge/src/verbs/pr_cmd.rs @@ -39,9 +39,9 @@ enum Cmd { Diff(verbs::diff::Args), /// Show title + body + comments. View(verbs::view::Args), - /// Edit the PR's title / body / state / milestone. Fields you don't - /// pass keep their current value. - Edit(verbs::issue_edit::Args), + /// Edit the PR's title / body / state / milestone / base branch. Fields + /// you don't pass keep their current value. + Edit(verbs::pr_edit::PrEditArgs), /// Post a comment on the PR. Comment(verbs::comment::Args), /// List comments on the PR. @@ -81,8 +81,8 @@ pub fn run(client: &Client, args: Args) -> Result<()> { verbs::view::run(client, a) } Cmd::Edit(a) => { - assert_kind(client, a.number, Kind::Pr)?; - verbs::issue_edit::run(client, a) + assert_kind(client, a.edit.number, Kind::Pr)?; + verbs::pr_edit::run(client, a) } Cmd::Comment(a) => { assert_kind(client, a.number, Kind::Pr)?; diff --git a/hive-forge/src/verbs/pr_edit.rs b/hive-forge/src/verbs/pr_edit.rs new file mode 100644 index 00000000..af1af297 --- /dev/null +++ b/hive-forge/src/verbs/pr_edit.rs @@ -0,0 +1,134 @@ +//! `pr edit [--title ] [body sources] [--state s] +//! [--milestone id] [--base ]` — partial update of a PR. Omitted +//! fields keep their current value. +//! +//! Without `--base` this is `issue edit` unchanged (`/issues/`). The +//! base branch only exists on Forgejo's pull-request edit endpoint +//! (`PATCH /pulls/`), so `--base` switches the whole edit onto that +//! endpoint. + +use anyhow::{Result, bail}; +use clap::Args as ClapArgs; +use clap::builder::NonEmptyStringValueParser; +use forgejo_api::structs::EditPullRequestOption; +use serde_json::json; + +use crate::client::{Client, index}; +use crate::verbs::issue_edit::{self, resolve_body}; +use crate::verbs::print_json; + +#[derive(ClapArgs)] +pub struct PrEditArgs { + #[command(flatten)] + pub(crate) edit: issue_edit::Args, + /// Retarget the PR onto this base branch (omit to leave unchanged). + #[arg(long, value_name = "BRANCH", value_parser = NonEmptyStringValueParser::new())] + base: Option, +} + +/// `--milestone 0` means "unset" on the issue endpoint; whether +/// `PATCH /pulls/` reads it the same way is unverified, so the +/// combination with `--base` is refused rather than sent. +fn reject_unverified(args: &PrEditArgs) -> Result<()> { + if args.base.is_some() && args.edit.milestone == Some(0) { + bail!( + "hive-forge: --milestone 0 (unset) cannot be combined with --base: \ + unsetting a milestone is unverified on the pull-request endpoint. \ + Run the two edits separately." + ); + } + Ok(()) +} + +/// The `PATCH /pulls/` payload, or `None` when `--base` is unset and the +/// edit rides the issue endpoint instead. +fn pull_payload(args: &PrEditArgs, body: Option) -> Result> { + let Some(base) = args.base.clone() else { + return Ok(None); + }; + Ok(Some(EditPullRequestOption { + allow_maintainer_edit: None, + assignee: None, + assignees: None, + base: Some(base), + body, + due_date: None, + labels: None, + milestone: args.edit.milestone.map(index).transpose()?, + state: args.edit.state.map(|s| <&str>::from(s).to_owned()), + title: args.edit.title.clone(), + unset_due_date: None, + })) +} + +/// # Errors +/// +/// Propagates any I/O error from the body input (`--body-file`, stdin), +/// any transport error from the Forgejo REST call (network unreachable, +/// 4xx/5xx response such as an unknown base branch, token +/// missing/invalid), and any I/O error from writing the response to +/// stdout. +pub fn run(client: &Client, args: PrEditArgs) -> Result<()> { + reject_unverified(&args)?; + let body = resolve_body(&args.edit)?; + let Some(payload) = pull_payload(&args, body.clone())? else { + return issue_edit::run_with_body(client, args.edit, body); + }; + let (owner, name) = client.owner_repo()?; + let resp = client + .api() + .repo_edit_pull_request(owner, name, index(args.edit.number)?, payload) + .send()?; + print_json(&json!({ + "number": resp.number, + "title": resp.title, + "state": resp.state, + "milestone": resp.milestone.as_ref().and_then(|m| m.title.as_deref()), + "base": resp.base.as_ref().and_then(|b| b.r#ref.as_deref()), + })) +} + +#[cfg(test)] +mod tests { + use super::*; + use clap::Parser; + + #[derive(clap::Parser)] + struct Cli { + #[command(flatten)] + args: PrEditArgs, + } + + fn payload(argv: &[&str]) -> Option { + let cli = Cli::parse_from(std::iter::once("x").chain(argv.iter().copied())); + pull_payload(&cli.args, None) + .unwrap() + .map(|p| serde_json::to_value(p).unwrap()) + } + + #[test] + fn base_flag_is_sent_as_base() { + let p = payload(&["5194", "--base", "main"]).expect("pulls payload"); + assert_eq!(p["base"], "main"); + assert!(p["title"].is_null()); + } + + #[test] + fn base_rides_alongside_other_fields() { + let p = payload(&["5194", "--base", "main", "--title", "t", "--state", "open"]) + .expect("pulls payload"); + assert_eq!(p["base"], "main"); + assert_eq!(p["title"], "t"); + assert_eq!(p["state"], "open"); + } + + #[test] + fn no_base_flag_sends_no_pulls_payload() { + assert!(payload(&["5194", "--title", "t"]).is_none()); + } + + #[test] + fn empty_base_is_rejected() { + assert!(Cli::try_parse_from(["x", "5194", "--base", ""]).is_err()); + } +} diff --git a/hive-forge/tests/pr_edit.rs b/hive-forge/tests/pr_edit.rs new file mode 100644 index 00000000..5aa4c9d1 --- /dev/null +++ b/hive-forge/tests/pr_edit.rs @@ -0,0 +1,167 @@ +//! `pr edit` against a mock forge that records every write it receives. +//! Drives the compiled binary with real stdin: `--body-file -` can be read +//! once, so a body resolved twice reaches the forge as an empty string. + +use std::io::{BufRead as _, BufReader, Read as _, Write as _}; +use std::net::{TcpListener, TcpStream}; +use std::path::PathBuf; +use std::process::{Command, Stdio}; +use std::sync::{Arc, Mutex}; + +const NUMBER: &str = "7"; + +/// `(method, path, body)` of each PATCH the mock forge received. +type Writes = Arc>>; + +/// A unique scratch `HYPERHIVE_STATE_DIR` holding a `forge-token` file, +/// same manual-tmpdir pattern as `credential_helper.rs`. +fn scratch_state_dir(tag: &str) -> PathBuf { + let ts = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_nanos()); + let dir = std::env::temp_dir().join(format!("hive-forge-pr-edit-test-{tag}-{ts}")); + std::fs::create_dir_all(&dir).expect("create scratch state dir"); + std::fs::write(dir.join("forge-token"), "test-token\n").expect("seed forge-token"); + dir +} + +/// Answer one request: #7 is a PR; both edit endpoints echo a minimal +/// object and record the PATCH body. +fn serve(mut stream: TcpStream, writes: &Writes) { + let mut reader = BufReader::new(stream.try_clone().expect("clone stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read request line"); + let mut content_length = 0usize; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).expect("read header") == 0 || line == "\r\n" { + break; + } + if let Some((name, value)) = line.split_once(':') + && name.eq_ignore_ascii_case("content-length") + { + content_length = value.trim().parse().expect("numeric content-length"); + } + } + let mut raw = vec![0u8; content_length]; + reader.read_exact(&mut raw).expect("read request body"); + let mut parts = request_line.split_whitespace(); + let method = parts.next().unwrap_or("").to_owned(); + let target = parts.next().unwrap_or(""); + let path = target.split('?').next().unwrap_or("").to_owned(); + if method == "PATCH" { + let body = serde_json::from_slice(&raw).expect("PATCH body is JSON"); + writes + .lock() + .expect("lock") + .push((method.clone(), path, body)); + } + let status = if method == "PATCH" { + "201 Created" + } else { + "200 OK" + }; + let body = serde_json::json!({ + "number": NUMBER.parse::().expect("numeric NUMBER"), + "title": "t", + "state": "open", + "pull_request": {"merged": false}, + "requested_reviewers": [], + }) + .to_string(); + let response = format!( + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + stream + .write_all(response.as_bytes()) + .expect("write response"); +} + +/// Start the mock forge on a loopback port; returns its base URL and the +/// shared record of `PATCH` requests. +fn mock_forge() -> (String, Writes) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock forge"); + let addr = listener.local_addr().expect("mock forge addr"); + let writes: Writes = Arc::default(); + let seen = Arc::clone(&writes); + std::thread::spawn(move || { + for stream in listener.incoming() { + serve(stream.expect("accept"), &seen); + } + }); + (format!("http://{addr}"), writes) +} + +/// Run `hive-forge -r o/r pr edit 7 ` with `stdin` piped in. Returns +/// `(success, stderr, `PATCH` requests)`. +fn pr_edit( + tag: &str, + args: &[&str], + stdin: &str, +) -> (bool, String, Vec<(String, String, serde_json::Value)>) { + let (url, writes) = mock_forge(); + let mut child = Command::new(env!("CARGO_BIN_EXE_hive-forge")) + .args(["-r", "o/r", "pr", "edit", NUMBER]) + .args(args) + .env("HIVE_FORGE_URL", url) + .env("HYPERHIVE_STATE_DIR", scratch_state_dir(tag)) + .env_remove("HIVE_FORGE_TOKEN_FILE") + .env_remove("HIVE_LABEL") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn hive-forge"); + child + .stdin + .take() + .expect("piped stdin") + .write_all(stdin.as_bytes()) + .expect("write stdin"); + let out = child.wait_with_output().expect("wait for hive-forge"); + let recorded = writes.lock().expect("lock").clone(); + ( + out.status.success(), + String::from_utf8_lossy(&out.stderr).into_owned(), + recorded, + ) +} + +#[test] +fn stdin_body_without_base_is_sent_once_to_the_issue_endpoint() { + let (ok, stderr, writes) = pr_edit("stdin-issue", &["--body-file", "-"], "stdin body\n"); + assert!(ok, "{stderr}"); + assert_eq!(writes.len(), 1, "{writes:?}"); + assert_eq!(writes[0].1, format!("/api/v1/repos/o/r/issues/{NUMBER}")); + assert_eq!(writes[0].2["body"], "stdin body\n"); + assert!(writes[0].2.get("base").is_none(), "{writes:?}"); +} + +#[test] +fn stdin_body_with_base_is_sent_to_the_pulls_endpoint() { + let (ok, stderr, writes) = pr_edit( + "stdin-pulls", + &["--body-file", "-", "--base", "main"], + "stdin body\n", + ); + assert!(ok, "{stderr}"); + assert_eq!(writes.len(), 1, "{writes:?}"); + assert_eq!(writes[0].1, format!("/api/v1/repos/o/r/pulls/{NUMBER}")); + assert_eq!(writes[0].2["body"], "stdin body\n"); + assert_eq!(writes[0].2["base"], "main"); +} + +#[test] +fn base_with_milestone_zero_is_refused_before_any_write() { + let (ok, stderr, writes) = pr_edit( + "milestone-zero", + &["--base", "main", "--milestone", "0"], + "", + ); + assert!(!ok); + assert!(stderr.contains("--milestone 0"), "{stderr}"); + assert!(writes.is_empty(), "{writes:?}"); +}