Skip to content

Avoid duplicate diffs after hooks rewrite files #2815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
j178 merged 1 commit into from
Oct 1, 2026
Merged

Avoid duplicate diffs after hooks rewrite files #2815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 3 additions & 25 deletions crates/prek/src/cli/run/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ pub(crate) struct DiffTracker<'a> {
}

enum DiffBaseline {
Clean,
Unknown,
Snapshot(Vec<u8>),
}
Expand All @@ -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()),
}
}

Expand All @@ -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;
Expand Down
26 changes: 0 additions & 26 deletions crates/prek/src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,32 +530,6 @@ async fn parse_merge_msg_for_conflicts() -> Result<Vec<PathBuf>> {
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<bool, Error> {
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
Expand Down
191 changes: 42 additions & 149 deletions crates/prek/tests/run/modifications.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
✓ <workspace>
root-noop..............................................................Passed

----- stderr -----
"#);
}

#[test]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(())
Expand Down
Loading