hive-forge: comments total comes from the comments endpoint, not the issue object
fetch_total read issue_get_issue's comments field, Forgejo's running counter on the Issue object that also increments for several non-prose event kinds (review_request, issue_ref, comment_ref, pull_push, ...) that issue_get_comments never returns. Diffing that count against a fetched window reported drift that was never real: a comment that would never be shown at any window size. Live repro on PR #4333 (damocles's diagnosis on the issue): more_after: 2, both events non-prose. Fix: read the total straight off issue_get_comments's own X-Total-Count header instead — the same endpoint that produces the window, so the two populations can never drift apart again. Mirrors timeline.rs's own fetch_total, which made the same move for its endpoint first. fetch_head now captures the header from the request it already makes (one fewer round trip in the --limit path); the --tail/default path still probes once via page_size=1, same shape as before but now scoped to the endpoint that actually produces the window. This also fixes the more serious half: fetch_tail derives its pagination offset (which page holds the tail) from the same total, so an inflated total didn't just skew the trailer's count, it could send the offset math reaching for a page beyond the real thread's end, returning fewer rows than requested or none at all when fetch_tail's empty-page guard tripped. New test pins this against fetch_tail's existing pagination-plan helper. Refs #4335
This commit is contained in:
parent
8464e50f3a
commit
e588b1803e
1 changed files with 113 additions and 20 deletions
|
|
@ -7,11 +7,13 @@
|
||||||
//! default moved to `--tail`'s window instead (see below).
|
//! default moved to `--tail`'s window instead (see below).
|
||||||
//! - `--tail N` returns the *last* N comments, chronological — and with
|
//! - `--tail N` returns the *last* N comments, chronological — and with
|
||||||
//! neither flag given, THIS is the default (N = 10): most-recent
|
//! neither flag given, THIS is the default (N = 10): most-recent
|
||||||
//! activity. Reads the issue's `comments` count first to compute which
|
//! activity. Reads this endpoint's own real total first (see
|
||||||
//! page holds the tail, then fetches only `ceil(N/50) + 1` pages —
|
//! `fetch_total`) to compute which page holds the tail, then fetches
|
||||||
//! bounded by `N`, not thread length.
|
//! only `ceil(N/50) + 1` pages — bounded by `N`, not thread length.
|
||||||
//! - **The count of comments outside whatever window is shown is
|
//! - **The count of comments outside whatever window is shown is
|
||||||
//! always reported** — never a silent truncation.
|
//! always reported** — never a silent truncation. That total is this
|
||||||
|
//! endpoint's own header, never `issue_get_issue`'s `comments` field
|
||||||
|
//! (see `fetch_total`'s doc comment for why).
|
||||||
//! - `--since <RFC3339>` filters to comments at or after that
|
//! - `--since <RFC3339>` filters to comments at or after that
|
||||||
//! timestamp instead of a head/tail window — a cursor: feed the
|
//! timestamp instead of a head/tail window — a cursor: feed the
|
||||||
//! last-seen row's own `created_at` back in next time. Mutually
|
//! last-seen row's own `created_at` back in next time. Mutually
|
||||||
|
|
@ -20,8 +22,7 @@
|
||||||
//!
|
//!
|
||||||
//! Review *bodies* on PRs are always merged in too (`pulls/<n>/reviews`
|
//! Review *bodies* on PRs are always merged in too (`pulls/<n>/reviews`
|
||||||
//! isn't the issues/comments thread), tagged `[review: STATE]`, with a
|
//! isn't the issues/comments thread), tagged `[review: STATE]`, with a
|
||||||
//! `(N line comment(s) — see 'pr reviews')` pointer when a review has
|
//! `(N line comment(s) — see 'pr reviews')` pointer for inline comments.
|
||||||
//! inline comments — this verb never inlines those, `pr reviews` does.
|
|
||||||
//!
|
//!
|
||||||
//! `--json` output is an object (`{"comments": [...], "more_before": N,
|
//! `--json` output is an object (`{"comments": [...], "more_before": N,
|
||||||
//! "more_after": N, "since_more": bool}`), not a bare array, so a
|
//! "more_after": N, "since_more": bool}`), not a bare array, so a
|
||||||
|
|
@ -86,8 +87,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||||
let (thread, more) = fetch_since(client, args.number, since, limit)?;
|
let (thread, more) = fetch_since(client, args.number, since, limit)?;
|
||||||
(thread, 0, 0, more, clamped)
|
(thread, 0, 0, more, clamped)
|
||||||
} else if let Some(limit) = args.limit {
|
} else if let Some(limit) = args.limit {
|
||||||
let total = fetch_total(client, args.number)?;
|
let (thread, total) = fetch_head(client, args.number, limit)?;
|
||||||
let thread = fetch_head(client, args.number, limit)?;
|
|
||||||
let more_after = total.saturating_sub(thread.len());
|
let more_after = total.saturating_sub(thread.len());
|
||||||
(thread, 0, more_after, false, false)
|
(thread, 0, more_after, false, false)
|
||||||
} else {
|
} else {
|
||||||
|
|
@ -276,19 +276,48 @@ fn attachment_json_from_value(c: &Value) -> Vec<Value> {
|
||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Total comment count on an issue/PR, straight off the issue object.
|
/// Interpret a comments-endpoint response's `X-Total-Count` header as
|
||||||
/// Used both to plan `--tail`'s pagination and to report how many
|
/// the real prose-comment total, falling back to `fallback` (the
|
||||||
|
/// window just fetched, or `0` for a bare probe) when the header is
|
||||||
|
/// absent or won't fit a `usize` (defensive — Forgejo always sends a
|
||||||
|
/// small non-negative value in practice). Never invents a number: a
|
||||||
|
/// missing header only ever *under*-reports via the fallback, it can't
|
||||||
|
/// manufacture a `more_before`/`more_after` count that then turns out
|
||||||
|
/// to be a non-prose event the comments endpoint would never surface.
|
||||||
|
fn total_from_header(x_total_count: Option<i64>, fallback: usize) -> usize {
|
||||||
|
x_total_count
|
||||||
|
.and_then(|t| usize::try_from(t).ok())
|
||||||
|
.unwrap_or(fallback)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Real *prose*-comment count on an issue/PR, read off this same
|
||||||
|
/// endpoint's own `X-Total-Count` header via a cheap `page_size=1`
|
||||||
|
/// probe — used to plan `--tail`'s pagination and to report how many
|
||||||
/// comments sit outside whatever window ends up shown.
|
/// comments sit outside whatever window ends up shown.
|
||||||
|
///
|
||||||
|
/// This used to read `issue_get_issue`'s `comments` field instead —
|
||||||
|
/// Forgejo's running counter on the Issue object, which also increments for
|
||||||
|
/// several non-prose event kinds (`review_request`, `issue_ref`,
|
||||||
|
/// `comment_ref`, `pull_push`, …) that `issue_get_comments` never
|
||||||
|
/// returns. Subtracting that inflated count against this endpoint's own
|
||||||
|
/// results reported drift that was never real: a comment that would
|
||||||
|
/// never be shown at any window size. Mirrors `timeline.rs`'s own
|
||||||
|
/// `fetch_total`, which moved to the same trick for its own endpoint
|
||||||
|
/// first — reading the count straight off the endpoint that also
|
||||||
|
/// produces the window means the two can never drift apart again.
|
||||||
fn fetch_total(client: &Client, number: u64) -> Result<usize> {
|
fn fetch_total(client: &Client, number: u64) -> Result<usize> {
|
||||||
let (owner, name) = client.owner_repo()?;
|
let (owner, name) = client.owner_repo()?;
|
||||||
let issue = client
|
let (headers, _) = client
|
||||||
.api()
|
.api()
|
||||||
.issue_get_issue(owner, name, index(number)?)
|
.issue_get_comments(
|
||||||
|
owner,
|
||||||
|
name,
|
||||||
|
index(number)?,
|
||||||
|
IssueGetCommentsQuery::default(),
|
||||||
|
)
|
||||||
|
.page_size(1)
|
||||||
.send()?;
|
.send()?;
|
||||||
Ok(issue
|
Ok(total_from_header(headers.x_total_count, 0))
|
||||||
.comments
|
|
||||||
.and_then(|c| usize::try_from(c).ok())
|
|
||||||
.unwrap_or(0))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Serialize a typed comment page back to the JSON `Value` shape the
|
/// Serialize a typed comment page back to the JSON `Value` shape the
|
||||||
|
|
@ -374,10 +403,13 @@ fn merge_chronological(mut items: Vec<Value>, reviews: Vec<Value>) -> Vec<Value>
|
||||||
items
|
items
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Fetch the first page's worth of comments (existing behaviour).
|
/// Fetch the first page's worth of comments (existing behaviour) plus
|
||||||
fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<Vec<Value>> {
|
/// the thread's real total, read straight off this same request's
|
||||||
|
/// `X-Total-Count` header — one request, no separate `fetch_total`
|
||||||
|
/// call needed for this branch.
|
||||||
|
fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<(Vec<Value>, usize)> {
|
||||||
let (owner, name) = client.owner_repo()?;
|
let (owner, name) = client.owner_repo()?;
|
||||||
let (_, comments) = client
|
let (headers, comments) = client
|
||||||
.api()
|
.api()
|
||||||
.issue_get_comments(
|
.issue_get_comments(
|
||||||
owner,
|
owner,
|
||||||
|
|
@ -387,7 +419,8 @@ fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<Vec<Value>> {
|
||||||
)
|
)
|
||||||
.page_size(u32::try_from(limit).unwrap_or(u32::MAX))
|
.page_size(u32::try_from(limit).unwrap_or(u32::MAX))
|
||||||
.send()?;
|
.send()?;
|
||||||
to_values(comments)
|
let total = total_from_header(headers.x_total_count, comments.len());
|
||||||
|
Ok((to_values(comments)?, total))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Fetch the last `n` comments on an issue/PR in chronological order.
|
/// Fetch the last `n` comments on an issue/PR in chronological order.
|
||||||
|
|
@ -530,6 +563,66 @@ mod tests {
|
||||||
assert_eq!(tail_plan(5, 100), (1, 1));
|
assert_eq!(tail_plan(5, 100), (1, 1));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn tail_plan_with_inflated_total_overshoots_the_real_last_page() {
|
||||||
|
// The offset half of the mismatched-total mechanism: 48 real prose comments
|
||||||
|
// (all fit on page 1, page_size 50) plus 5 non-prose events the
|
||||||
|
// old `issue.comments` counter double-counted gives an inflated
|
||||||
|
// total of 53. The correct plan never leaves page 1 — everything
|
||||||
|
// real lives there.
|
||||||
|
assert_eq!(tail_plan(48, 3), (1, 1));
|
||||||
|
// Fed the inflated total instead, the plan reaches for page 2,
|
||||||
|
// which doesn't exist on the real thread — `fetch_tail`'s
|
||||||
|
// `arr.is_empty()` guard then breaks immediately and returns
|
||||||
|
// zero comments instead of the real last 3. Not just a wrong
|
||||||
|
// trailer number: the wrong rows (none) get shown.
|
||||||
|
assert_eq!(tail_plan(53, 3), (2, 2));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn total_from_header_prefers_the_header_over_the_fallback() {
|
||||||
|
assert_eq!(total_from_header(Some(53), 8), 53);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn total_from_header_falls_back_when_header_absent() {
|
||||||
|
// Never invents a number it doesn't have — under-reports to the
|
||||||
|
// fallback rather than guessing.
|
||||||
|
assert_eq!(total_from_header(None, 8), 8);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn total_from_header_falls_back_on_a_malformed_value() {
|
||||||
|
// A negative count can't be a real `usize`; fall back rather
|
||||||
|
// than silently wrapping.
|
||||||
|
assert_eq!(total_from_header(Some(-1), 8), 8);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn drift_scenario_reports_nothing_withheld_when_header_matches_window() {
|
||||||
|
// The core mismatched-total scenario: the real total, read off the comments
|
||||||
|
// endpoint's own header, matches the fetched window exactly, even
|
||||||
|
// though a differently-scoped counter (the old `issue.comments`
|
||||||
|
// field, no longer read) would have come back higher because of
|
||||||
|
// unrelated non-prose events (`review_request`, `issue_ref`,
|
||||||
|
// `comment_ref`, …) that endpoint never returns. Trailer must
|
||||||
|
// report nothing withheld.
|
||||||
|
let window_len = 8;
|
||||||
|
let total = total_from_header(Some(8), window_len);
|
||||||
|
assert_eq!(truncation_note(total.saturating_sub(window_len), 0), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn drift_scenario_still_reports_a_genuine_truncation() {
|
||||||
|
// A real truncation must still surface — silencing the trailer
|
||||||
|
// outright would swap a false positive for a false negative,
|
||||||
|
// which is strictly worse.
|
||||||
|
let window_len = 10;
|
||||||
|
let total = total_from_header(Some(12), window_len);
|
||||||
|
let note = truncation_note(total.saturating_sub(window_len), 0).unwrap();
|
||||||
|
assert!(note.contains('2'), "{note}");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn merge_interleaves_reviews_by_timestamp() {
|
fn merge_interleaves_reviews_by_timestamp() {
|
||||||
// A review submitted between two comments must land between
|
// A review submitted between two comments must land between
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue