diff --git a/hive-forge/src/verbs/attach.rs b/hive-forge/src/verbs/attach.rs index ec05f23b..8819a879 100644 --- a/hive-forge/src/verbs/attach.rs +++ b/hive-forge/src/verbs/attach.rs @@ -51,7 +51,7 @@ pub fn run_issue(client: &Client, args: IssueArgs) -> Result<()> { .api() .issue_create_issue_attachment(owner, name, index(args.number)?, &bytes, query) .send()?; - println!("{}", require_url(&resp)?); + print_url(&resp); Ok(()) } @@ -78,7 +78,7 @@ pub fn run_comment(client: &Client, args: CommentArgs) -> Result<()> { .api() .issue_create_issue_comment_attachment(owner, name, index(args.id)?, &bytes, query) .send()?; - println!("{}", require_url(&resp)?); + print_url(&resp); Ok(()) } @@ -93,68 +93,8 @@ fn file_name(path: &Path) -> Option { path.file_name().map(|n| n.to_string_lossy().into_owned()) } -/// The uploaded attachment's browser download URL, hard-erroring if the -/// forge answered 2xx with none. A caller piping the printed URL (e.g. -/// `url=$(hive-forge attach-issue …)`) used to get an empty string and no -/// signal when this was `None` — the upload had happened, but nothing said -/// so, and nothing said it hadn't. -fn require_url(v: &Attachment) -> Result { - v.browser_download_url.clone().ok_or_else(|| { - anyhow::anyhow!( - "hive-forge attach: upload succeeded but the forge returned no download URL \ - (attachment {})", - attachment_ident(v) - ) - }) -} - -/// Best-effort identifier for an attachment error message — id if present, -/// else name, else a placeholder so the message never comes out empty. -fn attachment_ident(v: &Attachment) -> String { - match (v.id, v.name.as_deref()) { - (Some(id), Some(n)) => format!("id={id} name={n}"), - (Some(id), None) => format!("id={id}"), - (None, Some(n)) => format!("name={n}"), - (None, None) => "".to_owned(), - } -} - -#[cfg(test)] -mod tests { - use super::require_url; - use forgejo_api::structs::Attachment; - - fn attachment(url: Option<&str>) -> Attachment { - Attachment { - browser_download_url: url.map(|u| u.parse().unwrap()), - created_at: None, - download_count: None, - id: Some(7), - name: Some("out.log".to_owned()), - size: None, - r#type: None, - uuid: None, - } - } - - #[test] - fn control_present_url_is_returned() { - let a = attachment(Some("https://forge.example/a/1")); - assert_eq!( - require_url(&a).unwrap().as_str(), - "https://forge.example/a/1" - ); - } - - #[test] - fn missing_url_errors_naming_the_attachment() { - // A 2xx response with no browser_download_url used to print nothing - // and exit 0 — a caller doing `url=$(hive-forge attach-issue …)` - // got an empty string with no signal anything went wrong. - let a = attachment(None); - let err = require_url(&a).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("id=7"), "missing attachment id: {msg}"); - assert!(msg.contains("out.log"), "missing attachment name: {msg}"); +fn print_url(v: &Attachment) { + if let Some(url) = &v.browser_download_url { + println!("{url}"); } } diff --git a/hive-forge/src/verbs/labels.rs b/hive-forge/src/verbs/labels.rs index 41439111..77f6bcab 100644 --- a/hive-forge/src/verbs/labels.rs +++ b/hive-forge/src/verbs/labels.rs @@ -69,36 +69,22 @@ pub fn run(client: &Client, args: Args) -> Result<()> { bail!("hive-forge labels remove: pass at least one label name"); } let all = repo_labels(client)?; - // Same strict resolution as `add` — a typo used to be silently - // skipped here instead of refused. - let ids = resolve_ids(&all, &labels)?; - for id in ids { - client - .api() - .issue_remove_label( - owner, - name, - idx, - &id.to_string(), - DeleteLabelsOption { updated_at: None }, - ) - .send()?; + for label in &labels { + if let Some(id) = lookup_id(&all, label) { + let _ = client + .api() + .issue_remove_label( + owner, + name, + idx, + &id.to_string(), + DeleteLabelsOption { updated_at: None }, + ) + .send(); + } } - let current = client.api().issue_get_labels(owner, name, idx).send()?; - // Forgejo can 200 a remove it silently didn't apply (e.g. - // insufficient permission), same as the assignee case in - // `assign.rs` — diff the result against the intent instead of - // trusting the status code. - let still_present = still_present_names(&labels, ¤t); - if !still_present.is_empty() { - bail!( - "label(s) still present on #{} after remove — the forge \ - rejected the change: {}", - args.number, - still_present.join(", ") - ); - } - print_label_names(¤t); + let labels = client.api().issue_get_labels(owner, name, idx).send()?; + print_label_names(&labels); } } Ok(()) @@ -176,21 +162,6 @@ fn lookup_id(all: &[Label], name: &str) -> Option { .and_then(|l| l.id) } -/// Which of `requested` names are still present in `current` — the -/// remove-verb's post-condition check. A non-empty result means the forge -/// didn't actually drop those labels even though the call returned 200. -fn still_present_names(requested: &[String], current: &[Label]) -> Vec { - requested - .iter() - .filter(|r| { - current - .iter() - .any(|l| l.name.as_deref() == Some(r.as_str())) - }) - .cloned() - .collect() -} - fn print_label_names(labels: &[Label]) { let names: Vec<&str> = labels.iter().filter_map(|l| l.name.as_deref()).collect(); let _ = print_json(&json!(names)); @@ -198,7 +169,7 @@ fn print_label_names(labels: &[Label]) { #[cfg(test)] mod tests { - use super::{resolve_ids, still_present_names}; + use super::resolve_ids; use forgejo_api::structs::Label; fn label(id: i64, name: &str) -> Label { @@ -235,35 +206,4 @@ mod tests { let err = resolve_ids(&[], &["anything".to_owned()]).unwrap_err(); assert!(err.to_string().contains("(none)")); } - - #[test] - fn unresolved_remove_name_is_refused_like_add() { - // `remove` resolves names through the same `resolve_ids` as `add`, - // so a typo is refused up front instead of silently doing nothing. - let all = vec![label(1, "area/ops")]; - let err = resolve_ids(&all, &["area:ops".to_owned()]).unwrap_err(); - assert!(err.to_string().contains("area:ops")); - } - - #[test] - fn still_present_control_all_removed_reports_none() { - // Happy path: none of the requested names remain, so the caller - // should not bail. - let requested = vec!["area/ops".to_owned(), "type/bug".to_owned()]; - let current = vec![label(3, "state/open")]; - assert!(still_present_names(&requested, ¤t).is_empty()); - } - - #[test] - fn still_present_names_reports_labels_the_forge_kept() { - // A remove the forge silently rejected — the label is still on the - // issue after the "successful" call. This is the case the old code - // (`let _ = …send();` with no re-check) let through as success. - let requested = vec!["area/ops".to_owned(), "type/bug".to_owned()]; - let current = vec![label(1, "area/ops")]; - assert_eq!( - still_present_names(&requested, ¤t), - vec!["area/ops".to_owned()] - ); - } }