diff --git a/crates/prek/src/git.rs b/crates/prek/src/git.rs index 7dd991ac5..d70096c11 100644 --- a/crates/prek/src/git.rs +++ b/crates/prek/src/git.rs @@ -390,9 +390,22 @@ pub(crate) async fn get_diff(path: &Path) -> Result, Error> { .arg("--ignore-submodules") .arg("--") .arg(path) - .check(true) + // This diff is only used as a best-effort before/after snapshot of + // hook changes. Some CI environments keep enough of `.git` for + // `git ls-files` but omit blob objects needed by `git diff`; Git then + // exits 128 on stderr with empty stdout. Keep comparing stdout in that + // case so `run --all-files` can still run against the files Git can + // enumerate. + .check(false) .output() .await?; + if !output.status.success() { + debug!( + status = %output.status, + stderr = %String::from_utf8_lossy(&output.stderr), + "Continuing with git diff stdout despite non-zero exit status" + ); + } Ok(output.stdout) } diff --git a/crates/prek/tests/run.rs b/crates/prek/tests/run.rs index c03c45761..9c04a7057 100644 --- a/crates/prek/tests/run.rs +++ b/crates/prek/tests/run.rs @@ -2824,15 +2824,16 @@ fn git_commit_a_currently_fails_when_hook_writes_to_temp_git_index() -> Result<( let context = TestContext::new(); context.init_project(); - // Repro for #1786 documenting the current behavior. - // `git commit -a` exports `GIT_INDEX_FILE=.git/index.lock` to the pre-commit hook - // process. If the hook inherits that env var and then runs a git command that writes - // to an index in a different repository, Git will write those entries into the parent - // repo's temporary index instead. + // Repro for #1786 documenting the current behavior. `git commit -a` + // exports `GIT_INDEX_FILE=.git/index.lock` to the hook process. If the + // hook inherits that env var and then runs a git command that writes to an + // index in a different repository, Git writes those entries into the + // parent repo's temporary index instead. // // The important detail is that the temp repo stages `file.txt`, matching a tracked - // path in the parent repo. That makes prek's post-hook `git diff` read the corrupted - // parent index entry and fail with `fatal: unable to read `. + // path in the parent repo. `prek` treats the post-hook diff as a best-effort + // snapshot, so the commit continues until Git tries to build trees from the + // corrupted temporary index and fails with `invalid object ... for 'file.txt'`. context .work_dir() .child("hook.sh") @@ -2893,8 +2894,8 @@ fn git_commit_a_currently_fails_when_hook_writes_to_temp_git_index() -> Result<( .chain([ (r"\[master \w{7}\]", r"[master COMMIT]"), ( - r"fatal: unable to read [0-9a-f]{40}", - "fatal: unable to read [HASH]", + r"invalid object 100644 [0-9a-f]{40}", + "invalid object 100644 [HASH]", ), ]) .collect::>(); @@ -2905,13 +2906,11 @@ fn git_commit_a_currently_fails_when_hook_writes_to_temp_git_index() -> Result<( ----- stdout ----- ----- stderr ----- - error: Command `git diff` exited with an error: - - [status] - exit status: 128 - - [stderr] - fatal: unable to read [HASH] + write-temp-index.........................................................Passed + - hook id: write-temp-index + - duration: [TIME] + error: invalid object 100644 [HASH] for 'file.txt' + error: Error building trees " ); diff --git a/crates/prek/tests/skipped_hooks.rs b/crates/prek/tests/skipped_hooks.rs index d890b0f83..0236653c2 100644 --- a/crates/prek/tests/skipped_hooks.rs +++ b/crates/prek/tests/skipped_hooks.rs @@ -7,10 +7,12 @@ //! Includes regression tests for #1335: when all hooks in a group are skipped, //! prek should not call `git diff` to check for file modifications. +use std::time::{Duration, SystemTime}; + use anyhow::Result; use assert_fs::prelude::*; -use crate::common::{TestContext, cmd_snapshot}; +use crate::common::{TestContext, cmd_snapshot, git_cmd}; mod common; @@ -22,6 +24,20 @@ fn hook_env_count(context: &TestContext) -> Result { Ok(hooks_dir.read_dir()?.count()) } +fn remove_loose_blob(cwd: &assert_fs::fixture::ChildPath, filename: &str) -> Result<()> { + let output = git_cmd(cwd).arg("hash-object").arg(filename).output()?; + assert!(output.status.success(), "git hash-object should succeed"); + let blob = String::from_utf8(output.stdout)?; + let blob = blob.trim_ascii(); + let object_path = cwd + .child(".git") + .child("objects") + .child(&blob[..2]) + .child(&blob[2..]); + fs_err::remove_file(object_path.path())?; + Ok(()) +} + /// All hooks skip when no staged files match their file patterns. #[test] fn all_hooks_skipped_no_matching_files() -> Result<()> { @@ -660,6 +676,70 @@ fn all_files_with_existing_unstaged_changes_uses_snapshot_baseline() -> Result<( Ok(()) } +#[test] +fn all_files_clean_missing_blob_ignores_diff_snapshot_errors() -> Result<()> { + let context = TestContext::new(); + context.init_project(); + + let cwd = context.work_dir(); + context.write_pre_commit_config(indoc::indoc! {r#" + repos: + - repo: local + hooks: + - id: noop + name: noop + language: system + entry: python3 -c "pass" + pass_filenames: false + "#}); + + cwd.child("file.txt").write_str("original\n")?; + context.git_add("."); + context.git_commit("init"); + + remove_loose_blob(cwd, "file.txt")?; + + // Make the index stat data stale while keeping file content unchanged. A + // full `git diff` now exits non-zero because the blob is missing, but its + // stdout is still a usable best-effort before/after snapshot. + fs_err::OpenOptions::new() + .write(true) + .open(cwd.child("file.txt").path())? + .set_modified(SystemTime::now() + Duration::from_secs(10))?; + + let output = context + .run() + .arg("--all-files") + .env("RUST_LOG", "prek::git=trace") + .output()?; + + assert!( + output.status.success(), + "`--all-files` should not require blob objects when hooks leave a clean tree.\n\ + stdout:\n{}\n\ + stderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + let ignored_diff_errors = stderr + .matches("Continuing with git diff stdout despite non-zero exit status") + .count(); + assert_eq!( + ignored_diff_errors, 2, + "Expected before/after git diff errors to be logged and ignored, found {ignored_diff_errors}.\n\ + Trace output:\n{stderr}" + ); + assert!( + !stderr.contains("Command `git diff` exited with an error"), + "missing blobs should not turn hook modification detection into a fatal git diff error.\n\ + stderr:\n{stderr}" + ); + + Ok(()) +} + #[test] fn later_project_snapshots_diff_left_by_previous_project() -> Result<()> { let context = TestContext::new();