hive-forge list trusts the forge's own next-page signal for its trailer

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`
+  is driven by the forge's own `Link: rel="next"` / `X-HasMore`
-  (`… 50 of 187 shown … 137 more`), so trust the trailer, not the row
+  signal, so it stays right when the forge clamps `--limit`: while a next
-  count, when deciding whether you've seen everything. The trailer
+  page exists it prints the rows shown so far (out of the response's
-  reports the rows shown so far, out of the total when the forge sent
+  `X-Total-Count` when sent, `… 50 of 187 shown … 137 more`) and a
-  one, plus a `re-run with --page N+1` hint. The last page prints no
+  `re-run with --page N+1` hint. The last page prints no trailer, so
-  trailer and no hint, so silence means the list is complete. Without
+  silence means the list is complete. Two cases stay ambiguous: without
-  `X-Total-Count` it falls back to `N shown … more results may exist`
+  `X-Total-Count` the headers didn't come through, and it falls back to
-  on a full page.
+  `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

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<Issue>)>()
         .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<u64>,
+    /// `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<Self, Self::Error> {
+        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::<u64>()
+                    .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
+/// the requested `limit` is not the page size that was served, and the
-/// `--limit 400` a full clamped page of 50 comes back, and the old
+/// served size is not reported. The forge's own pagination signal doesn't
-/// `count == limit` test stayed silent — suppressing the warning in the
+/// need it: `has_next` (`Link: rel="next"` / `X-HasMore`) is present
-/// one case where the truncation is invisible. Hence the header.
+/// 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
+/// `total` is `X-Total-Count`; it only enriches the message. When it is
-/// *effective* page size isn't reported, but it doesn't need to be: a
+/// absent the response headers did not come through (older forge, a proxy
-/// short page is by definition the last one, so on a page that is not the
+/// that stripped them), so `has_next` can't be trusted either and the
-/// last, `count` **is** the page size and `page * count` is how many rows
+/// `count == limit` heuristic applies. It under-reports when the server
-/// have been shown up to here. On the last page `count` is only the
+/// clamped, which is why it is the fallback and not the rule. A proxy that
-/// remainder, so `page * count` undercounts; there the unclamped
+/// strips only `Link` while keeping `X-Total-Count` is not detectable and
-/// `(page - 1) * limit + count` lands exactly on `total`, which identifies
+/// reads as a last page.
-/// the page as the last one.
+fn trailer(
-///
+    total: Option<u64>,
-/// Deliberately not clamping `--limit` client-side to 50: that number is
+    has_next: bool,
-/// the *remote's* configuration, and baking a peer's setting into the
+    count: u64,
-/// client is how you get a value that's wrong on the one deployment that
+    page: u64,
-/// changed it.
+    limit: u64,
-fn trailer(total: Option<u64>, count: u64, page: u64, limit: u64) -> Option<String> {
+) -> Option<String> {
     // 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) => {
+        Some(total) => has_next.then(|| {
-            let by_limit = page
+            match total.checked_sub(shown).filter(|r| *r > 0) {
-                .saturating_sub(1)
+                Some(remaining) => format!(
-                .saturating_mul(limit)
+                    "… {shown} of {total} shown (page {page}, {count}/page) — {remaining} more; re-run with --page {next}."
-                .saturating_add(count);
+                ),
-            let shown = if by_limit == total {
+                None => format!(
-                total
+                    "… {shown} shown (page {page}, {count}/page); more results exist — re-run with --page {next}."
-            } 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
+        None if has_next => Some(format!(
-        // clamped, which is why it is the fallback and not the rule.
+            "… {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 {}.",
+                "… {count} shown (page {page}); more results may exist — re-run with --page {next}."
-                page + 1
             )
         }),
     }
@@ -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
+        // `--limit 400` against a forge that clamps to 50, 187 matches.
-        // a forge that clamps to 50, with 187 matches. `count != limit`,
+        let msg = trailer(Some(187), true, 50, 1, 400).expect("must warn about the other 137");
-        // 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}");
@@ -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() {
+    fn trailer_clamped_page_whose_arithmetic_looks_like_the_last_page() {
-        // Last page, short: 2 * 50 + ... -> 60 shown of 60.
+        // `--limit 100` clamped to 50/page, 150 rows, page 2:
-        assert_eq!(trailer(Some(60), 10, 6, 10), None);
+        // `(page - 1) * limit + count` equals the total, yet rows 101-150
-        // Exactly-full last page: no phantom "page 4".
+        // are on page 3 and the forge says so.
-        assert_eq!(trailer(Some(150), 50, 3, 50), None);
+        let msg = trailer(Some(150), true, 50, 2, 100).expect("page 3 remains");
-        // Empty page is the end even if the total disagrees (a filter
+        assert!(msg.contains("100 of 150"), "{msg}");
-        // the forge discarded, a concurrent close) — never advertise a
+        assert!(msg.contains("50 more"), "{msg}");
-        // next page we just proved is empty.
+        assert!(msg.contains("--page 3"), "{msg}");
-        assert_eq!(trailer(Some(999), 0, 9, 30), None);
+        // 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() {
+    fn trailer_silent_once_the_forge_reports_no_next_page() {
-        let msg = trailer(None, 30, 1, 30).expect("full page, unknown total");
+        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
+        assert_eq!(trailer(Some(1331), false, 31, 27, 50), None);
-        // 837 of 1331 and a hint to fetch page 28.
+        assert_eq!(trailer(Some(55), false, 5, 2, 50), None);
-        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);
+    fn headers(pairs: &[(&'static str, &'static str)]) -> PageHeaders {
-        // Control: the page before the last still hints.
+        let mut map = reqwest::header::HeaderMap::new();
-        assert!(trailer(Some(1331), 50, 26, 50).is_some());
+        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() {
+    fn page_headers_read_next_from_link_or_has_more() {
-        assert_eq!(trailer(Some(100), 50, 2, 50), None);
+        let link =
-        // Control: page 1 of the same list still hints.
+            r#"<https://f/api/v1/x?page=3>; rel="next", <https://f/api/v1/x?page=5>; rel="last""#;
-        let msg = trailer(Some(100), 50, 1, 50).expect("page 2 remains");
+        let h = headers(&[("x-total-count", "150"), ("link", link)]);
-        assert!(msg.contains("50 of 100 shown"), "{msg}");
+        assert_eq!(h.total, Some(150));
-    }
+        assert!(h.has_next);
-
+        assert!(headers(&[("x-hasmore", "true")]).has_next);
-    #[test]
+        // Controls: first/prev links alone, or HasMore false, are not "next".
-    fn trailer_without_total_is_silent_on_a_short_page() {
+        let prev =
-        assert_eq!(trailer(None, 31, 27, 50), None);
+            r#"<https://f/api/v1/x?page=1>; rel="first", <https://f/api/v1/x?page=2>; rel="prev""#;
-        // Control: a full page still hints.
+        assert!(!headers(&[("link", prev)]).has_next);
-        assert!(trailer(None, 50, 27, 50).is_some());
+        assert!(!headers(&[("x-hasmore", "false")]).has_next);
+        assert!(!headers(&[]).has_next);
     }
 }