From d0ab2525e4ecbed1ba6bd9ea324dd66051e2f7d9 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Sun, 20 Sep 2026 10:19:12 +0200 Subject: [PATCH] Apply review feedback on XDG paths Ignore relative XDG_* values and reject a relative HOME, per the XDG spec; add a hint to the missing-config error; tighten docs and comments. Co-Authored-By: Claude Sonnet 5 --- docs/SPEC.md | 13 ++++++++++--- src/paths.rs | 49 +++++++++++++++++++++++++++++++++++++++++-------- src/pipeline.rs | 17 +++++++++++++++-- src/state.rs | 2 +- 4 files changed, 67 insertions(+), 14 deletions(-) diff --git a/docs/SPEC.md b/docs/SPEC.md index 637eada..3361cbe 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -341,9 +341,16 @@ Open questions on the schema: | Last-published / pending versions | `~/.local/state/pkgwatch` | `XDG_STATE_HOME` | `PKGWATCH_STATE_DIR` | | Downloads and build trees (safe to delete) | `~/.cache/pkgwatch` | `XDG_CACHE_HOME` | `PKGWATCH_WORK_DIR` | - The overrides are used verbatim (no `pkgwatch/` suffix) and exist for - dry runs against scratch directories, like `PKGWATCH_REPO_DIR` does for - the pacman repo. Migrating from the old cwd-relative layout: move + Precedence per directory: override, then the XDG variable, then the + default under `$HOME`; an empty variable counts as unset. The overrides + are used verbatim (no `pkgwatch/` suffix) and exist for dry runs against + scratch directories, like `PKGWATCH_REPO_DIR` does for the pacman repo. + The XDG variables and `$HOME` must be absolute paths: a relative XDG + value is ignored, as the XDG spec requires, and a relative `$HOME` is an + error. + + The checkout's `packages.d/` is no longer read on its own; it's just the + source to link from. Migrating from the old cwd-relative layout: move `state/` to the state dir and copy or symlink `packages.d/` into the config dir; `work/` is cache and can simply be dropped. - **Checker**: per source type, resolves "what's the latest version" — diff --git a/src/paths.rs b/src/paths.rs index 3f14ac1..d4ec016 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -1,6 +1,7 @@ //! Decides where pkgwatch's config, state, and work directories live on -//! disk. The only module that reads the environment to answer that — every -//! other module takes the directories it needs as parameters. +//! disk — the only module that reads the environment to answer that; every +//! other module takes the directories it needs as parameters. (The pacman +//! repo dir, `pipeline::custom_repo_dir`, is resolved separately.) //! //! These follow the XDG base-directory spec instead of the current working //! directory, so an installed `/usr/bin/pkgwatch` behaves the same @@ -12,7 +13,7 @@ use std::path::{Path, PathBuf}; const APP_DIR: &str = "pkgwatch"; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq)] pub struct Paths { /// `*.toml` package declarations. Config: hand-edited, worth backing up. pub packages_dir: PathBuf, @@ -32,18 +33,21 @@ impl Paths { /// environment, which would race with `cargo test`'s parallel threads. /// /// Each directory has a pkgwatch-specific override, then the matching - /// XDG variable, then the XDG default under `$HOME`. The overrides exist - /// for dry runs against scratch directories, like `PKGWATCH_REPO_DIR` - /// does for the pacman repo. + /// XDG variable, then the XDG default under `$HOME`; empty counts as + /// unset. The overrides exist for dry runs against scratch + /// directories, like `PKGWATCH_REPO_DIR` does for the pacman repo, so + /// they're used verbatim. The XDG variables and `$HOME` must be + /// absolute: the spec says to ignore a relative XDG value, and honoring + /// one would bring back the cwd dependence this module exists to remove. fn resolve(getenv: impl Fn(&str) -> Option) -> Result { let base = |override_var: &str, xdg_var: &str, home_subpath: &str| -> Result { if let Some(dir) = non_empty(&getenv, override_var) { return Ok(PathBuf::from(dir)); } - if let Some(dir) = non_empty(&getenv, xdg_var) { + if let Some(dir) = absolute(&getenv, xdg_var) { return Ok(Path::new(&dir).join(APP_DIR)); } - let home = non_empty(&getenv, "HOME").context("HOME is not set")?; + let home = absolute(&getenv, "HOME").context("HOME is not set to an absolute path")?; Ok(Path::new(&home).join(home_subpath).join(APP_DIR)) }; @@ -61,6 +65,12 @@ fn non_empty(getenv: &impl Fn(&str) -> Option, key: &str) -> Option Option, key: &str) -> Option { + getenv(key).filter(|v| Path::new(v).is_absolute()) +} + #[cfg(test)] mod tests { use super::*; @@ -132,6 +142,29 @@ mod tests { assert_eq!(paths.work_dir, Path::new("/home/u/.cache/pkgwatch")); } + #[test] + fn relative_xdg_variables_are_ignored() { + let paths = Paths::resolve(env(&[ + ("HOME", "/home/u"), + ("XDG_CONFIG_HOME", "rel/config"), + ("XDG_STATE_HOME", "./state"), + ("XDG_CACHE_HOME", "cache"), + ])) + .unwrap(); + assert_eq!( + paths.packages_dir, + Path::new("/home/u/.config/pkgwatch/packages.d") + ); + assert_eq!(paths.state_dir, Path::new("/home/u/.local/state/pkgwatch")); + assert_eq!(paths.work_dir, Path::new("/home/u/.cache/pkgwatch")); + } + + #[test] + fn relative_home_is_an_error() { + let err = Paths::resolve(env(&[("HOME", "relative/home")])).unwrap_err(); + assert!(err.to_string().contains("HOME")); + } + #[test] fn errors_when_nothing_locates_home() { let err = Paths::resolve(env(&[])).unwrap_err(); diff --git a/src/pipeline.rs b/src/pipeline.rs index 146eac1..753698e 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -42,6 +42,19 @@ fn custom_repo_dir() -> Result { Ok(Path::new(&home).join(CUSTOM_REPO_SUBPATH)) } +/// Adds a hint to the bare "No such file" a missing config dir would give: +/// the dir is no longer relative to the cwd, so a checkout's `packages.d/` +/// isn't picked up on its own (see docs/SPEC.md > Paths). +fn load_packages(packages_dir: &Path) -> Result> { + config::load_packages_dir(packages_dir).with_context(|| { + format!( + "no package config at {} (set PKGWATCH_CONFIG_DIR to the directory containing \ + packages.d, or see docs/SPEC.md > Paths for moving a checkout's packages.d/ there)", + packages_dir.display() + ) + }) +} + pub fn run_check() -> Result<()> { let client = build_client()?; let endpoints = GithubEndpoints::default(); @@ -51,7 +64,7 @@ pub fn run_check() -> Result<()> { work_dir, } = Paths::from_env()?; - let packages = config::load_packages_dir(&packages_dir)?; + let packages = load_packages(&packages_dir)?; if packages.is_empty() { println!("no packages configured under {}/", packages_dir.display()); return Ok(()); @@ -272,7 +285,7 @@ pub fn run_review(args: &[String]) -> Result<()> { state_dir, work_dir, } = Paths::from_env()?; - let packages = config::load_packages_dir(&packages_dir)?; + let packages = load_packages(&packages_dir)?; match args { [] => { diff --git a/src/state.rs b/src/state.rs index 70dbbb2..6158ebd 100644 --- a/src/state.rs +++ b/src/state.rs @@ -1,6 +1,6 @@ //! Persists two independent per-package facts as plain files: the last //! published version, and any version currently pending human review. -//! The only module that touches `state/` on disk. +//! The only module that touches the state directory (see `paths.rs`) on disk. use anyhow::Result; use std::path::Path;