diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 9c420f14..aa87d698 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -344,14 +344,16 @@ to discover valid label names before triaging or to audit the label set. - `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 (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. The trailer - reports the rows shown so far, out of the total when the forge sent - one, plus a `re-run with --page N+1` hint. The last page prints no - trailer and no hint, so silence means the list is complete. Without - `X-Total-Count` it falls back to `N shown … more results may exist` - on a full page. + is driven by the forge's own `Link: rel="next"` / `X-HasMore` + signal, so it stays right when the forge clamps `--limit`: while a next + page exists it prints the rows shown so far (out of the response's + `X-Total-Count` when sent, `… 50 of 187 shown … 137 more`) and a + `re-run with --page N+1` hint. The last page prints no trailer, so + silence means the list is complete. Two cases stay ambiguous: without + `X-Total-Count` the headers didn't come through, and it falls back to + `N shown … more results may exist` on a page of exactly `--limit` rows + (silent on a clamped one); a proxy that strips only `Link` while + keeping `X-Total-Count` reads as a last page. - `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 diff --git a/hive-forge/src/verbs/list.rs b/hive-forge/src/verbs/list.rs index efccc1ee..0f0332a5 100644 --- a/hive-forge/src/verbs/list.rs +++ b/hive-forge/src/verbs/list.rs @@ -26,8 +26,9 @@ use anyhow::Result; use clap::{Args as ClapArgs, ValueEnum}; +use forgejo_api::StructureError; use forgejo_api::structs::{ - IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, + Issue, IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, }; use serde_json::Value; @@ -171,10 +172,12 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .issue_list_issues(owner, name, query) .page(u32::try_from(args.page).unwrap_or(u32::MAX)) .page_size(u32::try_from(args.limit).unwrap_or(u32::MAX)) + .response_type::<(PageHeaders, Vec)>() .send()?; let count = issues.len() as u64; let note = trailer( - headers.x_total_count.and_then(|t| u64::try_from(t).ok()), + headers.total, + headers.has_next, count, args.page, args.limit, @@ -208,58 +211,88 @@ pub fn run(client: &Client, args: Args) -> Result<()> { Ok(()) } +/// Pagination headers of the list response, read through `response_type` +/// because the generated `IssueListHeaders` carries `X-Total-Count` only. +struct PageHeaders { + total: Option, + /// `Link: rel="next"` present, or `X-HasMore: true`. + has_next: bool, +} + +impl TryFrom<&reqwest::header::HeaderMap> for PageHeaders { + type Error = StructureError; + + fn try_from(map: &reqwest::header::HeaderMap) -> Result { + let ascii = |name: &str| { + map.get(name) + .map(|v| v.to_str().map_err(|_| StructureError::HeaderNotAscii)) + .transpose() + }; + let total = ascii("x-total-count")? + .map(|s| { + s.parse::() + .map_err(|_| StructureError::HeaderParseFailed) + }) + .transpose()?; + let link_next = ascii("link")?.is_some_and(|l| { + l.split(',') + .any(|part| part.split(';').skip(1).any(|p| p.trim() == "rel=\"next\"")) + }); + let has_more = ascii("x-hasmore")?.is_some_and(|v| v.trim().eq_ignore_ascii_case("true")); + Ok(Self { + total, + has_next: link_next || has_more, + }) + } +} + /// Tell the caller, on stderr, whether this page is the whole answer. /// /// The forge clamps `page_size` to its own `api.MAX_RESPONSE_ITEMS`, so -/// the requested `limit` is not the page size that was served: with -/// `--limit 400` a full clamped page of 50 comes back, and the old -/// `count == limit` test stayed silent — suppressing the warning in the -/// one case where the truncation is invisible. Hence the header. +/// the requested `limit` is not the page size that was served, and the +/// served size is not reported. The forge's own pagination signal doesn't +/// need it: `has_next` (`Link: rel="next"` / `X-HasMore`) is present +/// exactly when more rows follow. No `has_next` ⇒ last page ⇒ silent. With +/// it, this page is full, so `count` **is** the served page size and every +/// earlier page had it too: `page * count` rows have been shown. /// -/// `total` is `X-Total-Count`, which Forgejo sends on this endpoint. The -/// *effective* page size isn't reported, but it doesn't need to be: a -/// short page is by definition the last one, so on a page that is not the -/// last, `count` **is** the page size and `page * count` is how many rows -/// have been shown up to here. On the last page `count` is only the -/// remainder, so `page * count` undercounts; there the unclamped -/// `(page - 1) * limit + count` lands exactly on `total`, which identifies -/// the page as the last one. -/// -/// Deliberately not clamping `--limit` client-side to 50: that number is -/// the *remote's* configuration, and baking a peer's setting into the -/// client is how you get a value that's wrong on the one deployment that -/// changed it. -fn trailer(total: Option, count: u64, page: u64, limit: u64) -> Option { +/// `total` is `X-Total-Count`; it only enriches the message. When it is +/// absent the response headers did not come through (older forge, a proxy +/// that stripped them), so `has_next` can't be trusted either and the +/// `count == limit` heuristic applies. It under-reports when the server +/// clamped, which is why it is the fallback and not the rule. A proxy that +/// strips only `Link` while keeping `X-Total-Count` is not detectable and +/// reads as a last page. +fn trailer( + total: Option, + has_next: bool, + count: u64, + page: u64, + limit: u64, +) -> Option { // An empty page is the end, whatever else is true. if count == 0 { return None; } + let next = page + 1; + let shown = page.saturating_mul(count); match total { - Some(total) => { - let by_limit = page - .saturating_sub(1) - .saturating_mul(limit) - .saturating_add(count); - let shown = if by_limit == total { - total - } else { - page.saturating_mul(count).min(total) - }; - let remaining = total.saturating_sub(shown); - (remaining > 0).then(|| { - format!( - "… {shown} of {total} shown (page {page}, {count}/page) — {remaining} more; re-run with --page {}.", - page + 1 - ) - }) - } - // No header (older forge, or a proxy that dropped it): fall back - // to the full-page heuristic. It under-reports when the server - // clamped, which is why it is the fallback and not the rule. + Some(total) => has_next.then(|| { + match total.checked_sub(shown).filter(|r| *r > 0) { + Some(remaining) => format!( + "… {shown} of {total} shown (page {page}, {count}/page) — {remaining} more; re-run with --page {next}." + ), + None => format!( + "… {shown} shown (page {page}, {count}/page); more results exist — re-run with --page {next}." + ), + } + }), + None if has_next => Some(format!( + "… {shown} shown (page {page}, {count}/page); more results exist — re-run with --page {next}." + )), None => (count == limit).then(|| { format!( - "… {count} shown (page {page}); more results may exist — re-run with --page {}.", - page + 1 + "… {count} shown (page {page}); more results may exist — re-run with --page {next}." ) }), } @@ -487,11 +520,8 @@ mod tests { #[test] fn trailer_fires_on_a_clamped_page_the_old_heuristic_missed() { - // The regression this function exists for: `--limit 400` against - // a forge that clamps to 50, with 187 matches. `count != limit`, - // so the pre-header code printed nothing and the caller concluded - // the list was complete. - let msg = trailer(Some(187), 50, 1, 400).expect("must warn about the other 137"); + // `--limit 400` against a forge that clamps to 50, 187 matches. + let msg = trailer(Some(187), true, 50, 1, 400).expect("must warn about the other 137"); assert!(msg.contains("50 of 187"), "{msg}"); assert!(msg.contains("137 more"), "{msg}"); assert!(msg.contains("--page 2"), "{msg}"); @@ -500,38 +530,55 @@ mod tests { #[test] fn trailer_counts_from_the_served_page_size_not_the_requested_one() { // Page 3 of a clamped 50/page walk has shown 150, not 3 * 400. - let msg = trailer(Some(187), 50, 3, 400).expect("187 > 150"); + let msg = trailer(Some(187), true, 50, 3, 400).expect("187 > 150"); assert!(msg.contains("150 of 187"), "{msg}"); assert!(msg.contains("37 more"), "{msg}"); } #[test] - fn trailer_silent_once_the_total_is_accounted_for() { - // Last page, short: 2 * 50 + ... -> 60 shown of 60. - assert_eq!(trailer(Some(60), 10, 6, 10), None); - // Exactly-full last page: no phantom "page 4". - assert_eq!(trailer(Some(150), 50, 3, 50), None); - // Empty page is the end even if the total disagrees (a filter - // the forge discarded, a concurrent close) — never advertise a - // next page we just proved is empty. - assert_eq!(trailer(Some(999), 0, 9, 30), None); + fn trailer_clamped_page_whose_arithmetic_looks_like_the_last_page() { + // `--limit 100` clamped to 50/page, 150 rows, page 2: + // `(page - 1) * limit + count` equals the total, yet rows 101-150 + // are on page 3 and the forge says so. + let msg = trailer(Some(150), true, 50, 2, 100).expect("page 3 remains"); + assert!(msg.contains("100 of 150"), "{msg}"); + assert!(msg.contains("50 more"), "{msg}"); + assert!(msg.contains("--page 3"), "{msg}"); + // Control: the same shape on the real last page is silent. + assert_eq!(trailer(Some(150), false, 50, 3, 100), None); } #[test] - fn trailer_falls_back_to_the_full_page_heuristic_without_the_header() { - let msg = trailer(None, 30, 1, 30).expect("full page, unknown total"); + fn trailer_silent_once_the_forge_reports_no_next_page() { + assert_eq!(trailer(Some(60), false, 10, 6, 10), None); + // Exactly-full last page: no phantom "page 4". + assert_eq!(trailer(Some(150), false, 50, 3, 50), None); + // Empty page is the end even if the forge claims a next page. + assert_eq!(trailer(Some(999), true, 0, 9, 30), None); + } + + #[test] + fn trailer_next_page_without_a_total_still_hints() { + let msg = trailer(None, true, 50, 2, 400).expect("next page exists"); + assert!(msg.contains("100 shown"), "{msg}"); + assert!(msg.contains("--page 3"), "{msg}"); + } + + #[test] + fn trailer_falls_back_to_the_full_page_heuristic_without_headers() { + let msg = trailer(None, false, 30, 1, 30).expect("full page, unknown total"); assert!(msg.contains("30 shown (page 1)"), "{msg}"); assert!(msg.contains("--page 2"), "{msg}"); // Short page with no header: the end, as far as we can tell. - assert_eq!(trailer(None, 12, 1, 30), None); + assert_eq!(trailer(None, false, 12, 1, 30), None); } #[test] fn trailer_counts_rows_shown_on_first_and_middle_pages() { - let msg = trailer(Some(1331), 50, 1, 50).expect("more pages"); + let msg = trailer(Some(1331), true, 50, 1, 50).expect("more pages"); assert!(msg.contains("50 of 1331 shown"), "{msg}"); assert!(msg.contains("1281 more"), "{msg}"); - let msg = trailer(Some(1331), 50, 26, 50).expect("one page left"); + let msg = trailer(Some(1331), true, 50, 26, 50).expect("one page left"); assert!(msg.contains("1300 of 1331 shown"), "{msg}"); assert!(msg.contains("31 more"), "{msg}"); assert!(msg.contains("--page 27"), "{msg}"); @@ -539,27 +586,31 @@ mod tests { #[test] fn trailer_short_last_page_prints_no_hint() { - // Page 27 of 1331 rows at 50/page holds 31: `page * count` gave - // 837 of 1331 and a hint to fetch page 28. - assert_eq!(trailer(Some(1331), 31, 27, 50), None); - // Page 2 of 2: `2 * 5 = 10 < 55` also produced a bogus hint. - assert_eq!(trailer(Some(55), 5, 2, 50), None); - // Control: the page before the last still hints. - assert!(trailer(Some(1331), 50, 26, 50).is_some()); + assert_eq!(trailer(Some(1331), false, 31, 27, 50), None); + assert_eq!(trailer(Some(55), false, 5, 2, 50), None); + } + + fn headers(pairs: &[(&'static str, &'static str)]) -> PageHeaders { + let mut map = reqwest::header::HeaderMap::new(); + for (k, v) in pairs { + map.insert(*k, reqwest::header::HeaderValue::from_static(v)); + } + PageHeaders::try_from(&map).expect("parses") } #[test] - fn trailer_exact_multiple_last_page_is_silent() { - assert_eq!(trailer(Some(100), 50, 2, 50), None); - // Control: page 1 of the same list still hints. - let msg = trailer(Some(100), 50, 1, 50).expect("page 2 remains"); - assert!(msg.contains("50 of 100 shown"), "{msg}"); - } - - #[test] - fn trailer_without_total_is_silent_on_a_short_page() { - assert_eq!(trailer(None, 31, 27, 50), None); - // Control: a full page still hints. - assert!(trailer(None, 50, 27, 50).is_some()); + fn page_headers_read_next_from_link_or_has_more() { + let link = + r#"; rel="next", ; rel="last""#; + let h = headers(&[("x-total-count", "150"), ("link", link)]); + assert_eq!(h.total, Some(150)); + assert!(h.has_next); + assert!(headers(&[("x-hasmore", "true")]).has_next); + // Controls: first/prev links alone, or HasMore false, are not "next". + let prev = + r#"; rel="first", ; rel="prev""#; + assert!(!headers(&[("link", prev)]).has_next); + assert!(!headers(&[("x-hasmore", "false")]).has_next); + assert!(!headers(&[]).has_next); } }