From 7189ba01e3de5a8ddeed40a39c48d1ceedbb119f Mon Sep 17 00:00:00 2001 From: Jo <10510431+j178@users.noreply.github.com> Date: Thu, 1 Oct 2026 09:59:02 +0800 Subject: [PATCH] Avoid duplicate diffs after hooks rewrite files --- crates/prek/src/cli/run/diff.rs | 28 +--- crates/prek/src/git.rs | 26 ---- crates/prek/tests/run/modifications.rs | 191 ++++++------------------- 3 files changed, 45 insertions(+), 200 deletions(-) diff --git a/crates/prek/src/cli/run/diff.rs b/crates/prek/src/cli/run/diff.rs index 93846df7e..6cf410b82 100644 --- a/crates/prek/src/cli/run/diff.rs +++ b/crates/prek/src/cli/run/diff.rs @@ -11,7 +11,6 @@ pub(crate) struct DiffTracker<'a> { } enum DiffBaseline { - Clean, Unknown, Snapshot(Vec), } @@ -21,7 +20,7 @@ impl<'a> DiffTracker<'a> { pub(crate) fn clean_baseline(path: &'a Path) -> Self { Self { path, - baseline: DiffBaseline::Clean, + baseline: DiffBaseline::Snapshot(Vec::new()), } } @@ -48,30 +47,9 @@ impl<'a> DiffTracker<'a> { } match &mut self.baseline { - DiffBaseline::Clean => { - // `WorktreeStash` already removed unstaged changes. A quiet - // worktree check keeps the common no-op path cheap. - if !git::has_worktree_diff(self.path).await? { - return Ok(false); - } - // `diff-files --quiet` is stat-based, so an in-place rewrite - // can look dirty even when the content is unchanged. Do a full - // diff here to ignore stat-only changes and reuse the content - // diff as the baseline if the hook really modified files. - let curr_diff = git::diff_worktree(self.path).await?; - if curr_diff.is_empty() { - return Ok(false); - } - - // Capture the dirty state after this group so later groups can - // compare against the exact diff left by previous hooks. - self.baseline = DiffBaseline::Snapshot(curr_diff); - Ok(true) - } DiffBaseline::Snapshot(prev_diff) => { - // Unknown initial state, `--all-files`, and later dirty groups - // need a full before/after diff comparison to avoid confusing - // pre-existing user changes with hook changes. + // One patch both detects changes and becomes the next baseline. A + // separate quiet check would repeat Git work whenever files are rewritten. let curr_diff = git::diff_worktree(self.path).await?; let modified = curr_diff != *prev_diff; *prev_diff = curr_diff; diff --git a/crates/prek/src/git.rs b/crates/prek/src/git.rs index b2d948e8c..1c17dad3f 100644 --- a/crates/prek/src/git.rs +++ b/crates/prek/src/git.rs @@ -530,32 +530,6 @@ async fn parse_merge_msg_for_conflicts() -> Result> { Ok(conflicts) } -/// Check for unstaged changes under the absolute directory `path`, ignoring submodules. -#[instrument(level = "trace")] -pub(crate) async fn has_worktree_diff(path: &Path) -> Result { - let mut cmd = git_cmd()?; - let status = cmd - .current_dir(path) - .arg("diff-files") - .arg("--quiet") - .hidden_args(["--no-ext-diff", "--no-textconv", "--ignore-submodules"]) - .arg("--") - .arg(path) - .check(false) - .status() - .await?; - - if status.success() { - return Ok(false); - } - if status.code() == Some(1) { - return Ok(true); - } - - cmd.check_status(status)?; - Ok(true) -} - /// Return a patch for unstaged changes under the absolute directory `path`. /// /// File names are repository-relative by default, or relative to `path` when diff --git a/crates/prek/tests/run/modifications.rs b/crates/prek/tests/run/modifications.rs index 536b7ce86..d295dea22 100644 --- a/crates/prek/tests/run/modifications.rs +++ b/crates/prek/tests/run/modifications.rs @@ -5,7 +5,7 @@ use std::time::{Duration, SystemTime}; use anyhow::Result; use assert_fs::prelude::*; -use crate::common::TestEnv; +use crate::common::{TestEnv, cmd_snapshot}; fn remove_loose_blob(context: &TestEnv, filename: &str) -> Result<()> { let output = context @@ -26,46 +26,9 @@ fn remove_loose_blob(context: &TestEnv, filename: &str) -> Result<()> { Ok(()) } -#[test] -fn external_hook_without_changes_uses_quiet_diff_check() -> Result<()> { - let context = TestEnv::new() - .with_config(indoc::indoc! {r#" - repos: - - repo: local - hooks: - - id: noop - name: noop - language: system - entry: python3 -c "pass" - pass_filenames: false - "#}) - .with_file("file.txt", "original\n") - .init_git(); - - let output = context.run().env("RUST_LOG", "prek::git=trace").output()?; - - assert!(output.status.success(), "noop hook should pass"); - - let stderr = String::from_utf8_lossy(&output.stderr); - let has_worktree_diff_calls = stderr.matches("has_worktree_diff").count(); - assert_eq!( - has_worktree_diff_calls, 1, - "Expected one cheap worktree diff check, found {has_worktree_diff_calls}.\n\ - Trace output:\n{stderr}" - ); - let diff_worktree_calls = stderr.matches("diff_worktree").count(); - assert_eq!( - diff_worktree_calls, 0, - "Expected no full diff_worktree calls when the hook leaves files unchanged, found {diff_worktree_calls}.\n\ - Trace output:\n{stderr}" - ); - - Ok(()) -} - #[test] #[cfg(unix)] -fn identical_rewrite_with_stat_change_is_not_modified() -> Result<()> { +fn identical_rewrite_with_stat_change_is_not_modified() { let context = TestEnv::new() .with_config(indoc::indoc! {r" repos: @@ -95,37 +58,26 @@ fn identical_rewrite_with_stat_change_is_not_modified() -> Result<()> { .with_file("file.txt", "original\n") .init_git(); - let output = context.run().env("RUST_LOG", "prek::git=trace").output()?; - - assert!( - output.status.success(), - "rewriting identical content should not be treated as a hook modification" - ); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("rewrite-identical") && stdout.contains("Passed")); - assert!(!stdout.contains("files were modified by this hook")); - - let stderr = String::from_utf8_lossy(&output.stderr); - let has_worktree_diff_calls = stderr.matches("has_worktree_diff").count(); - assert_eq!( - has_worktree_diff_calls, 1, - "Expected one cheap worktree diff check, found {has_worktree_diff_calls}.\n\ - Trace output:\n{stderr}" - ); - - let diff_worktree_calls = stderr.matches("diff_worktree").count(); - assert_eq!( - diff_worktree_calls, 1, - "Expected one content diff to filter out stat-only changes, found {diff_worktree_calls}.\n\ - Trace output:\n{stderr}" - ); - - Ok(()) + for auto_refresh in ["true", "false"] { + context + .git() + .run(["config", "diff.autoRefreshIndex", auto_refresh]); + + insta::allow_duplicates! { + cmd_snapshot!(context, context.run(), @r#" + success: true + exit_code: 0 + ----- stdout ----- + rewrite-identical........................................................Passed + + ----- stderr ----- + "#); + } + } } #[test] -fn modifying_hook_uses_clean_baseline_diff_detection() -> Result<()> { +fn modifying_hook_is_reported() { let context = TestEnv::new() .with_config(indoc::indoc! {r#" repos: @@ -139,32 +91,16 @@ fn modifying_hook_uses_clean_baseline_diff_detection() -> Result<()> { "#}) .with_file("file.txt", "original\n").init_git(); - let output = context.run().env("RUST_LOG", "prek::git=trace").output()?; + cmd_snapshot!(context, context.run(), @r#" + success: false + exit_code: 1 + ----- stdout ----- + modify...................................................................Failed + - hook id: modify + - files were modified by this hook - assert!( - !output.status.success(), - "prek should fail when hooks modify files" - ); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("files were modified by this hook")); - - let stderr = String::from_utf8_lossy(&output.stderr); - let has_worktree_diff_calls = stderr.matches("has_worktree_diff").count(); - assert_eq!( - has_worktree_diff_calls, 1, - "Expected one cheap worktree diff check, found {has_worktree_diff_calls}.\n\ - Trace output:\n{stderr}" - ); - - let diff_worktree_calls = stderr.matches("diff_worktree").count(); - assert_eq!( - diff_worktree_calls, 1, - "Expected one full diff_worktree call after detecting modifications, found {diff_worktree_calls}.\n\ - Trace output:\n{stderr}" - ); - - Ok(()) + ----- stderr ----- + "#); } #[test] @@ -263,13 +199,6 @@ fn all_files_with_existing_unstaged_changes_uses_snapshot_baseline() -> Result<( assert!(stdout.contains("files were modified by this hook")); let stderr = String::from_utf8_lossy(&output.stderr); - let has_worktree_diff_calls = stderr.matches("has_worktree_diff").count(); - assert_eq!( - has_worktree_diff_calls, 0, - "`--all-files` should not use the clean-baseline diff check.\n\ - Trace output:\n{stderr}" - ); - let diff_worktree_calls = stderr.matches("diff_worktree").count(); assert_eq!( diff_worktree_calls, 2, @@ -342,7 +271,7 @@ fn all_files_clean_missing_blob_ignores_diff_snapshot_errors() -> Result<()> { } #[test] -fn later_project_snapshots_diff_left_by_previous_project() -> Result<()> { +fn later_project_snapshots_diff_left_by_previous_project() { let context = TestEnv::new() .with_config(indoc::indoc! {r#" repos: @@ -371,30 +300,19 @@ fn later_project_snapshots_diff_left_by_previous_project() -> Result<()> { ) .with_file("child/child.txt", "original\n").init_git(); - let output = context.run().env("RUST_LOG", "prek::git=trace").output()?; - - assert!( - !output.status.success(), - "prek should fail because the child hook modified files" - ); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("child-modify") && stdout.contains("files were modified by this hook")); - assert!( - stdout.contains("root-noop") && stdout.contains("Passed"), - "root hook should not be blamed for the child project's diff.\n\ - stdout:\n{stdout}" - ); - - let stderr = String::from_utf8_lossy(&output.stderr); - let has_worktree_diff_calls = stderr.matches("has_worktree_diff").count(); - assert_eq!( - has_worktree_diff_calls, 1, - "Only the first project should use the clean-baseline check.\n\ - Trace output:\n{stderr}" - ); - - Ok(()) + cmd_snapshot!(context, context.run(), @r#" + success: false + exit_code: 1 + ----- stdout ----- + × child + child-modify...........................................................Failed + - hook id: child-modify + - files were modified by this hook + ✓ + root-noop..............................................................Passed + + ----- stderr ----- + "#); } #[test] @@ -463,12 +381,6 @@ fn read_only_languages_do_not_run_diff_detection() -> Result<()> { assert!(!stdout.contains("files were modified by this hook")); let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!( - stderr.matches("has_worktree_diff").count(), - 0, - "Read-only languages should not require a worktree diff check.\n\ - Trace output:\n{stderr}" - ); assert_eq!( stderr.matches("diff_worktree").count(), 0, @@ -516,12 +428,6 @@ fn same_group_known_modification_skips_diff_detection() -> Result<()> { ); let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!( - stderr.matches("has_worktree_diff").count(), - 0, - "A known modification should make the same-group quiet diff unnecessary.\n\ - Trace output:\n{stderr}" - ); assert_eq!( stderr.matches("diff_worktree").count(), 0, @@ -575,12 +481,6 @@ fn same_group_known_modification_rebaselines_later_external_hook() -> Result<()> ); let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!( - stderr.matches("has_worktree_diff").count(), - 0, - "The known modification should invalidate the clean baseline.\n\ - Trace output:\n{stderr}" - ); assert_eq!( stderr.matches("diff_worktree").count(), 2, @@ -627,12 +527,6 @@ fn modifying_builtin_invalidates_baseline_for_later_external_hook() -> Result<() assert!(stdout.contains("noop") && stdout.contains("Passed")); let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!( - stderr.matches("has_worktree_diff").count(), - 0, - "The builtin result and dirty baseline should avoid the clean-worktree check.\n\ - Trace output:\n{stderr}" - ); assert_eq!( stderr.matches("diff_worktree").count(), 2, @@ -667,7 +561,6 @@ fn failed_non_modifying_builtin_skips_diff_detection() -> Result<()> { assert!(!stdout.contains("files were modified by this hook")); let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!(stderr.matches("has_worktree_diff").count(), 0); assert_eq!(stderr.matches("diff_worktree").count(), 0); Ok(())