diff --git a/docs/tools/forge.md b/docs/tools/forge.md index efdefd7d..8ad403cf 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -341,4 +341,18 @@ to discover valid label names before triaging or to audit the label set. 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. +- `repo-search` reads `X-Total-Count` the same way `list` does and warns + with the real remaining count when it's present (`… 50 of 187 shown … + 137 more exist`); without it (an older forge, a stripping proxy) it + falls back to the cruder `count == limit` heuristic, same + trust-the-trailer-not-the-row-count principle as `list` above. Unlike + `list`, this verb has no `--page` — so once the forge's own page cap + has clamped the result (`count < limit`), raising `--limit` again asks + for the same clamp and gets nothing new; the warning says to narrow + `--query` instead in that case, and only suggests raising `--limit` + while still under the cap. Either way it can't tell "doesn't exist" + from "you lack permission to see it" — both render as an absent row + with no signal either way — and it matches repo **name** only, not + owner, so there's no way to enumerate an org's repos through this + verb. diff --git a/hive-forge/src/verbs/repo_search.rs b/hive-forge/src/verbs/repo_search.rs index 5ad952c8..8b492481 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -7,14 +7,58 @@ //! //! Human output: one `owner/repo description` line per hit. `--json` //! emits the full `SearchResults.data` array as raw JSON. +//! +//! Like `list`, the forge clamps the page size to its own +//! `api.MAX_RESPONSE_ITEMS` (50 by default), so a large `--limit` silently +//! returns a smaller page. Unlike `list`'s generated binding, this crate's +//! `repo_search` method does not expose the response headers — but +//! Forgejo's own docs name `/repos/search` as the worked example for +//! `X-Total-Count`, so [`response_type`][forgejo_api::sync::Request::response_type] +//! reinterprets the response as `(TotalCount, SearchResults)` to read it +//! anyway, same number `list`'s trailer reports. When the header is +//! missing or unparseable (an older forge, a stripping proxy) we fall back +//! to the cruder `count == limit` heuristic, which under-reports exactly +//! when the server clamped — the same fallback-not-rule relationship +//! `list.rs`'s `trailer()` documents. Either way we warn on stderr rather +//! than let a capped list read as complete; stdout — and `--json` stdout +//! especially — stays exactly the raw result set, so a script piping it +//! doesn't have to parse prose out of its own data. +//! +//! The total (when present) still can't tell "doesn't exist" from "you +//! lack permission to see it" — a hidden repo is just absent from both the +//! count and the rows, with no signal either way. use anyhow::Result; use clap::Args as ClapArgs; -use forgejo_api::structs::RepoSearchQuery; +use forgejo_api::StructureError; +use forgejo_api::structs::{RepoSearchQuery, SearchResults}; use crate::client::Client; use crate::verbs::print_json; +/// `X-Total-Count`, read generically via `response_type` since this +/// crate's `repo_search` binding (unlike `issue_list_issues`'s) has no +/// headers-carrying `Response` type of its own. Mirrors the header's own +/// error handling (`forgejo_api`'s `IssueListHeaders`) exactly, so a +/// non-ASCII or unparseable header fails the request the same way there. +struct TotalCount(Option); + +impl TryFrom<&reqwest::header::HeaderMap> for TotalCount { + type Error = StructureError; + + fn try_from(map: &reqwest::header::HeaderMap) -> Result { + let total = map + .get("x-total-count") + .map(|v| -> Result<_, StructureError> { + let s = v.to_str().map_err(|_| StructureError::HeaderNotAscii)?; + s.parse::() + .map_err(|_| StructureError::HeaderParseFailed) + }) + .transpose()?; + Ok(Self(total)) + } +} + #[derive(ClapArgs)] pub struct Args { /// Keyword to search for (matches repo name by default; combine with @@ -40,7 +84,7 @@ pub struct Args { /// serialisation errors when emitting `--json` output. pub fn run(client: &Client, args: Args) -> Result<()> { let limit = u32::try_from(args.limit.min(u64::from(u32::MAX))).unwrap_or(u32::MAX); - let results = client + let (TotalCount(total), results) = client .api() .repo_search(RepoSearchQuery { q: args.query, @@ -62,9 +106,13 @@ pub fn run(client: &Client, args: Args) -> Result<()> { order: None, }) .page_size(limit) + .response_type::<(TotalCount, SearchResults)>() .send()?; let repos = results.data.unwrap_or_default(); + if let Some(msg) = truncation_warning(total, repos.len() as u64, args.limit) { + eprintln!("{msg}"); + } if client.json_mode() { return print_json(&serde_json::to_value(&repos)?); } @@ -83,3 +131,120 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } Ok(()) } + +/// Tell the caller, on stderr, whether this page is the whole answer. +/// +/// `total` is `X-Total-Count` when the header parsed; `repo-search` always +/// starts from page 1 (it has no `--page`, unlike `list`), so `count` alone +/// is how much of `total` has been shown — no page-number multiply needed. +/// +/// Without a usable total, falls back to the cruder `count == limit` +/// heuristic: a hit on the cap, not necessarily the whole answer, but a +/// short result *is* the complete answer. This is the case the server's +/// own clamp (`api.MAX_RESPONSE_ITEMS`, 50 by default) makes silently +/// wrong — `--limit 400` returning a clamped 50 reads as complete here, +/// which is exactly why the header-based path above is the rule and this +/// is only the fallback (same relationship as `list.rs`'s `trailer()`). +fn truncation_warning(total: Option, count: u64, limit: u64) -> Option { + if count == 0 { + // "(no results)" is printed separately in `run` — a truncation + // warning here would wrongly imply more might still exist. + return None; + } + match total { + Some(total) => { + let remaining = total.saturating_sub(count); + (remaining > 0).then(|| { + // `count < limit` means the forge clamped the page below what + // was asked for — raising `--limit` again asks for the same + // clamp and gets the same `count` back. Unlike `list`, this + // verb has no `--page` to walk past the clamp with, so + // narrowing `--query` is the only remedy that can work once + // the server has capped the page. "raise --limit" only + // helps the other case: still under the cap, where asking + // for more actually serves more of it. + let remedy = if count < limit { + "the forge capped this page at its own limit — narrow --query to see them" + } else { + "raise --limit or narrow --query to see them" + }; + format!( + "warning: {count} of {total} results shown — {remaining} more exist (the \ + forge still silently drops any repo you lack permission to see, with no \ + signal either way); {remedy}" + ) + }) + } + None => (count == limit).then(|| { + format!( + "warning: got exactly --limit ({limit}) results — the forge may be holding back \ + more (and silently drops any repo you lack permission to see, with no signal \ + either way); raise --limit or narrow --query rather than trusting this as a \ + complete list" + ) + }), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn warns_with_the_real_remaining_count_when_the_total_is_known() { + let msg = truncation_warning(Some(187), 30, 30).expect("137 more exist"); + assert!(msg.contains("30 of 187"), "{msg}"); + assert!(msg.contains("157 more"), "{msg}"); + } + + #[test] + fn catches_the_clamp_the_old_count_eq_limit_heuristic_missed() { + // The regression this header read exists for: `--limit 400` against + // a forge that clamps to 50, with 187 matches. `count != limit` + // (50 != 400), so the old heuristic alone stayed silent here. + let msg = truncation_warning(Some(187), 50, 400).expect("137 more exist"); + assert!(msg.contains("50 of 187"), "{msg}"); + assert!(msg.contains("137 more"), "{msg}"); + } + + #[test] + fn clamp_case_tells_the_caller_to_narrow_not_to_raise_limit() { + // `--limit` already exceeded the forge's own page cap here, so + // "raise --limit" is a dead end — only `--query` can still narrow + // the result set below the cap. (The bug argus caught: both + // branches used to say "raise --limit", which can't work once the + // server itself has clamped the page.) + let msg = truncation_warning(Some(187), 50, 400).expect("137 more exist"); + assert!(msg.contains("narrow --query"), "{msg}"); + assert!(!msg.contains("raise --limit"), "{msg}"); + } + + #[test] + fn under_the_cap_case_still_offers_raise_limit() { + // `count == limit` and both are under the forge's cap: asking for + // more can actually serve more, so "raise --limit" is live advice + // here, not a dead end. + let msg = truncation_warning(Some(187), 30, 30).expect("157 more exist"); + assert!(msg.contains("raise --limit"), "{msg}"); + } + + #[test] + fn stays_quiet_once_the_total_is_accounted_for() { + assert_eq!(truncation_warning(Some(30), 30, 30), None); + } + + #[test] + fn falls_back_to_the_cap_heuristic_without_a_usable_total() { + assert!(truncation_warning(None, 30, 30).is_some()); + assert_eq!(truncation_warning(None, 9, 30), None); + } + + #[test] + fn stays_quiet_on_zero_results_even_with_a_total() { + // Permission-filtering can make count 0 while total is nonzero + // (every match is hidden from this caller) — still not a + // truncation warning's job; "(no results)" covers it in `run`. + assert_eq!(truncation_warning(Some(5), 0, 30), None); + assert_eq!(truncation_warning(None, 0, 30), None); + } +}