Skip to content

Commit 4308f8f

Browse files
committed
test: add integration tests for git diff --quiet optimization
1 parent b1e9d20 commit 4308f8f

File tree

1 file changed

+140
-3
lines changed

1 file changed

+140
-3
lines changed

crates/prek/tests/skipped_hooks.rs

Lines changed: 140 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,13 +206,150 @@ fn all_hooks_skipped_multiple_priority_groups() -> Result<()> {
206206
assert!(stdout.contains("priority-20") && stdout.contains("Skipped"));
207207
assert!(stdout.contains("priority-30") && stdout.contains("Skipped"));
208208

209-
// Regression test for #1335: only 1 get_diff call (initial baseline)
210-
// Without fix: 4 calls (1 initial + 3 per priority group)
209+
// Optimization: when worktree is cleaned (stashed) and all hooks skip, no diff needed.
210+
// - #1335 fix: skip diff check when all hooks in a group skip
211+
// - --quiet optimization: use has_worktree_changes instead of get_diff
212+
// Combined: 0 get_diff calls when all hooks skip in normal workflow
211213
let stderr = String::from_utf8_lossy(&output.stderr);
212214
let get_diff_calls = stderr.matches("get_diff").count();
215+
assert_eq!(
216+
get_diff_calls, 0,
217+
"Expected 0 get_diff calls when all hooks skip in stashed workflow.\n\
218+
Found {get_diff_calls} get_diff calls.\n\
219+
Trace output:\n{stderr}"
220+
);
221+
222+
Ok(())
223+
}
224+
225+
/// Optimization test: when worktree is stashed (normal workflow), use `--quiet`
226+
/// check instead of full diff comparison.
227+
///
228+
/// In normal pre-commit workflow:
229+
/// - Unstaged changes are stashed, so worktree matches index
230+
/// - After hooks run, we only need to check IF changes exist (boolean)
231+
/// - `git diff --quiet` (exit code) is faster than capturing/comparing full diff
232+
///
233+
/// This test verifies:
234+
/// 1. `has_worktree_changes` is called (not `get_diff`) when hooks run
235+
/// 2. File modifications by hooks are still detected correctly
236+
#[test]
237+
fn uses_has_worktree_changes_when_stashed() -> Result<()> {
238+
let context = TestContext::new();
239+
context.init_project();
240+
241+
let cwd = context.work_dir();
242+
243+
// Hook that modifies files (triggers modification detection)
244+
context.write_pre_commit_config(indoc::indoc! {r#"
245+
repos:
246+
- repo: local
247+
hooks:
248+
- id: modifier
249+
name: modifier
250+
language: system
251+
entry: python3 -c "open('file.txt', 'a').write('modified')"
252+
files: \.txt$
253+
"#});
254+
255+
cwd.child("file.txt").write_str("original")?;
256+
context.git_add(".");
257+
258+
let output = context.run().env("RUST_LOG", "prek::git=trace").output()?;
259+
260+
// Hook should fail because it modified files
261+
assert!(
262+
!output.status.success(),
263+
"hook should fail due to file modification"
264+
);
265+
266+
let stderr = String::from_utf8_lossy(&output.stderr);
267+
268+
// Optimization: use has_worktree_changes for first check (avoids initial get_diff).
269+
// When changes are detected, get_diff is called to capture state for subsequent groups.
270+
let has_worktree_changes_calls = stderr.matches("has_worktree_changes").count();
271+
let get_diff_calls = stderr.matches("get_diff").count();
272+
273+
assert!(
274+
has_worktree_changes_calls >= 1,
275+
"Expected has_worktree_changes to be called for first modification check.\n\
276+
Found {has_worktree_changes_calls} has_worktree_changes calls, {get_diff_calls} get_diff calls.\n\
277+
Trace output:\n{stderr}"
278+
);
279+
280+
// Optimization benefit: 1 get_diff call (after change detection) vs 2 (before + after)
281+
// The initial get_diff is replaced by the cheaper has_worktree_changes check.
213282
assert_eq!(
214283
get_diff_calls, 1,
215-
"Expected 1 get_diff call (initial baseline) when all hooks skip, found {get_diff_calls}.\n\
284+
"Expected 1 get_diff call (to capture state after change detection).\n\
285+
Found {get_diff_calls} get_diff calls.\n\
286+
Trace output:\n{stderr}"
287+
);
288+
289+
Ok(())
290+
}
291+
292+
/// When using `--all-files`, worktree is NOT stashed, so full diff comparison is needed.
293+
///
294+
/// With --all-files, the worktree may have existing unstaged changes. To detect if
295+
/// hooks added MORE changes, we must compare before/after diffs (not just check
296+
/// existence). This ensures correctness for the edge case where a file is already
297+
/// modified and a hook modifies it further.
298+
#[test]
299+
fn uses_get_diff_when_all_files() -> Result<()> {
300+
let context = TestContext::new();
301+
context.init_project();
302+
303+
let cwd = context.work_dir();
304+
305+
// Hook that modifies files
306+
context.write_pre_commit_config(indoc::indoc! {r#"
307+
repos:
308+
- repo: local
309+
hooks:
310+
- id: modifier
311+
name: modifier
312+
language: system
313+
entry: python3 -c "open('file.txt', 'a').write('modified')"
314+
files: \.txt$
315+
"#});
316+
317+
cwd.child("file.txt").write_str("original")?;
318+
// Stage and commit so file is tracked, then use --all-files
319+
context.git_add(".");
320+
context.configure_git_author();
321+
context.git_commit("initial");
322+
323+
let output = context
324+
.run()
325+
.arg("--all-files")
326+
.env("RUST_LOG", "prek::git=trace")
327+
.output()?;
328+
329+
// Hook should fail because it modified files
330+
assert!(
331+
!output.status.success(),
332+
"hook should fail due to file modification"
333+
);
334+
335+
let stderr = String::from_utf8_lossy(&output.stderr);
336+
337+
// With --all-files (no stash), should use get_diff for comparison
338+
let get_diff_calls = stderr.matches("get_diff").count();
339+
let has_worktree_changes_calls = stderr.matches("has_worktree_changes").count();
340+
341+
assert!(
342+
get_diff_calls >= 1,
343+
"Expected get_diff to be called for --all-files workflow.\n\
344+
Found {get_diff_calls} get_diff calls, {has_worktree_changes_calls} has_worktree_changes calls.\n\
345+
Trace output:\n{stderr}"
346+
);
347+
348+
// has_worktree_changes should NOT be used in --all-files workflow
349+
assert_eq!(
350+
has_worktree_changes_calls, 0,
351+
"Expected 0 has_worktree_changes calls in --all-files workflow.\n\
352+
Found {has_worktree_changes_calls} has_worktree_changes calls.\n\
216353
Trace output:\n{stderr}"
217354
);
218355

0 commit comments

Comments
 (0)