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")); +}