From a9a42e3a5a3d90982445663e623a6aec5dfea8a0 Mon Sep 17 00:00:00 2001 From: hamodywe Date: Wed, 26 Aug 2026 04:14:55 +0300 Subject: [PATCH] feat: add --diff , a baseline taken from git instead of a file A baseline is a file someone has to keep up to date. Most of the time the question is simpler: what does this branch add that main did not already have? `--diff ` checks out the merge base of and HEAD into a temporary worktree, runs the same command with the same thresholds over it, and uses what it reports as the baseline for the working copy. The worktree is removed before the report prints. The merge base rather than the tip of , because a fix that landed on main after the branch was cut is not something the branch removed, and a finding main gained since is not something the branch added. A test builds exactly that history and asserts the branch is not blamed for it. The baseline machinery is reused rather than duplicated: the comparison run produces the JSON documents `--json` would print and `Baseline` reads them, so "already known" means the same thing whichever way the baseline arrived. `--diff` and `--baseline` combine through `Baseline::merge`, which keeps the worse reading of a finding both record. The note at the end of the report now names what hid the findings and the merge base. Also rejects `--baseline` and `--diff` on `cost` and `why`, which have no findings to hide: the flag was accepted and changed nothing, and a flag that silently does nothing is worse than one that is not accepted. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: hamodywe --- CHANGELOG.md | 22 +++++ README.md | 34 ++++++- ROADMAP.md | 11 ++- docs/ci.md | 2 +- scripts/ci.sh | 11 +++ src/baseline.rs | 98 +++++++++++++++++--- src/cli.rs | 62 +++++++++++++ src/diff.rs | 214 ++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/report.rs | 6 +- src/run.rs | 87 ++++++++++++++++-- tests/cli.rs | 240 ++++++++++++++++++++++++++++++++++++++++++++++++ 12 files changed, 758 insertions(+), 30 deletions(-) create mode 100644 src/diff.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 184d875..d82706d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,28 @@ All notable changes to this project are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- **`--diff `.** Report only what the working copy adds relative to a git + ref, with no baseline file to keep up to date. The tree at the merge base of + `` and `HEAD` is checked out into a temporary worktree, analysed with + the same command and thresholds, and its findings become the baseline for + the run; the worktree is removed before the report prints. It is the merge + base rather than the tip of `` so a fix that landed on `main` after the + branch was cut does not make the branch look like it introduced the finding. + A finding that got worse than it was at the merge base is still reported, + exactly as with `--baseline`, and the two flags combine. The note at the end + of the report now names what hid the findings, and the merge base it used. + +### Changed + +- **`--baseline` and `--diff` are rejected on `cost` and `why`.** Neither + command reports findings a baseline could hide, so the flag was accepted and + changed nothing. A flag that silently does nothing is worse than one that is + not accepted, which is the rule the other flags already follow. + ## [0.2.0] — 2026-08-18 Three verdict bugs and a set of features that make the tool adoptable on a diff --git a/README.md b/README.md index 2ccd61b..2f96eae 100644 --- a/README.md +++ b/README.md @@ -247,6 +247,34 @@ reporting a finding that got *worse*, since a cycle that was benign and now crashes is new information about an old cycle. Suppressed counts are printed, so a quiet run is never mistaken for a clean one. +### `--diff ` — the same judgement, without a file to maintain + +```sh +overpull check --diff main --fail-on hazard +``` + +A baseline file is something somebody has to regenerate. `--diff` asks git +instead: the tree at the **merge base** of `` and `HEAD` is checked out +into a temporary worktree, analysed with the same command and thresholds, +and whatever it reports is the baseline for this run. The worktree is removed +before the report prints. + +It is the merge base, not the tip of ``, on purpose. A fix that landed on +`main` after your branch was cut is not something your branch removed, and a +finding `main` gained since is not something your branch added. Comparing +against the point the histories diverged judges the branch on its own +changes. The report names the commit it compared against: + +``` + 1 finding hidden by --diff main (merge base 1a2b3c4) (1 cycle, 0 barrels) +``` + +`--diff` and `--baseline` can be combined; a finding known to either is +hidden. Both apply to `barrels`, `cycles` and `check` — `cost` and `why` have +no findings to hide, and reject the flags rather than silently ignore them. +`--diff` needs `git` on `PATH`, and an `--entry` that does not exist yet at +the merge base is simply not an entry there. + ## CI ```yaml @@ -285,6 +313,8 @@ including `scripts/ci.sh`, which runs every check offline on a laptop. --json Machine-readable output --sarif SARIF 2.1.0, for code-scanning dashboards --baseline Suppress findings already recorded in this file +--diff Suppress findings already present at the merge base + of and HEAD (needs git) --fail-on never | crash | hazard | any --max-modules `cost` budget: exit 1 above this many modules --max-bytes `cost` budget in bytes; accepts 900kb, 2mb @@ -414,8 +444,8 @@ unresolved bare specifier is still counted as an external package, and unresolved relative imports are reported. **Why does `check` show fewer findings than I expected?** -If you passed `--baseline`, the run prints how many findings it hid and says -"no *new* import cycles" rather than "clean". If you did not, check the +If you passed `--baseline` or `--diff`, the run prints how many findings it +hid, and what hid them, and says "no *new* import cycles" rather than "clean". If you did not, check the `simulated from N entry points` line — the verdicts are only as good as the entry set, and `--entry` overrides it. diff --git a/ROADMAP.md b/ROADMAP.md index 64bbcd6..2209e30 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -30,12 +30,13 @@ scope. Nothing here is a promise of a date. Three verdict bugs went with them; see the [changelog](CHANGELOG.md#020--2026-08-18). -## Next +## Shipped — unreleased + +- **`--diff `.** Analyse the merge base of `` and `HEAD`, analyse + the working tree, report the difference. No file to keep up to date. See + the [changelog](CHANGELOG.md#unreleased). -**`--diff `.** A baseline is a file someone has to keep up to date. -Comparing against a git ref directly — analyse the merge base, analyse the -working tree, report the difference — removes that step. The baseline -machinery is already in place; this is the ergonomic half. +## Next **Workspace awareness.** Read npm/pnpm/Yarn workspace declarations and report per-package, rather than treating a monorepo as one flat tree. Entry-point diff --git a/docs/ci.md b/docs/ci.md index 6bd88e1..3ef5e5b 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -38,7 +38,7 @@ Stages: | `test` | `cargo test --all-targets` | | `msrv` | the version `rust-version` claims actually builds | | `fixtures` | Node throws where the fixtures say it throws, and loads where they say it loads | -| `behaviour` | the release binary's exit codes, and that `--json` / `--sarif` parse | +| `behaviour` | the release binary's exit codes, that `--json` / `--sarif` parse, and that `--diff HEAD` hides a committed finding and removes its checkout | | `dogfood` | the analyser survives a 1,500-module real graph (needs the network) | `msrv` is skipped rather than failed when the toolchain is not installed; the diff --git a/scripts/ci.sh b/scripts/ci.sh index 6372dc0..5f6c179 100644 --- a/scripts/ci.sh +++ b/scripts/ci.sh @@ -143,6 +143,17 @@ behaviour_checks() { printf ' json and sarif documents parse\n' "$binary" check --root tests/fixtures/barrel-project --json >/dev/null || return 1 "$binary" check --root tests/fixtures/crashing-cycle --sarif --fail-on never >/dev/null || return 1 + + # The crashing fixture is committed, so against HEAD it is nothing new; the + # checkout --diff makes must be gone again afterwards. + if have git; then + printf ' --diff HEAD hides a committed crash and cleans up after itself\n' + "$binary" cycles --root tests/fixtures/crashing-cycle --diff HEAD >/dev/null || return 1 + if [ "$(git worktree list --porcelain | grep -c '^worktree ')" -ne 1 ]; then + echo " expected the --diff worktree to be removed" >&2 + return 1 + fi + fi return 0 } diff --git a/src/baseline.rs b/src/baseline.rs index 1c7bb7c..8761d6c 100644 --- a/src/baseline.rs +++ b/src/baseline.rs @@ -6,6 +6,10 @@ //! there so a pull request is judged on what it adds. Findings that got //! *worse* than the baseline are still reported — a cycle that was benign //! and now crashes is new information about an old cycle. +//! +//! A baseline comes from one of two places: a JSON file a previous run +//! wrote, or the findings of the same command over the tree `--diff ` +//! names. Both end up here, in the same shape, judged by the same rules. use std::collections::HashMap; use std::fs; @@ -27,6 +31,45 @@ pub struct Baseline { const AMPLIFICATION_SLACK: f64 = 0.05; impl Baseline { + /// A baseline that knows nothing, so every finding is new. + pub fn empty() -> Self { + Self { + cycles: HashMap::new(), + barrels: HashMap::new(), + } + } + + /// Builds a baseline from the JSON documents a run would have printed — + /// what `--diff` produces from the comparison tree. Unlike a file, an + /// empty set of findings is a valid answer here: the other tree may + /// simply have been clean. + pub fn from_parts(parts: &[Value]) -> Self { + let mut baseline = Self::empty(); + for part in parts { + baseline.absorb(part); + } + baseline + } + + /// Combines two baselines so a finding known to either is known to the + /// result. Where both record the same finding, the worse reading is + /// kept: "not worse than what was already known" has to mean the most + /// that was already known. + pub fn merge(&mut self, other: Self) { + for (key, hazard) in other.cycles { + let entry = self.cycles.entry(key).or_insert(hazard); + if hazard > *entry { + *entry = hazard; + } + } + for (file, amplification) in other.barrels { + let entry = self.barrels.entry(file).or_insert(amplification); + if amplification > *entry { + *entry = amplification; + } + } + } + /// Reads a baseline from a document produced by `--json`, in either /// shape: a single command's output, or the `check` envelope. pub fn load(path: &Path) -> Result { @@ -41,10 +84,7 @@ impl Baseline { let document: Value = serde_json::from_str(&text) .map_err(|error| format!("--baseline `{}` is not JSON: {error}", path.display()))?; - let mut baseline = Self { - cycles: HashMap::new(), - barrels: HashMap::new(), - }; + let mut baseline = Self::empty(); match document.get("results").and_then(Value::as_array) { Some(parts) => { for part in parts { @@ -134,12 +174,7 @@ mod tests { use super::*; fn baseline_from(json: &str) -> Baseline { - let mut baseline = Baseline { - cycles: HashMap::new(), - barrels: HashMap::new(), - }; - baseline.absorb(&serde_json::from_str(json).unwrap()); - baseline + Baseline::from_parts(&[serde_json::from_str(json).unwrap()]) } #[test] @@ -170,12 +205,47 @@ mod tests { assert!(!baseline.covers_barrel("src/other.ts", 1.0)); } + #[test] + fn merging_keeps_the_worse_reading_of_a_shared_finding() { + let members = vec!["a.ts".to_string(), "b.ts".to_string()]; + let mut file = + baseline_from(r#"{"cycles":[{"hazard":"benign","members":["a.ts","b.ts"]}]}"#); + let tree = baseline_from( + r#"{"cycles":[{"hazard":"crash","members":["b.ts","a.ts"]}], + "barrels":[{"file":"src/index.ts","amplification":6.0}]}"#, + ); + file.merge(tree); + assert!( + file.covers_cycle(&members, Hazard::Crash), + "known to the tree" + ); + assert!( + file.covers_barrel("src/index.ts", 6.0), + "known only to the tree" + ); + assert!(!file.covers_barrel("src/other.ts", 1.0)); + + let mut reversed = + baseline_from(r#"{"cycles":[{"hazard":"crash","members":["a.ts","b.ts"]}]}"#); + reversed.merge(baseline_from( + r#"{"cycles":[{"hazard":"benign","members":["a.ts","b.ts"]}]}"#, + )); + assert!( + reversed.covers_cycle(&members, Hazard::Crash), + "merge order must not lose the worse reading" + ); + } + + #[test] + fn an_empty_set_of_parts_is_a_baseline_that_knows_nothing() { + let baseline = Baseline::from_parts(&[]); + assert!(!baseline.covers_cycle(&["a.ts".to_string()], Hazard::Benign)); + assert!(!baseline.covers_barrel("src/index.ts", 0.0)); + } + #[test] fn the_check_envelope_is_unwrapped() { - let mut baseline = Baseline { - cycles: HashMap::new(), - barrels: HashMap::new(), - }; + let mut baseline = Baseline::empty(); let document: Value = serde_json::from_str( r#"{"command":"check","results":[ {"barrels":[{"file":"src/index.ts","amplification":5.0}]}, diff --git a/src/cli.rs b/src/cli.rs index f945fad..3c1b9e2 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -41,6 +41,9 @@ pub struct Options { pub max_bytes: Option, /// Findings already recorded here are known, and are not reported again. pub baseline: Option, + /// A git ref. Findings present at the merge base of it and HEAD are + /// known, and are not reported again. + pub diff: Option, } #[derive(PartialEq, Eq, Clone, Copy)] @@ -94,6 +97,9 @@ OPTIONS --baseline Suppress findings already present in this file, so a first run on a large codebase reports only what is new. Create it with: overpull check --json > baseline.json + --diff Suppress findings already present at the merge base of + and HEAD, so a branch is judged on what it adds. + No file to keep up to date; needs git. --fail-on never | crash | hazard | any crash — throws on your own entry order (default for `cycles` and `check`) @@ -116,6 +122,7 @@ EXAMPLES overpull why src/legacy/config.ts overpull check --json > overpull.json overpull check --baseline overpull.json --fail-on hazard + overpull check --diff main --fail-on hazard EXIT CODES 0 no findings at or above --fail-on, and no budget exceeded @@ -185,6 +192,7 @@ impl Options { max_modules: None, max_bytes: None, baseline: None, + diff: None, } } } @@ -218,6 +226,15 @@ fn parse_flags>( "a file path", )?)); } + "--diff" => { + let reference = value(&mut args, "--diff", "a git ref")?; + if reference.starts_with('-') { + return Err(format!( + "--diff needs a git ref such as main or HEAD~1 (got `{reference}`)" + )); + } + options.diff = Some(reference); + } "--fail-on" => { let level = value(&mut args, "--fail-on", "a level")?; options.fail_on = match level.as_str() { @@ -310,6 +327,11 @@ fn validate(options: &Options) -> Option { { Some("--max-modules and --max-bytes apply to `overpull cost`".into()) } + // Only barrels and cycles are findings a baseline can hold; on + // `cost` and `why` either flag would be accepted and change nothing. + Command::Cost | Command::Why if options.baseline.is_some() || options.diff.is_some() => { + Some("--baseline and --diff apply to `overpull barrels`, `cycles` and `check`".into()) + } // A flag that silently does nothing is worse than one that is not // accepted: `cost` and `why` report no findings to gate on. Command::Cost if options.fail_on != FailOn::Never => Some( @@ -457,6 +479,46 @@ mod tests { ); } + #[test] + fn diff_takes_a_ref_and_rejects_a_flag_in_its_place() { + assert_eq!( + run_options(&["check", "--diff", "main"]).diff.as_deref(), + Some("main") + ); + assert!(matches!( + parse_args(&["check", "--diff"]), + ParseOutcome::Error(_) + )); + assert!(matches!( + parse_args(&["check", "--diff", "--json"]), + ParseOutcome::Error(_) + )); + } + + #[test] + fn baselines_belong_to_commands_that_report_findings() { + for flag in [&["--diff", "main"][..], &["--baseline", "b.json"][..]] { + let mut cost = vec!["cost", "a.ts"]; + cost.extend_from_slice(flag); + assert!( + matches!(parse_args(&cost), ParseOutcome::Error(_)), + "{flag:?}" + ); + let mut why = vec!["why", "a.ts"]; + why.extend_from_slice(flag); + assert!( + matches!(parse_args(&why), ParseOutcome::Error(_)), + "{flag:?}" + ); + let mut cycles = vec!["cycles"]; + cycles.extend_from_slice(flag); + assert!( + matches!(parse_args(&cycles), ParseOutcome::Run(_)), + "{flag:?}" + ); + } + } + #[test] fn json_and_sarif_are_mutually_exclusive() { assert!(matches!( diff --git a/src/diff.rs b/src/diff.rs new file mode 100644 index 0000000..e8deb31 --- /dev/null +++ b/src/diff.rs @@ -0,0 +1,214 @@ +//! `--diff ` — report only what a change adds, without a baseline file. +//! +//! A baseline is a file someone has to keep up to date. Most of the time the +//! question is simpler than that: what does this branch add that `main` did +//! not already have? Git holds the answer. The tree at the merge base of +//! `` and `HEAD` is checked out into a temporary worktree, analysed with +//! the same command and the same thresholds, and whatever it reports becomes +//! the baseline for the run over the working copy. +//! +//! The merge base, not the tip of ``. A fix that landed on `main` after +//! this branch was cut is not something the branch removed, and a finding +//! `main` gained since is not something the branch added. Comparing against +//! the commit where the histories diverged judges a branch on its own +//! changes and nothing else. + +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::{env, fs}; + +/// The tree the working copy is compared against, checked out for the +/// duration of one run and removed when dropped. +pub struct Comparison { + /// Full hash of the merge base, so the report can name what it compared + /// against. + pub merge_base: String, + /// Where `--root` lands inside the checked-out tree, or `None` when that + /// directory did not exist at the merge base — every finding is new then. + pub root: Option, + /// The working copy's `--root`, kept to map paths across. + working_root: PathBuf, + toplevel: PathBuf, + worktree: PathBuf, +} + +/// Distinguishes two concurrent runs in one process (the test suite) that +/// would otherwise race for the same temporary directory. +static CHECKOUT_COUNTER: AtomicU64 = AtomicU64::new(0); + +impl Comparison { + /// Checks out the merge base of `reference` and `HEAD` for the repository + /// that contains `root`. + pub fn checkout(root: &Path, reference: &str) -> Result { + let toplevel = match git(root, &["rev-parse", "--show-toplevel"]) { + Ok(path) => PathBuf::from(path.trim()), + Err(GitError::NotInstalled) => { + return Err("--diff needs git on PATH".into()); + } + Err(GitError::Failed(_)) => { + return Err(format!( + "--diff needs a git repository, and `{}` is not inside one", + root.display() + )); + } + }; + + let commitish = format!("{reference}^{{commit}}"); + if git(root, &["rev-parse", "--verify", "--quiet", &commitish]).is_err() { + return Err(format!( + "--diff `{reference}` is not a commit, branch or tag in this repository" + )); + } + + let merge_base = git(root, &["merge-base", reference, "HEAD"]) + .map(|hash| hash.trim().to_string()) + .map_err(|error| { + format!( + "--diff cannot find where `{reference}` and HEAD diverge: {}", + error.detail() + ) + })?; + + let prefix = git(root, &["rev-parse", "--show-prefix"]) + .map(|prefix| prefix.trim().to_string()) + .unwrap_or_default(); + + let worktree = env::temp_dir().join(format!( + "overpull-diff-{}-{}-{}", + std::process::id(), + CHECKOUT_COUNTER.fetch_add(1, Ordering::Relaxed), + &merge_base[..merge_base.len().min(12)] + )); + let worktree_text = worktree.to_string_lossy().into_owned(); + git( + &toplevel, + &["worktree", "add", "--detach", &worktree_text, &merge_base], + ) + .map_err(|error| { + format!( + "--diff could not check out `{}` for comparison: {}", + short(&merge_base), + error.detail() + ) + })?; + + let checked_out = if prefix.is_empty() { + worktree.clone() + } else { + worktree.join(prefix) + }; + let root_at_base = checked_out + .is_dir() + .then(|| crate::util::normalize(&checked_out)); + + Ok(Self { + merge_base, + root: root_at_base, + working_root: root.to_path_buf(), + toplevel, + worktree, + }) + } + + /// Where a file under the working copy's root sits in the checked-out + /// tree, if it exists there. Used to carry `--entry` and `--tsconfig` + /// across to the comparison run without pointing them at the wrong tree. + pub fn counterpart(&self, path: &Path) -> Option { + let root = self.root.as_ref()?; + let relative = crate::util::normalize(path) + .strip_prefix(&self.working_root) + .ok()? + .to_path_buf(); + let candidate = root.join(relative); + candidate + .is_file() + .then(|| crate::util::normalize(&candidate)) + } + + /// `main (merge base 1a2b3c4)` — the label the report uses for what was + /// compared against. + pub fn label(&self, reference: &str) -> String { + format!( + "--diff {reference} (merge base {})", + short(&self.merge_base) + ) + } +} + +impl Drop for Comparison { + fn drop(&mut self) { + let path = self.worktree.to_string_lossy().into_owned(); + if git(&self.toplevel, &["worktree", "remove", "--force", &path]).is_ok() { + return; + } + // The worktree could not be removed cleanly — take the directory + // away ourselves and let git drop the dangling registration, so the + // next run does not trip over a checkout that no longer exists. + let _ = fs::remove_dir_all(&self.worktree); + let _ = git(&self.toplevel, &["worktree", "prune"]); + } +} + +fn short(hash: &str) -> &str { + &hash[..hash.len().min(7)] +} + +enum GitError { + NotInstalled, + /// What git printed on stderr, trimmed. + Failed(String), +} + +impl GitError { + fn detail(&self) -> String { + match self { + Self::NotInstalled => "git is not on PATH".to_string(), + Self::Failed(stderr) if stderr.is_empty() => "git exited with an error".to_string(), + Self::Failed(stderr) => stderr.clone(), + } + } +} + +/// Runs git in `cwd` and returns stdout when it succeeds. +fn git(cwd: &Path, args: &[&str]) -> Result { + let output = Command::new("git") + .arg("-C") + .arg(cwd) + .args(args) + .output() + .map_err(|_| GitError::NotInstalled)?; + if output.status.success() { + Ok(String::from_utf8_lossy(&output.stdout).into_owned()) + } else { + Err(GitError::Failed( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn outside_a_repository_is_a_usage_error() { + let dir = env::temp_dir().join("overpull-diff-no-repo"); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + let Err(error) = Comparison::checkout(&dir, "HEAD") else { + panic!("a directory outside any repository cannot be compared"); + }; + assert!( + error.contains("git repository") || error.contains("git on PATH"), + "{error}" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn short_hash_never_slices_past_the_end() { + assert_eq!(short("abc"), "abc"); + assert_eq!(short("0123456789"), "0123456"); + } +} diff --git a/src/lib.rs b/src/lib.rs index 1969b14..2607f93 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,6 +8,7 @@ pub mod baseline; pub mod cli; pub mod cost; pub mod cycles; +pub mod diff; pub mod entries; pub mod graph; pub mod model; diff --git a/src/report.rs b/src/report.rs index 86df9b3..ae243fb 100644 --- a/src/report.rs +++ b/src/report.rs @@ -748,14 +748,16 @@ impl Reporter<'_> { // ---- shared -------------------------------------------------------- - pub fn suppressed_note(&self, suppressed: Suppressed) -> String { + /// `hidden_by` names what did the hiding: `the baseline`, or + /// `--diff main (merge base 1a2b3c4)`. + pub fn suppressed_note(&self, suppressed: Suppressed, hidden_by: &str) -> String { if suppressed.total() == 0 { return String::new(); } self.style.paint( DIM, &format!( - "\n {} finding{} hidden by the baseline ({} cycle{}, {} barrel{})\n", + "\n {} finding{} hidden by {hidden_by} ({} cycle{}, {} barrel{})\n", suppressed.total(), plural(suppressed.total()), suppressed.cycles, diff --git a/src/run.rs b/src/run.rs index 7c80eeb..52ae4a4 100644 --- a/src/run.rs +++ b/src/run.rs @@ -9,6 +9,7 @@ use crate::baseline::Baseline; use crate::cli::{Command, FailOn, Options}; use crate::cost::{self, EntryCostReport}; use crate::cycles::{self, CycleAnalysis, Hazard}; +use crate::diff::Comparison; use crate::entries::{self, Entry}; use crate::graph::ModuleGraph; use crate::report::Reporter; @@ -44,6 +45,8 @@ struct Analyses { cycles: Option, why: Option, suppressed: Suppressed, + /// What hid them, for the note at the end of the report. + hidden_by: String, budget_exceeded: bool, } @@ -83,6 +86,32 @@ impl Analyses { pub fn run(options: &Options) -> Result { let root = validate_root(options)?; + + // Both sources of known findings are settled before the working copy is + // analysed: a missing file or an unknown ref is a usage error, and a + // usage error should not cost a full graph build first. + let mut baseline = match &options.baseline { + Some(path) => Some(Baseline::load(path)?), + None => None, + }; + let mut hidden_by = Vec::new(); + if options.baseline.is_some() { + hidden_by.push("the baseline".to_string()); + } + if let Some(reference) = &options.diff { + let comparison = Comparison::checkout(&root, reference)?; + let known = known_at_merge_base(options, &comparison)?; + hidden_by.push(comparison.label(reference)); + baseline = Some(match baseline { + Some(mut file) => { + file.merge(known); + file + } + None => known, + }); + // The checkout is removed here, before the working copy is analysed. + } + let resolver = SpecResolver::new(options.tsconfig.as_deref()); let seeds = seed_files(options, &root)?; let graph = ModuleGraph::build(&root, &seeds, &resolver); @@ -90,13 +119,10 @@ pub fn run(options: &Options) -> Result { return Err("nothing to analyze: no module could be parsed".into()); } - let baseline = match &options.baseline { - Some(path) => Some(Baseline::load(path)?), - None => None, - }; let explicit = module_indices(&graph, &root, &options.entry_files, "--entry")?; let entry_set = entries::classify(&graph, &explicit); - let analyses = analyze(options, &graph, &root, &entry_set, baseline.as_ref())?; + let mut analyses = analyze(options, &graph, &root, &entry_set, baseline.as_ref())?; + analyses.hidden_by = hidden_by.join(" and "); let style = if options.json || options.sarif { Style::plain() @@ -244,11 +270,60 @@ fn render( analyses.suppressed.cycles, )); } - text.push_str(&reporter.suppressed_note(analyses.suppressed)); + text.push_str(&reporter.suppressed_note(analyses.suppressed, &analyses.hidden_by)); text.push('\n'); text } +/// Runs the same command over the tree at the merge base and returns what +/// it found, in the shape `--baseline` reads. The comparison tree is judged +/// with the working copy's thresholds and entry overrides, so a finding is +/// "already there" only if this run would have reported it there too. +fn known_at_merge_base(options: &Options, comparison: &Comparison) -> Result { + let Some(root) = comparison.root.as_deref() else { + return Ok(Baseline::empty()); + }; + let tsconfig = options + .tsconfig + .as_deref() + .and_then(|path| comparison.counterpart(path)); + let resolver = SpecResolver::new(tsconfig.as_deref()); + let seeds = discover_sources(root); + if seeds.is_empty() { + return Ok(Baseline::empty()); + } + let graph = ModuleGraph::build(root, &seeds, &resolver); + if graph.modules.is_empty() { + return Ok(Baseline::empty()); + } + + // An --entry that did not exist yet at the merge base is simply not an + // entry there; it is not an error, and it is not moved to another file. + let explicit: Vec = options + .entry_files + .iter() + .filter_map(|entry| { + let candidate = if Path::new(entry).is_absolute() { + comparison.counterpart(Path::new(entry))? + } else { + crate::util::normalize(&root.join(entry)) + }; + graph.index_by_path.get(&candidate).copied() + }) + .collect(); + let entry_set = entries::classify(&graph, &explicit); + let analyses = analyze(options, &graph, root, &entry_set, None)?; + + let reporter = Reporter { + graph: &graph, + style: Style::plain(), + top: options.top, + }; + Ok(Baseline::from_parts(&json_parts( + &reporter, options, &analyses, + ))) +} + fn json_parts(reporter: &Reporter, options: &Options, analyses: &Analyses) -> Vec { let mut parts = Vec::new(); if options.command == Command::Cost { diff --git a/tests/cli.rs b/tests/cli.rs index ade7fd5..4586b70 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -435,3 +435,243 @@ fn an_entry_override_changes_the_verdict_it_should_change() { "the unsafe cycle is unreachable from the safe entry point" ); } + +// ---- --diff --------------------------------------------------------- +// +// These build a real repository in a temporary directory, because the whole +// point of --diff is what git says the merge base is. Every git call disables +// commit signing and sets an identity, so the tests do not depend on the +// machine's configuration. + +struct Repo { + dir: PathBuf, +} + +impl Repo { + fn new(name: &str) -> Self { + let dir = std::env::temp_dir().join(format!("overpull-cli-{name}")); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let repo = Self { dir }; + repo.git(&["init", "-q"]); + repo.git(&["checkout", "-q", "-b", "main"]); + repo + } + + fn git(&self, args: &[&str]) { + let status = Command::new("git") + .arg("-C") + .arg(&self.dir) + .args([ + "-c", + "user.name=overpull tests", + "-c", + "user.email=tests@overpull.invalid", + "-c", + "commit.gpgsign=false", + ]) + .args(args) + .status() + .expect("git is needed for the --diff tests"); + assert!(status.success(), "git {args:?} failed"); + } + + fn commit(&self, message: &str) { + self.git(&["add", "-A"]); + self.git(&["commit", "-q", "-m", message]); + } + + fn write(&self, name: &str, contents: &str) { + std::fs::write(self.dir.join(name), contents).unwrap(); + } + + /// The crashing-cycle fixture, dropped into the repository as-is. + fn add_crashing_cycle(&self) { + for name in ["entry.mjs", "registry.mjs", "service.mjs"] { + std::fs::copy(fixture("crashing-cycle").join(name), self.dir.join(name)).unwrap(); + } + } + + fn path(&self) -> &str { + self.dir.to_str().unwrap() + } + + fn worktrees(&self) -> usize { + let output = Command::new("git") + .arg("-C") + .arg(&self.dir) + .args(["worktree", "list", "--porcelain"]) + .output() + .unwrap(); + String::from_utf8_lossy(&output.stdout) + .lines() + .filter(|line| line.starts_with("worktree ")) + .count() + } +} + +impl Drop for Repo { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.dir); + } +} + +#[test] +fn diff_reports_only_what_the_working_copy_adds() { + let repo = Repo::new("diff-adds"); + repo.write("entry.mjs", "export const x = 1;\n"); + repo.commit("clean"); + + // The crash is uncommitted, so it is new relative to HEAD. + repo.add_crashing_cycle(); + let uncommitted = run(&["cycles", "--root", repo.path(), "--diff", "HEAD"]); + assert_eq!( + uncommitted.status.code(), + Some(1), + "{}", + stdout(&uncommitted) + ); + assert!(stdout(&uncommitted).contains("crash")); + + // Committed, the same crash is part of what HEAD already had. + repo.commit("crash"); + let committed = run(&["cycles", "--root", repo.path(), "--diff", "HEAD"]); + assert_eq!(committed.status.code(), Some(0), "{}", stdout(&committed)); + let text = stdout(&committed); + assert!( + text.contains("hidden by --diff HEAD (merge base "), + "{text}" + ); + assert!( + text.contains("no new import cycles"), + "a suppressed run must not claim to be clean: {text}" + ); + assert_eq!( + repo.worktrees(), + 1, + "the comparison checkout must be removed" + ); +} + +#[test] +fn diff_compares_against_the_merge_base_not_the_tip() { + let repo = Repo::new("diff-merge-base"); + repo.add_crashing_cycle(); + repo.commit("crash on main"); + + // A branch is cut with the crash in it, and main fixes the crash later. + repo.git(&["checkout", "-q", "-b", "feature"]); + repo.write("extra.mjs", "export const extra = 1;\n"); + repo.commit("unrelated work on the branch"); + repo.git(&["checkout", "-q", "main"]); + repo.write("registry.mjs", "export function register() {}\n"); + repo.commit("fix the crash on main"); + repo.git(&["checkout", "-q", "feature"]); + + // Against the tip of main the crash would look new; against the merge + // base it is what the branch started from, which is the honest answer. + let output = run(&["cycles", "--root", repo.path(), "--diff", "main"]); + assert_eq!(output.status.code(), Some(0), "{}", stdout(&output)); + assert!(stdout(&output).contains("hidden by --diff main")); +} + +#[test] +fn diff_treats_a_finding_that_got_worse_as_new() { + let repo = Repo::new("diff-worse"); + // The same two files as the crashing fixture, in a cycle that only reads + // across the loop inside a function: benign, and committed as such. + repo.write( + "entry.mjs", + "import { start } from \"./service.mjs\"; +start(); +", + ); + repo.write( + "service.mjs", + "import { register } from \"./registry.mjs\"; + export const SERVICE_NAME = \"svc\"; + export function start() { + register(SERVICE_NAME); +} +", + ); + repo.write( + "registry.mjs", + "import { SERVICE_NAME } from \"./service.mjs\"; + export function register(name) { + return name === SERVICE_NAME; +} +", + ); + repo.commit("benign cycle"); + let benign = run(&[ + "cycles", + "--root", + repo.path(), + "--diff", + "HEAD", + "--fail-on", + "any", + ]); + assert_eq!( + benign.status.code(), + Some(0), + "the committed benign cycle is known: {}", + stdout(&benign) + ); + + // Same members, now with a load-time read: worse than what was known, + // so it is reported even though the cycle itself is not new. + repo.add_crashing_cycle(); + let worse = run(&["cycles", "--root", repo.path(), "--diff", "HEAD"]); + assert_eq!(worse.status.code(), Some(1), "{}", stdout(&worse)); + assert!(stdout(&worse).contains("crash")); +} + +#[test] +fn diff_names_an_unknown_ref_and_a_missing_repository() { + let repo = Repo::new("diff-errors"); + repo.write("entry.mjs", "export const x = 1;\n"); + repo.commit("clean"); + + let unknown = run(&["cycles", "--root", repo.path(), "--diff", "no-such-branch"]); + assert_eq!(unknown.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&unknown.stderr).contains("no-such-branch")); + + let outside = std::env::temp_dir().join("overpull-cli-diff-no-repo"); + let _ = std::fs::remove_dir_all(&outside); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("entry.mjs"), "export const x = 1;\n").unwrap(); + let missing = run(&[ + "cycles", + "--root", + outside.to_str().unwrap(), + "--diff", + "HEAD", + ]); + assert_eq!(missing.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&missing.stderr).contains("git repository")); + let _ = std::fs::remove_dir_all(&outside); +} + +#[test] +fn diff_and_a_baseline_file_both_hide_findings() { + let repo = Repo::new("diff-and-baseline"); + repo.add_crashing_cycle(); + repo.commit("crash"); + let baseline = repo.dir.join("baseline.json"); + let recorded = run(&["cycles", "--root", repo.path(), "--json"]); + std::fs::write(&baseline, stdout(&recorded)).unwrap(); + + let output = run(&[ + "cycles", + "--root", + repo.path(), + "--diff", + "HEAD", + "--baseline", + baseline.to_str().unwrap(), + ]); + assert_eq!(output.status.code(), Some(0), "{}", stdout(&output)); + assert!(stdout(&output).contains("hidden by the baseline and --diff HEAD")); +}