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<String> {
     path.file_name().map(|n| n.to_string_lossy().into_owned())
 }
 
-/// The uploaded attachment's browser download URL, hard-erroring if the
+fn print_url(v: &Attachment) {
-/// forge answered 2xx with none. A caller piping the printed URL (e.g.
+    if let Some(url) = &v.browser_download_url {
-/// `url=$(hive-forge attach-issue …)`) used to get an empty string and no
+        println!("{url}");
-/// 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<url::Url> {
-    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) => "<unknown>".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}");
     }
 }

hive-forge/src/verbs/labels.rs

@@ -69,11 +69,9 @@ 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
+            for label in &labels {
-            // skipped here instead of refused.
+                if let Some(id) = lookup_id(&all, label) {
-            let ids = resolve_ids(&all, &labels)?;
+                    let _ = client
-            for id in ids {
-                client
                         .api()
                         .issue_remove_label(
                             owner,
@@ -82,23 +80,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
                             &id.to_string(),
                             DeleteLabelsOption { updated_at: None },
                         )
-                    .send()?;
+                        .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, &current);
-            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(&current);
+            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<i64> {
         .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<String> {
-    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, &current).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, &current),
-            vec!["area/ops".to_owned()]
-        );
-    }
 }