From c90a66a24f740d1b4bbf958a919daa8fa8482a41 Mon Sep 17 00:00:00 2001 From: rays Date: Sat, 17 Oct 2026 21:59:37 +0000 Subject: [PATCH] Clippy clean, and rustfmt and clippy part of the checks cargo clippy --workspace --all-targets had 24 warnings; its own fixes took most, the rest by hand: two type aliases for long types, a struct built whole, a checked division, a doc comment's stray blank line. Its fix for the stale-feed fold was wrong: `any` stops at the first feed it removes, where the fold removed every one, so that is a loop instead. A pre-commit hook in .githooks refuses unformatted Rust; clippy, being a build, is in CLAUDE.md's checks with -D warnings. Co-Authored-By: Claude Opus 5.5 --- .git-blame-ignore-revs | 3 ++ .githooks/pre-commit | 7 ++++ CLAUDE.md | 9 ++++ crates/directory/src/store.rs | 9 ++-- crates/feed/src/lib.rs | 5 ++- crates/ipx/src/db.rs | 13 +++--- crates/ipx/src/download.rs | 2 +- crates/ipx/src/main.rs | 78 ++++++++++++++++++++--------------- crates/ipx/src/signals.rs | 2 +- crates/ipx/src/web.rs | 18 ++++---- 10 files changed, 89 insertions(+), 57 deletions(-) create mode 100644 .git-blame-ignore-revs create mode 100755 .githooks/pre-commit diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs new file mode 100644 index 0000000..2ccd93b --- /dev/null +++ b/.git-blame-ignore-revs @@ -0,0 +1,3 @@ +# Formatting only: `git config blame.ignoreRevsFile .git-blame-ignore-revs` makes blame skip these. +# rustfmt adopted +692a5102179b958b5d15cecc4f5e6f16b2955b76 diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000..d3969e8 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,7 @@ +#!/bin/sh +# Refuses a commit whose Rust is not as rustfmt writes it. Clippy is left to the checks before a +# push (CLAUDE.md, Tests): it is a build, and Tower's builds go one at a time. +git diff --cached --name-only --diff-filter=ACM | grep -q '\.rs$' || exit 0 +cargo fmt --all --check >/dev/null 2>&1 && exit 0 +echo "pre-commit: the Rust is not formatted; run cargo fmt --all" >&2 +exit 1 diff --git a/CLAUDE.md b/CLAUDE.md index 013efe4..cf999a3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -171,6 +171,8 @@ Patching that file by guessing an anchor string has failed repeatedly. Read the ## Tests ```sh +cargo fmt --all --check # rustfmt, as rustfmt.toml says; the pre-commit hook refuses a commit without it +cargo clippy --workspace --all-targets -- -D warnings # no warnings, ever cargo test # ~80 tests: parsing, filters, retention, schedules, SQL, per-user state npx tsc -p . # type-checks web/src node tests/page-smoke.js @@ -194,6 +196,13 @@ session was stopped. So: * Builds and test binaries live under `target/`, a link to `/mnt/fast`; linking through `/mnt/user` (FUSE) once took the load to 248 on its own. +The Rust is formatted by rustfmt (`rustfmt.toml`) and kept free of clippy's warnings. A clone +turns on the hook once: `git config core.hooksPath .githooks`; and `git config blame.ignoreRevsFile +.git-blame-ignore-revs` keeps blame past the commit that first formatted everything. A clippy +suggestion is not always the same code: on adopting it, `fold(false, |any, id| remove(id) || any)` +was offered as `any(...)`, which stops at the first and would have removed one feed of many. A lint +that is wrong for a line gets `#[allow(clippy::...)]` there with a comment saying why. + Things about the browser suite that have cost time: * It starts **its own daemon and database** under `/tmp/ipx-ui-test`, wiped once per run. Playwright diff --git a/crates/directory/src/store.rs b/crates/directory/src/store.rs index c09e2f7..83ee9e3 100644 --- a/crates/directory/src/store.rs +++ b/crates/directory/src/store.rs @@ -424,8 +424,7 @@ impl Store { } pub async fn due_one(&self, id: &str) -> Result> { - Ok(self - .rows("SELECT id, url, etag, last_modified FROM feeds WHERE id = $1", vec![id.into()]) + self.rows("SELECT id, url, etag, last_modified FROM feeds WHERE id = $1", vec![id.into()]) .await? .first() .map(|r| -> Result { @@ -436,7 +435,7 @@ impl Store { last_modified: r.try_get("", "last_modified")?, }) }) - .transpose()?) + .transpose() } pub async fn save(&self, id: &str, got: Refreshed) -> Result<()> { @@ -507,8 +506,8 @@ impl Store { /// A feed's last error as its page says it: explained once it has been failing a day, or else /// only that the check failed. Never the error itself, which is for the log. fn failure_words(err: &str, failing_for: i64) -> String { - Some(err) - .filter(|_| failing_for >= FLAG_AFTER_SECS) + (failing_for >= FLAG_AFTER_SECS) + .then_some(err) .and_then(ipx_feed::explain_failure) .map_or_else(|| "Its last check failed.".to_owned(), |f| f.reason.to_owned()) } diff --git a/crates/feed/src/lib.rs b/crates/feed/src/lib.rs index 3feb6d5..395ad9b 100644 --- a/crates/feed/src/lib.rs +++ b/crates/feed/src/lib.rs @@ -386,8 +386,11 @@ fn patreon_campaign(head: &[u8]) -> Option { }) } +/// A campaign's name, and its shows as (id, title). +type PatreonShows = (Option, Vec<(String, String)>); + /// A campaign's name and its shows as (id, title), from Patreon's JSON:API answer. -fn parse_patreon_shows(json: &[u8]) -> Result<(Option, Vec<(String, String)>)> { +fn parse_patreon_shows(json: &[u8]) -> Result { let v: serde_json::Value = serde_json::from_slice(json).context("Patreon's answer is not JSON")?; // Missing is not the same as none. Read as no shows, the creator feed would be scanned as // a plain feed, claim every show's files, and leave the shows empty once the list returned. diff --git a/crates/ipx/src/db.rs b/crates/ipx/src/db.rs index 7ac6577..6907c27 100644 --- a/crates/ipx/src/db.rs +++ b/crates/ipx/src/db.rs @@ -771,7 +771,6 @@ impl Db { /// Returns true when this enclosure URL is new. False means we have downloaded it /// before, or deliberately reaped it -- either way it is not fetched again. - pub async fn mark_downloaded(&self, url: &str, path: &std::path::Path, bytes: u64) -> Result<()> { self.exec( "UPDATE enclosures SET state = 'done', path = $2, bytes_done = $3, @@ -1789,7 +1788,7 @@ impl Db { /// False when they do not subscribe to it, as `set_pinned`. pub async fn set_kept(&self, user_id: i64, feed_id: &str, on: bool) -> Result { let r = subscriptions::Entity::update_many() - .col_expr(subscriptions::Column::Kept, Expr::val(on).into()) + .col_expr(subscriptions::Column::Kept, Expr::val(on)) .filter(subscriptions::Column::UserId.eq(user_id)) .filter(subscriptions::Column::FeedId.eq(feed_id)) .exec(&self.orm) @@ -1819,7 +1818,7 @@ impl Db { pub async fn set_pinned(&self, user_id: i64, feed_id: &str, on: bool) -> Result { let r = subscriptions::Entity::update_many() - .col_expr(subscriptions::Column::Pinned, Expr::val(on).into()) + .col_expr(subscriptions::Column::Pinned, Expr::val(on)) .filter(subscriptions::Column::UserId.eq(user_id)) .filter(subscriptions::Column::FeedId.eq(feed_id)) .exec(&self.orm) @@ -1927,7 +1926,7 @@ impl Db { use sea_orm::sea_query::ExprTrait; let now = now(); users::Entity::update_many() - .col_expr(users::Column::LastLogin, Expr::val(now).into()) + .col_expr(users::Column::LastLogin, Expr::val(now)) .filter(users::Column::Id.eq(id)) .filter(Expr::expr(Func::coalesce([Expr::col(users::Column::LastLogin), Expr::val(0)])).lte(now - 3600)) .exec(&self.orm) @@ -1978,7 +1977,7 @@ impl Db { .and_then(|(_, u)| u); if found.is_some() { sessions::Entity::update_many() - .col_expr(sessions::Column::Seen, Expr::val(now()).into()) + .col_expr(sessions::Column::Seen, Expr::val(now())) .filter(sessions::Column::Token.eq(token)) .exec(&self.orm) .await?; @@ -2035,7 +2034,7 @@ impl Db { .await?; let Some((token, Some(user))) = found else { return Ok(None) }; api_tokens::Entity::update_many() - .col_expr(api_tokens::Column::LastUsed, Expr::val(now()).into()) + .col_expr(api_tokens::Column::LastUsed, Expr::val(now())) .filter(api_tokens::Column::Id.eq(token.id)) .exec(&self.orm) .await?; @@ -2711,7 +2710,7 @@ impl Db { } if name.is_some() || words.is_some() || description.is_some() { signal_verdicts::Entity::update_many() - .col_expr(signal_verdicts::Column::Recheck, Expr::val(true).into()) + .col_expr(signal_verdicts::Column::Recheck, Expr::val(true)) .filter(signal_verdicts::Column::SignalId.eq(id)) .filter(signal_verdicts::Column::Hit.eq(true)) .exec(&self.orm) diff --git a/crates/ipx/src/download.rs b/crates/ipx/src/download.rs index 0cf9e47..e2a9edd 100644 --- a/crates/ipx/src/download.rs +++ b/crates/ipx/src/download.rs @@ -85,7 +85,7 @@ pub fn filename_for(url: &str, content_disposition: Option<&str>) -> String { } let from_url = url::Url::parse(url) .ok() - .and_then(|u| u.path_segments().and_then(|s| s.filter(|p| !p.is_empty()).next_back()).map(str::to_owned)) + .and_then(|u| u.path_segments().and_then(|mut s| s.rfind(|p| !p.is_empty())).map(str::to_owned)) .map(|seg| percent_encoding::percent_decode_str(&seg).decode_utf8_lossy().into_owned()) .unwrap_or_default(); sanitize(&from_url) diff --git a/crates/ipx/src/main.rs b/crates/ipx/src/main.rs index 6e09e90..38c1fc2 100644 --- a/crates/ipx/src/main.rs +++ b/crates/ipx/src/main.rs @@ -397,10 +397,12 @@ async fn main() -> Result<()> { Command::Export { file } => export(&ctx, &file).await, _ => run(&ctx, wire_cmd.expect("only List and Daemon have no wire form")).await, }; - if result.is_ok() && changes_catalogue && ipc::daemon_is_live(&socket).await { - if let Err(e) = ipc::reload(&socket).await { - eprintln!("the running daemon did not take the change ({e:#}); restart it, or it may undo it"); - } + if result.is_ok() + && changes_catalogue + && ipc::daemon_is_live(&socket).await + && let Err(e) = ipc::reload(&socket).await + { + eprintln!("the running daemon did not take the change ({e:#}); restart it, or it may undo it"); } // The batch exporter holds the last few seconds of spans; without this they are lost. if let Some(p) = otel { @@ -573,8 +575,8 @@ pub(crate) async fn status(ctx: &Ctx) -> Event { use std::sync::atomic::Ordering::Relaxed; let st = &metrics::STATS; st.feeds.store(feeds as i64, Relaxed); - st.pending.store(pending as i64, Relaxed); - st.downloaded.store(downloaded as i64, Relaxed); + st.pending.store(pending, Relaxed); + st.downloaded.store(downloaded, Relaxed); Event::Status { feeds, pending, downloaded } } Err(e) => Event::Error { msg: format!("{e:#}") }, @@ -1086,8 +1088,10 @@ async fn assemble_config(db: &db::Db, mut cfg: config::Config, path: &std::path: } async fn export(ctx: &Ctx, file: &std::path::Path) -> Result<()> { - let mut doc = opml::OPML::default(); - doc.head = Some(opml::Head { title: Some("ipx subscriptions".into()), ..Default::default() }); + let mut doc = opml::OPML { + head: Some(opml::Head { title: Some("ipx subscriptions".into()), ..Default::default() }), + ..Default::default() + }; for (id, feed) in &ctx.cfg().feeds { let title = ctx.db.feed_summary(id).await.ok().and_then(|s| s.title).unwrap_or_else(|| id.clone()); doc.add_feed(&title, &feed.url); @@ -1139,7 +1143,12 @@ async fn clean_directory(ctx: &Ctx) -> Result<()> { return Ok(()); } let mut cfg = ctx.edit_cfg().await; - if stale.iter().fold(false, |any, id| cfg.feeds.remove(id).is_some() || any) { + // Every one removed, so not `any`, which would stop at the first. + let mut removed = false; + for id in &stale { + removed |= cfg.feeds.remove(id).is_some(); + } + if removed { ctx.store_cfg(cfg).await?; } for id in &stale { @@ -1208,14 +1217,15 @@ async fn fetch(ctx: &Arc, only: Option<&str>, force: bool, scope: &[String] state.last_modified = None; } - if !force && let Some(at) = ctx.due(&cfg, &sub.cfg, id, &state, subscribed.contains_key(id)) { - if at > db::now() { - ctx.out.emit(Event::FeedSkip { - feed: id.clone(), - reason: format!("not due for {}", duration((at - db::now()) as u64)), - }); - continue; - } + if !force + && let Some(at) = ctx.due(&cfg, &sub.cfg, id, &state, subscribed.contains_key(id)) + && at > db::now() + { + ctx.out.emit(Event::FeedSkip { + feed: id.clone(), + reason: format!("not due for {}", duration((at - db::now()) as u64)), + }); + continue; } due.push((sub, state)); } @@ -1672,7 +1682,7 @@ async fn scan_one( force: bool, // The body the scan already fetched for it, if it did (#104), and where it moved, if it did; // with the site's icon when a forced scan looked it up meanwhile, "" for none (#228). - prefetched: Option<(Result<(feed::Fetched, Option)>, Option)>, + prefetched: Option, ) -> Result { let (prefetched, icon) = match prefetched { Some((got, icon)) => (Some(got), icon), @@ -2004,6 +2014,9 @@ async fn scan_one( Ok(Outcome::Feed(scan)) } +/// A feed's body fetched by the scan ahead of its turn, and the site's icon if one was looked up. +type Prefetched = (Result<(feed::Fetched, Option)>, Option); + async fn sync_opml(ctx: &Arc, parent_id: &str, parent: &config::Feed, bytes: &[u8]) -> Result { let listed = feed::parse_opml(bytes)?; if let Some(title) = feed::opml_title(bytes) { @@ -2381,19 +2394,18 @@ async fn torrent_one( ctx.torrents() .await? .fetch(&cfg, url, dest_dir, |done, total| { - if total > 0 { - let pct = (done * 100 / total) as i64; - if pct > last_pct { - last_pct = pct; - ctx.out.emit(Event::Progress { - feed: feed_id.to_string(), - enclosure, - url: url.to_string(), - file: name.clone(), - done, - total: Some(total), - }); - } + if let Some(pct) = (done * 100).checked_div(total).map(|p| p as i64) + && pct > last_pct + { + last_pct = pct; + ctx.out.emit(Event::Progress { + feed: feed_id.to_string(), + enclosure, + url: url.to_string(), + file: name.clone(), + done, + total: Some(total), + }); } }) .await @@ -2522,8 +2534,8 @@ mod tests { fn feed() -> config::Feed { // Whatever `ipx add` would write, which is the shape every code path sees. let mut cfg = config::Config::default(); - let f = add_one_cfg(&mut cfg, "http://x/f.xml", None, vec![]); - f + + add_one_cfg(&mut cfg, "http://x/f.xml", None, vec![]) } /// The feed entry `add` builds, without the network round trip it does for a title. diff --git a/crates/ipx/src/signals.rs b/crates/ipx/src/signals.rs index 25d9354..df83294 100644 --- a/crates/ipx/src/signals.rs +++ b/crates/ipx/src/signals.rs @@ -231,7 +231,7 @@ fn full(items: &[Unjudged]) -> bool { } pub fn refuses_key(e: &anyhow::Error) -> bool { - matches!(status(e), Some(401 | 402 | 403)) + matches!(status(e), Some(401..=403)) } fn rate_limited(e: &anyhow::Error) -> bool { diff --git a/crates/ipx/src/web.rs b/crates/ipx/src/web.rs index b20d8dd..1e72aba 100644 --- a/crates/ipx/src/web.rs +++ b/crates/ipx/src/web.rs @@ -174,10 +174,10 @@ async fn auth(State(state): State, mut req: Request, next: Next) -> Re } // 2. A session cookie from signing in here. - if user.is_none() { - if let Some(sid) = cookie(&req, SESSION_COOKIE) { - user = state.ctx.db.session_user(&sid, cfg.web.session_days.max(1) * 86_400).await.unwrap_or(None); - } + if user.is_none() + && let Some(sid) = cookie(&req, SESSION_COOKIE) + { + user = state.ctx.db.session_user(&sid, cfg.web.session_days.max(1) * 86_400).await.unwrap_or(None); } // 3. The shared token, which is the admin: the healthcheck and any scripts predate @@ -213,10 +213,10 @@ async fn auth(State(state): State, mut req: Request, next: Next) -> Re req.extensions_mut().insert(user); req.extensions_mut().insert(Proxied(by_proxy)); let mut resp = next.run(req).await; - if let Some(c) = set_cookie { - if let Ok(v) = header::HeaderValue::from_str(&c) { - resp.headers_mut().insert(header::SET_COOKIE, v); - } + if let Some(c) = set_cookie + && let Ok(v) = header::HeaderValue::from_str(&c) + { + resp.headers_mut().insert(header::SET_COOKIE, v); } resp } @@ -938,7 +938,7 @@ async fn feed_rows(state: &WebState, user: &crate::db::User, only: Option<&str>) failing: s .error_since .filter(|since| crate::db::now() - since >= FLAG_AFTER_SECS) - .and_then(|_| s.last_error.as_deref()) + .and(s.last_error.as_deref()) .and_then(crate::feed::explain_failure) .map(|f| FailingRow { reason: f.reason, new_url: f.new_url }), last_error: s.last_error,