From acb5b676d76d91138ab6f45498794d55675b2187 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 14 Oct 2026 15:57:39 +0200 Subject: [PATCH] repo-search: read X-Total-Count so the warning fires at the server's own clamp --- docs/tools/forge.md | 11 ++- hive-forge/src/verbs/repo_search.rs | 142 ++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 35 deletions(-) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 62d7a81f..60987899 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -341,11 +341,12 @@ 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` has no `X-Total-Count` to fall back on, so it can only - warn, not quantify: a result count that exactly equals `--limit` - prints a stderr warning that more may exist, same trust-the-trailer - principle as `list` above but without a real number behind it. It - also can't tell "doesn't exist" from "you lack permission to see +- `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. 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 68fb2372..91a6eef2 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -8,21 +8,57 @@ //! Human output: one `owner/repo description` line per hit. `--json` //! emits the full `SearchResults.data` array as raw JSON. //! -//! A result count that exactly equals `--limit` is a hit on the cap, not -//! necessarily the whole answer — the forge may be holding back more (it -//! also silently drops repos the caller lacks permission to see, with no -//! distinguishing signal at all). Either way we warn on stderr rather than -//! let a capped list read as complete; stdout — and `--json` stdout +//! 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 @@ -48,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, @@ -70,10 +106,11 @@ 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(repos.len() as u64, args.limit) { + if let Some(msg) = truncation_warning(total, repos.len() as u64, args.limit) { eprintln!("{msg}"); } if client.json_mode() { @@ -95,20 +132,45 @@ pub fn run(client: &Client, args: Args) -> Result<()> { Ok(()) } -/// A result count that exactly equals `--limit` is a hit on the cap, not -/// necessarily the whole answer — see the module doc. Returns the stderr -/// warning to print, or `None` when the count came in under the cap (a -/// short result *is* the complete answer, same reasoning `list`'s own -/// `trailer()` uses for its full-page heuristic). -fn truncation_warning(count: u64, limit: u64) -> Option { - (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" - ) - }) +/// 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(|| { + 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); raise --limit or narrow --query to see them" + ) + }) + } + 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)] @@ -116,19 +178,39 @@ mod tests { use super::*; #[test] - fn warns_when_count_hits_the_cap() { - assert!(truncation_warning(30, 30).is_some()); + 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 stays_quiet_under_the_cap() { - assert_eq!(truncation_warning(9, 30), None); + 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 stays_quiet_on_zero_results() { - // An empty result needs "(no results)" (printed separately in - // `run`), not a truncation warning implying more might exist. - assert_eq!(truncation_warning(0, 30), None); + 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); } }