feat(forge): search + milestone filters, and a page trailer that can't lie
`list` already built its query with `q: None, milestones: None` — both fields were on the request it was sending. So full-text search over title and body is a flag, not a new verb, and a text match is only useful composed with the other filters anyway. The trailer was the real defect. It fired on `count == limit`, but the forge clamps page size to its own `api.MAX_RESPONSE_ITEMS`: ask for 400, get a full 50, and `50 != 400` kept it silent — suppressing the warning in precisely the case where the truncation is invisible. It now reports the real total from `X-Total-Count`, which the response header struct already parsed and the call site discarded. The requested limit is not clamped client-side: that ceiling is the remote's configuration, not ours.
This commit is contained in:
parent
bcd4fbd918
commit
8e690c0694
2 changed files with 135 additions and 17 deletions
|
|
@ -67,6 +67,8 @@ hive-forge diff 42 # unified diff (lockfile hunks colla
|
|||
hive-forge diff 42 --full # include unfiltered lockfile hunks
|
||||
hive-forge list # open issues/PRs
|
||||
hive-forge list --kind pr --state all --page 2 # page 2 of all PRs (walk --page 1,2,… with --limit as page size for a repo-wide sweep)
|
||||
hive-forge list --search "trust bundle" --state all # full-text over title AND body — the duplicate check
|
||||
hive-forge list --milestone 11 --state all # what's left in a milestone (name or id, repeatable)
|
||||
hive-forge milestone # list milestones
|
||||
hive-forge branches deployed/ # filter branches by pattern
|
||||
hive-forge tree-sha main # git tree SHA for a ref
|
||||
|
|
@ -282,3 +284,15 @@ to discover valid label names before triaging or to audit the label set.
|
|||
PR number is parsed back out of the push output (the AGit push
|
||||
itself has no label field), so they're silently skipped if that
|
||||
parse fails — same fallback as the deferred multi-line body.
|
||||
- `list --milestone <name>` takes a milestone **name or id**, is
|
||||
repeatable, and the forge *discards* one it doesn't recognise. So a
|
||||
typo returns the **unfiltered** list rather than an empty one — the
|
||||
failure looks like "this milestone contains everything", not like an
|
||||
error. Confirm the spelling with `hive-forge milestone`. (Same
|
||||
silent-discard shape as unknown labels above.)
|
||||
- `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 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.
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
//! `list [--kind issue|pr|both] [--state open|closed|all] [--assignee
|
||||
//! <user>] [--author <user>] [--label <name>] [--limit N] [--page N]` —
|
||||
//! <user>] [--author <user>] [--label <name>] [--milestone <name>]
|
||||
//! [--search <text>] [--limit N] [--page N]` —
|
||||
//! list issues / PRs with filters. Pretty `#NNN [author] title` output by
|
||||
//! default; `--json` for piping. `--limit` is the page size and `--page`
|
||||
//! the 1-based page number — walk pages incrementally for a large result
|
||||
|
|
@ -9,6 +10,11 @@
|
|||
//! filters one-for-one so the mental model carries over. Closes the
|
||||
//! read-side curl-fallback gap (no boundary concerns —
|
||||
//! every agent + the operator queries the issue tracker constantly).
|
||||
//!
|
||||
//! `--search` is why there is no separate `search` verb: the forge
|
||||
//! matches it against title **and body** on this same endpoint, and a
|
||||
//! text match is only useful *composed* with the other filters — a
|
||||
//! separate verb would have to grow every one of them back.
|
||||
|
||||
use anyhow::Result;
|
||||
use clap::{Args as ClapArgs, ValueEnum};
|
||||
|
|
@ -85,8 +91,22 @@ pub struct Args {
|
|||
/// Filter to items carrying any of these label names. Repeatable.
|
||||
#[arg(long = "label")]
|
||||
labels: Vec<String>,
|
||||
/// Page size — items per page (default: 30; forge's per-page cap,
|
||||
/// ~50, applies). Must be >= 1.
|
||||
/// Filter to items in any of these milestones, by name or id.
|
||||
/// Repeatable. A name that doesn't exist is discarded by the forge
|
||||
/// rather than rejected — so a typo returns the UNFILTERED list, not
|
||||
/// an empty one. Check the spelling against `milestone list`.
|
||||
#[arg(long = "milestone")]
|
||||
milestones: Vec<String>,
|
||||
/// Full-text search over title AND body, server-side. Composes with
|
||||
/// every filter above — this is the duplicate-hunting path that
|
||||
/// grepping `list` output can't cover, since grep only ever sees
|
||||
/// the titles.
|
||||
#[arg(long)]
|
||||
search: Option<String>,
|
||||
/// Page size — items per page (default: 30). The forge clamps this
|
||||
/// to its own `api.MAX_RESPONSE_ITEMS` (50 by default), so a large
|
||||
/// `--limit` silently returns a smaller page; the trailer reports
|
||||
/// the real total rather than trusting this number. Must be >= 1.
|
||||
#[arg(long, default_value_t = 30, value_parser = clap::value_parser!(u64).range(1..))]
|
||||
limit: u64,
|
||||
/// Page number to fetch (1-based, default 1). Combine with `--limit`
|
||||
|
|
@ -101,9 +121,10 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
state: Some(args.state.query_state()),
|
||||
// The forge parses `labels` as a comma-separated list of names.
|
||||
labels: (!args.labels.is_empty()).then(|| args.labels.join(",")),
|
||||
q: None,
|
||||
q: args.search.clone().filter(|s| !s.is_empty()),
|
||||
r#type: args.kind.query_type(),
|
||||
milestones: None,
|
||||
// Same comma-separated shape as `labels` above.
|
||||
milestones: (!args.milestones.is_empty()).then(|| args.milestones.join(",")),
|
||||
since: None,
|
||||
before: None,
|
||||
created_by: args.author.clone().filter(|s| !s.is_empty()),
|
||||
|
|
@ -111,7 +132,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
mentioned_by: args.mention.clone().filter(|s| !s.is_empty()),
|
||||
sort: None,
|
||||
};
|
||||
let (_, issues) = client
|
||||
let (headers, issues) = client
|
||||
.api()
|
||||
.issue_list_issues(owner, name, query)
|
||||
.page(u32::try_from(args.page).unwrap_or(u32::MAX))
|
||||
|
|
@ -125,21 +146,63 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
for item in items.as_array().into_iter().flatten() {
|
||||
print_row(item);
|
||||
}
|
||||
// When the returned page is exactly `--limit` items, more pages
|
||||
// may exist. Print a hint to stderr so the caller knows to fetch
|
||||
// the next page rather than assuming the result is complete. Only
|
||||
// fires on a full page — a short or empty page signals the end.
|
||||
if count == args.limit {
|
||||
eprintln!(
|
||||
"… {} shown (page {}); more results may exist — re-run with --page {} (or raise --limit).",
|
||||
args.limit,
|
||||
args.page,
|
||||
args.page + 1
|
||||
);
|
||||
if let Some(msg) = trailer(
|
||||
headers.x_total_count.and_then(|t| u64::try_from(t).ok()),
|
||||
count,
|
||||
args.page,
|
||||
args.limit,
|
||||
) {
|
||||
eprintln!("{msg}");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// 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.
|
||||
///
|
||||
/// `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<u64>, count: u64, page: u64, limit: u64) -> Option<String> {
|
||||
// An empty page is the end, whatever else is true.
|
||||
if count == 0 {
|
||||
return None;
|
||||
}
|
||||
match total {
|
||||
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 {}.",
|
||||
page + 1
|
||||
)
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
/// Render one issue/PR as a single `#NNN [author] title` line.
|
||||
/// Defensive: missing fields drop to placeholders so a partial
|
||||
/// response from a future API change still produces readable output
|
||||
|
|
@ -187,4 +250,45 @@ mod tests {
|
|||
);
|
||||
assert_eq!(State::All.query_state(), IssueListIssuesQueryState::All);
|
||||
}
|
||||
|
||||
#[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");
|
||||
assert!(msg.contains("50 of 187"), "{msg}");
|
||||
assert!(msg.contains("137 more"), "{msg}");
|
||||
assert!(msg.contains("--page 2"), "{msg}");
|
||||
}
|
||||
|
||||
#[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");
|
||||
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);
|
||||
}
|
||||
|
||||
#[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");
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue