diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 70c8f539..9f471541 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -336,8 +336,8 @@ to discover valid label names before triaging or to audit the label set. isn't a current member isn't necessarily a typo. - `list --limit N` is a *request*: the forge clamps page size to its own `api.MAX_RESPONSE_ITEMS` (50 by default), so `--limit 400` returns at - most 50 rows. The stderr trailer reports the real total from the - response's `X-Total-Count` (`… 50 of 187 shown … 137 more`), so trust - the trailer, not the row count, when deciding whether you've seen - everything. + most 50 rows. The stderr trailer (printed in `--json` mode too) + reports the real total from the response's `X-Total-Count` + (`… 50 of 187 shown … 137 more`), so trust the trailer, not the row + count, when deciding whether you've seen everything. diff --git a/hive-forge/src/verbs/branches.rs b/hive-forge/src/verbs/branches.rs index 0b4ec3fd..07fe2dfd 100644 --- a/hive-forge/src/verbs/branches.rs +++ b/hive-forge/src/verbs/branches.rs @@ -4,7 +4,7 @@ use anyhow::Result; use clap::Args as ClapArgs; use crate::client::Client; -use crate::verbs::refuse_json; +use crate::verbs::{all_pages, refuse_json}; #[derive(ClapArgs)] pub struct Args { @@ -15,11 +15,15 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { refuse_json(client.json_mode(), "branches", "only the plain name list")?; let (owner, name) = client.owner_repo()?; - let (_, branches) = client - .api() - .repo_list_branches(owner, name) - .page_size(100) - .send()?; + let branches = all_pages(|page, size| { + let (_, batch) = client + .api() + .repo_list_branches(owner, name) + .page(page) + .page_size(size) + .send()?; + Ok(batch) + })?; for branch in &branches { let Some(n) = branch.name.as_deref() else { continue; diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 0db9b078..66ee7826 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -38,7 +38,7 @@ use time::OffsetDateTime; use crate::client::{Client, index}; use crate::notify; use crate::verbs::{ - MAX_LIMIT, PAGE_SIZE, clamp_limit, comment_reactions, parse_rfc3339, print_json, + MAX_LIMIT, PAGE_SIZE, all_pages, clamp_limit, comment_reactions, parse_rfc3339, print_json, reaction_summary, rfc3339, }; @@ -355,12 +355,16 @@ fn fetch_review_bodies(client: &Client, number: u64) -> Vec { let Ok(idx) = index(number) else { return Vec::new(); }; - let reviews = client - .api() - .repo_list_pull_reviews(owner, name, idx) - .send() - .map(|(_, reviews)| reviews) - .unwrap_or_default(); + let reviews = all_pages(|page, size| { + let (_, batch) = client + .api() + .repo_list_pull_reviews(owner, name, idx) + .page(page) + .page_size(size) + .send()?; + Ok(batch) + }) + .unwrap_or_default(); reviews .into_iter() .filter_map(|r| { diff --git a/hive-forge/src/verbs/labels.rs b/hive-forge/src/verbs/labels.rs index 41439111..168e2e23 100644 --- a/hive-forge/src/verbs/labels.rs +++ b/hive-forge/src/verbs/labels.rs @@ -7,7 +7,7 @@ use forgejo_api::structs::{DeleteLabelsOption, IssueLabelsOption, IssueListLabel use serde_json::json; use crate::client::{Client, index}; -use crate::verbs::print_json; +use crate::verbs::{all_pages, print_json}; #[derive(ClapArgs)] pub struct Args { @@ -105,37 +105,24 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } /// The repo's whole label set, for name → id resolution. `pub(crate)` so -/// other verbs (`issue create`, `pr create`, `list`) can resolve a label -/// name without duplicating the lookup. +/// other verbs (`issue create`, `pr create`, `list`, `repo-labels`) share +/// one lookup. /// -/// Paginated, where this used to ask for one page of 100 — **a page size -/// with no page loop is a lie the size of the page.** Truncation here is -/// worse than it looks: it doesn't drop a result, it makes a *valid* -/// label past the cut fail to resolve, and then prints an "available -/// labels" list that is itself incomplete, so the error argues for the -/// typo. Same root as the `list` trailer bug (assuming one request -/// returns the whole population), opposite direction — that one was a -/// false positive, this is a false negative. +/// Every page, not one: a page size with no page loop silently truncates +/// the set, and a truncated set makes a *valid* label past the cut fail to +/// resolve, with an "available labels" list that is itself incomplete, so +/// the error argues for the typo. pub(crate) fn repo_labels(client: &Client) -> Result> { - /// Generous cap so a misbehaving server can't spin us forever. - const MAX_PAGES: u32 = 50; - const PAGE: u32 = 100; let (owner, name) = client.owner_repo()?; - let mut all = Vec::new(); - for page in 1..=MAX_PAGES { + all_pages(|page, size| { let (_, batch) = client .api() .issue_list_labels(owner, name, IssueListLabelsQuery::default()) .page(page) - .page_size(PAGE) + .page_size(size) .send()?; - let short = batch.len() < PAGE as usize; - all.extend(batch); - if short { - break; - } - } - Ok(all) + Ok(batch) + }) } /// Resolve label names to ids, hard-erroring if any name doesn't match an diff --git a/hive-forge/src/verbs/list.rs b/hive-forge/src/verbs/list.rs index cc9b34d1..042ad1be 100644 --- a/hive-forge/src/verbs/list.rs +++ b/hive-forge/src/verbs/list.rs @@ -173,10 +173,20 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .page_size(u32::try_from(args.limit).unwrap_or(u32::MAX)) .send()?; let count = issues.len() as u64; + let note = trailer( + headers.x_total_count.and_then(|t| u64::try_from(t).ok()), + count, + args.page, + args.limit, + ); let mut items = serde_json::to_value(issues)?; if client.json_mode() { hoist_merge_state(&mut items); - return print_json(&items); + print_json(&items)?; + if let Some(msg) = note { + eprintln!("{msg}"); + } + return Ok(()); } for item in items.as_array().into_iter().flatten() { let number = item.get("number").and_then(Value::as_u64); @@ -192,12 +202,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { blocks.as_deref().map_or(0, blocking_open_count), ); } - if let Some(msg) = trailer( - headers.x_total_count.and_then(|t| u64::try_from(t).ok()), - count, - args.page, - args.limit, - ) { + if let Some(msg) = note { eprintln!("{msg}"); } Ok(()) diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 491a152e..8acd1645 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -120,9 +120,99 @@ pub(crate) fn clamp_limit(requested: u64) -> (u64, bool) { (limit, limit < requested) } +/// [`PAGE_SIZE`] as the `u32` the generated client's `.page_size()` takes. +const PAGE_SIZE_U32: u32 = 50; + +/// Runaway cap for [`all_pages`]: at most `MAX_PAGES * PAGE_SIZE` rows. +const MAX_PAGES: u32 = 50; + +/// Every row of a paged forge listing. `fetch(page, page_size)` sends one +/// request; the walk stops at the first page shorter than `page_size`. +/// The page size is the forge's clamp, not a larger request: a clamped +/// full page would read as short and end the walk after one page. +/// Stopping at [`MAX_PAGES`] prints a warning on stderr, so a capped +/// result is never presented as the whole population. +pub(crate) fn all_pages(fetch: impl FnMut(u32, u32) -> Result>) -> Result> { + let (rows, capped) = walk_pages(MAX_PAGES, fetch)?; + if capped { + eprintln!( + "hive-forge: stopped after {MAX_PAGES} pages ({} rows); the result may be incomplete", + rows.len() + ); + } + Ok(rows) +} + +/// [`all_pages`]' loop, with the cap as a parameter. Returns the rows and +/// whether the walk hit `max_pages` without seeing a short page. +fn walk_pages( + max_pages: u32, + mut fetch: impl FnMut(u32, u32) -> Result>, +) -> Result<(Vec, bool)> { + let mut rows = Vec::new(); + for page in 1..=max_pages { + let batch = fetch(page, PAGE_SIZE_U32)?; + let short = batch.len() < PAGE_SIZE_U32 as usize; + rows.extend(batch); + if short { + return Ok((rows, false)); + } + } + Ok((rows, true)) +} + #[cfg(test)] mod page_limit_tests { - use super::{MAX_LIMIT, PAGE_SIZE, clamp_limit}; + use anyhow::Result; + + use super::{MAX_LIMIT, PAGE_SIZE, PAGE_SIZE_U32, clamp_limit, walk_pages}; + + /// A fake listing of `total` rows, served `page_size` at a time. + fn listing(total: usize) -> impl FnMut(u32, u32) -> Result> { + move |page, size| { + let (page, size) = (page as usize, size as usize); + Ok(((page - 1) * size..(page * size).min(total)).collect()) + } + } + + #[test] + fn page_size_u32_matches_page_size() { + assert_eq!(u64::from(PAGE_SIZE_U32), PAGE_SIZE); + } + + #[test] + fn walk_pages_collects_every_row_past_the_first_page() { + let (rows, capped) = walk_pages(50, listing(86)).unwrap(); + assert_eq!(rows, (0..86).collect::>()); + assert!(!capped); + } + + #[test] + fn walk_pages_stops_on_the_empty_page_after_an_exact_multiple() { + let mut calls = 0; + let mut inner = listing(100); + let (rows, capped) = walk_pages(50, |p, s| { + calls += 1; + inner(p, s) + }) + .unwrap(); + assert_eq!(rows.len(), 100); + assert_eq!(calls, 3); + assert!(!capped); + } + + #[test] + fn walk_pages_reports_hitting_the_cap() { + let (rows, capped) = walk_pages(2, listing(500)).unwrap(); + assert_eq!(rows.len(), 100); + assert!(capped); + } + + #[test] + fn walk_pages_propagates_a_fetch_error() { + let err = walk_pages::(5, |_, _| anyhow::bail!("boom")).unwrap_err(); + assert_eq!(err.to_string(), "boom"); + } #[test] fn clamp_limit_passes_small_requests_through() { diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index 38de7fe3..b00b4ffb 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -7,7 +7,7 @@ use forgejo_api::structs::CreatePullReviewOptions; use serde_json::{Value, json}; use crate::client::{Client, index}; -use crate::verbs::print_json; +use crate::verbs::{all_pages, print_json}; #[derive(ClapArgs)] pub struct Args { @@ -97,10 +97,16 @@ fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec /// List all reviews for a PR, dispatching to the appropriate output mode. fn list_reviews(client: &Client, number: u64) -> Result<()> { let (owner, name) = client.owner_repo()?; - let (_, reviews) = client - .api() - .repo_list_pull_reviews(owner, name, index(number)?) - .send()?; + let idx = index(number)?; + let reviews = all_pages(|page, size| { + let (_, batch) = client + .api() + .repo_list_pull_reviews(owner, name, idx) + .page(page) + .page_size(size) + .send()?; + Ok(batch) + })?; let reviews = serde_json::to_value(reviews)?; let reviews = reviews.as_array().cloned().unwrap_or_default(); if client.json_mode() { diff --git a/hive-forge/src/verbs/repo_labels.rs b/hive-forge/src/verbs/repo_labels.rs index b0d1de48..01826bd9 100644 --- a/hive-forge/src/verbs/repo_labels.rs +++ b/hive-forge/src/verbs/repo_labels.rs @@ -5,16 +5,10 @@ use anyhow::Result; use clap::Args as ClapArgs; -use forgejo_api::structs::{IssueListLabelsQuery, Label}; +use forgejo_api::structs::Label; use crate::client::Client; -use crate::verbs::print_json; - -/// Page size on the label list endpoint. -const PAGE_SIZE: u32 = 50; - -/// Runaway cap on label pagination — same ceiling the raw client used. -const MAX_PAGES: u32 = 10; +use crate::verbs::{labels, print_json}; #[derive(ClapArgs)] pub struct Args { @@ -23,23 +17,7 @@ pub struct Args { } pub fn run(client: &Client, args: Args) -> Result<()> { - let (owner, name) = client.owner_repo()?; - // Repos can carry more than one page of labels; paginate so the list - // is complete rather than capped at the first page. - let mut labels: Vec