hive-forge: pr edit --base retargets a PR
Forgejo's issue edit endpoint (/issues/<n>) has no base field, so `pr edit` could not retarget a stacked PR once its base merged. `--base <BRANCH>` (non-empty) switches the edit onto PATCH /pulls/<n>, which carries `base` alongside title/body/state/milestone. Without `--base`, `pr edit` is unchanged and still goes through the issue endpoint.
This commit is contained in:
parent
18ed3f5040
commit
9e3a00825b
7 changed files with 348 additions and 27 deletions
|
|
@ -1,9 +1,9 @@
|
|||
//! `issue edit <number> [--title <t>] [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/<n>` 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/<n>` 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<String>,
|
||||
pub(crate) title: Option<String>,
|
||||
/// Inline body text (omit to leave unchanged).
|
||||
#[arg(long, conflicts_with = "body_file")]
|
||||
body: Option<String>,
|
||||
pub(crate) body: Option<String>,
|
||||
/// Read body from a file. `-` means stdin.
|
||||
#[arg(long = "body-file")]
|
||||
body_file: Option<String>,
|
||||
pub(crate) body_file: Option<String>,
|
||||
/// New state.
|
||||
#[arg(long, value_enum)]
|
||||
state: Option<StateArg>,
|
||||
pub(crate) state: Option<StateArg>,
|
||||
/// Milestone id (0 to unset).
|
||||
#[arg(long)]
|
||||
milestone: Option<u64>,
|
||||
pub(crate) milestone: Option<u64>,
|
||||
}
|
||||
|
||||
/// 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<Option<String>> {
|
||||
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<String>) -> Result<()> {
|
||||
let payload = EditIssueOption {
|
||||
assignee: None,
|
||||
assignees: None,
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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)?;
|
||||
|
|
|
|||
134
hive-forge/src/verbs/pr_edit.rs
Normal file
134
hive-forge/src/verbs/pr_edit.rs
Normal file
|
|
@ -0,0 +1,134 @@
|
|||
//! `pr edit <number> [--title <t>] [body sources] [--state s]
|
||||
//! [--milestone id] [--base <branch>]` — partial update of a PR. Omitted
|
||||
//! fields keep their current value.
|
||||
//!
|
||||
//! Without `--base` this is `issue edit` unchanged (`/issues/<n>`). The
|
||||
//! base branch only exists on Forgejo's pull-request edit endpoint
|
||||
//! (`PATCH /pulls/<n>`), 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<String>,
|
||||
}
|
||||
|
||||
/// `--milestone 0` means "unset" on the issue endpoint; whether
|
||||
/// `PATCH /pulls/<n>` 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/<n>` payload, or `None` when `--base` is unset and the
|
||||
/// edit rides the issue endpoint instead.
|
||||
fn pull_payload(args: &PrEditArgs, body: Option<String>) -> Result<Option<EditPullRequestOption>> {
|
||||
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<serde_json::Value> {
|
||||
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());
|
||||
}
|
||||
}
|
||||
167
hive-forge/tests/pr_edit.rs
Normal file
167
hive-forge/tests/pr_edit.rs
Normal file
|
|
@ -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<Mutex<Vec<(String, String, serde_json::Value)>>>;
|
||||
|
||||
/// 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::<u64>().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 <args>` 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:?}");
|
||||
}
|
||||
Loading…
Reference in a new issue