diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 8ad403cf..efdefd7d 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -341,18 +341,4 @@ 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 8b492481..5ad952c8 100644 --- a/hive-forge/src/verbs/repo_search.rs +++ b/hive-forge/src/verbs/repo_search.rs @@ -7,58 +7,14 @@ //! //! 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::StructureError; -use forgejo_api::structs::{RepoSearchQuery, SearchResults}; +use forgejo_api::structs::RepoSearchQuery; 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 @@ -84,7 +40,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 (TotalCount(total), results) = client + let results = client .api() .repo_search(RepoSearchQuery { q: args.query, @@ -106,13 +62,9 @@ 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)?); } @@ -131,120 +83,3 @@ 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); - } -}