diff --git a/docs/tools/forge.md b/docs/tools/forge.md index aa87d698..4970fe63 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -344,16 +344,9 @@ 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) - 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. + 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 diff --git a/hive-forge/src/verbs/list.rs b/hive-forge/src/verbs/list.rs index 0f0332a5..042ad1be 100644 --- a/hive-forge/src/verbs/list.rs +++ b/hive-forge/src/verbs/list.rs @@ -26,9 +26,8 @@ use anyhow::Result; use clap::{Args as ClapArgs, ValueEnum}; -use forgejo_api::StructureError; use forgejo_api::structs::{ - Issue, IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, + IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, }; use serde_json::Value; @@ -172,12 +171,10 @@ 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.total, - headers.has_next, + headers.x_total_count.and_then(|t| u64::try_from(t).ok()), count, args.page, args.limit, @@ -211,88 +208,47 @@ 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, 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. +/// 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. /// -/// `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 { +/// `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 whenever more pages can +/// exist at all, `count` **is** the page size and `page * count` is how +/// many rows have been shown up to here. +/// +/// 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 { // 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) => 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}." - )), + Some(total) => { + let shown = page.saturating_mul(count); + 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. None => (count == limit).then(|| { format!( - "… {count} shown (page {page}); more results may exist — re-run with --page {next}." + "… {count} shown (page {page}); more results may exist — re-run with --page {}.", + page + 1 ) }), } @@ -520,8 +476,11 @@ mod tests { #[test] fn trailer_fires_on_a_clamped_page_the_old_heuristic_missed() { - // `--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"); + // 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"); assert!(msg.contains("50 of 187"), "{msg}"); assert!(msg.contains("137 more"), "{msg}"); assert!(msg.contains("--page 2"), "{msg}"); @@ -530,87 +489,29 @@ 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), true, 50, 3, 400).expect("187 > 150"); + let msg = trailer(Some(187), 50, 3, 400).expect("187 > 150"); assert!(msg.contains("150 of 187"), "{msg}"); assert!(msg.contains("37 more"), "{msg}"); } #[test] - 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_silent_once_the_forge_reports_no_next_page() { - assert_eq!(trailer(Some(60), false, 10, 6, 10), None); + 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), 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); + 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); } #[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"); + fn trailer_falls_back_to_the_full_page_heuristic_without_the_header() { + let msg = trailer(None, 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, false, 12, 1, 30), None); - } - - #[test] - fn trailer_counts_rows_shown_on_first_and_middle_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), 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}"); - } - - #[test] - fn trailer_short_last_page_prints_no_hint() { - 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 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); + assert_eq!(trailer(None, 12, 1, 30), None); } }