diff --git a/crates/prek/src/cli/auto_update.rs b/crates/prek/src/cli/auto_update.rs index 13f1f1c5b..72efa875d 100644 --- a/crates/prek/src/cli/auto_update.rs +++ b/crates/prek/src/cli/auto_update.rs @@ -2,6 +2,7 @@ use std::fmt::{self, Write}; use std::ops::Range; use std::path::{Path, PathBuf}; use std::process::Stdio; +use std::sync::OnceLock; use std::time::{SystemTime, UNIX_EPOCH}; use annotate_snippets::{AnnotationKind, Level, Renderer, Snippet, renderer::DecorStyle}; @@ -11,6 +12,7 @@ use itertools::Itertools; use lazy_regex::regex; use owo_colors::OwoColorize; use prek_consts::PRE_COMMIT_HOOKS_YAML; +use prek_consts::env_vars::EnvVars; use rustc_hash::FxHashMap; use rustc_hash::FxHashSet; use semver::Version; @@ -116,10 +118,21 @@ enum FrozenMismatch { ReplaceWith(String), /// Remove the stale comment because no ref points at the pinned commit. Remove, - /// Warn only because the pinned commit itself could not be resolved. + /// Warn only because we cannot safely decide a comment-only fix. NoReplacement, } +/// Whether the pinned SHA is available from the refs fetched for `auto-update`. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum CommitPresence { + /// The commit is present in the fetched repository view. + Present, + /// The commit is not present in the fetched repository view. + Absent, + /// The current Git cannot disable lazy fetch, so presence could not be checked safely. + Unknown, +} + /// Why an existing `# frozen:` comment no longer matches the configured `rev`. enum FrozenMismatchReason { /// The frozen reference resolves successfully, but to a different commit than `rev`. @@ -142,6 +155,8 @@ struct FrozenCommentMismatch<'a> { frozen_site: Option, /// Why the existing frozen reference is stale. reason: FrozenMismatchReason, + /// Whether the pinned SHA is available in the fetched repository view. + current_rev_presence: CommitPresence, /// The action to take for this stale comment. mismatch: FrozenMismatch, } @@ -260,7 +275,7 @@ pub(crate) async fn auto_update( .then_with(|| a.target.required_hook_ids.cmp(&b.target.required_hook_ids)) }); - warn_frozen_mismatches(&outcomes, dry_run, printer)?; + warn_frozen_mismatches(&outcomes, printer)?; // Group results by project config file #[expect(clippy::mutable_key_type)] @@ -360,11 +375,7 @@ fn collect_repo_sources(workspace: &Workspace) -> Result>> { } /// Emits all frozen-comment warnings before the normal update output. -fn warn_frozen_mismatches( - updates: &[RepoUpdate<'_>], - dry_run: bool, - printer: Printer, -) -> Result<()> { +fn warn_frozen_mismatches(updates: &[RepoUpdate<'_>], printer: Printer) -> Result<()> { for update in updates { let Ok(resolved) = &update.result else { continue; @@ -377,8 +388,7 @@ fn warn_frozen_mismatches( render_frozen_mismatch_warning( update.target.repo, update.target.current_rev, - mismatch, - dry_run + mismatch ) )?; } @@ -536,14 +546,8 @@ async fn collect_frozen_mismatches<'a>( return Ok(Vec::new()); } - let current_rev_is_valid = resolve_revision_to_commit(repo_path, target.current_rev) - .await - .is_ok(); - let rev_tags = if current_rev_is_valid { - get_tags_pointing_at_revision(tag_timestamps, target.current_rev) - } else { - Vec::new() - }; + let current_rev_presence = is_commit_present(repo_path, target.current_rev).await?; + let rev_tags = get_tags_pointing_at_revision(tag_timestamps, target.current_rev); let mut resolved_frozen_refs = FxHashMap::default(); for frozen_ref in frozen_refs_to_check { let resolved = resolve_revision_to_commit(repo_path, frozen_ref).await.ok(); @@ -565,10 +569,13 @@ async fn collect_frozen_mismatches<'a>( None => FrozenMismatchReason::Unresolvable, }; let mismatch = select_best_tag(&rev_tags, current_frozen, true).map_or_else( - || { - if current_rev_is_valid { + || match current_rev_presence { + CommitPresence::Present => { + // We only remove the stale comment when we can prove the pinned commit is + // present in the fetched repo view and no fetched tag points at it. FrozenMismatch::Remove - } else { + } + CommitPresence::Absent | CommitPresence::Unknown => { FrozenMismatch::NoReplacement } }, @@ -582,6 +589,7 @@ async fn collect_frozen_mismatches<'a>( current_frozen: current_frozen.to_string(), frozen_site: usage.current_frozen_site.clone(), reason, + current_rev_presence, mismatch, }) }) @@ -708,16 +716,6 @@ async fn evaluate_repo_target<'a>( /// Initializes a temporary git repo and fetches the remote HEAD plus tags. async fn setup_and_fetch_repo(repo_url: &str, repo_path: &Path) -> Result<()> { git::init_repo(repo_url, repo_path).await?; - git::git_cmd("git config")? - .arg("config") - .arg("extensions.partialClone") - .arg("true") - .current_dir(repo_path) - .remove_git_envs() - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .await?; git::git_cmd("git fetch")? .arg("fetch") .arg("origin") @@ -749,6 +747,63 @@ async fn resolve_revision_to_commit(repo_path: &Path, rev: &str) -> Result Result { + static GIT_SUPPORTS_NO_LAZY_FETCH: OnceLock = OnceLock::new(); + + if matches!(GIT_SUPPORTS_NO_LAZY_FETCH.get(), Some(false)) { + return Ok(CommitPresence::Unknown); + } + + let output = git::git_cmd("git cat-file")? + .arg("--no-lazy-fetch") + .arg("cat-file") + .arg("-e") + .arg(format!("{commit}^{{commit}}")) + .env(EnvVars::LC_ALL, "C") + .check(false) + .current_dir(repo_path) + .remove_git_envs() + .stdout(Stdio::null()) + .output() + .await?; + + if output.status.success() { + let _ = GIT_SUPPORTS_NO_LAZY_FETCH.set(true); + return Ok(CommitPresence::Present); + } + + if no_lazy_fetch_unsupported(&output.stderr) { + let _ = GIT_SUPPORTS_NO_LAZY_FETCH.set(false); + return Ok(CommitPresence::Unknown); + } + + let _ = GIT_SUPPORTS_NO_LAZY_FETCH.set(true); + Ok(CommitPresence::Absent) +} + +fn no_lazy_fetch_unsupported(stderr: &[u8]) -> bool { + let stderr = String::from_utf8_lossy(stderr); + stderr.contains("--no-lazy-fetch") && stderr.contains("unknown option") +} + fn get_tags_pointing_at_revision<'a>( tag_timestamps: &'a [TagTimestamp], rev: &str, @@ -765,7 +820,6 @@ fn render_frozen_mismatch_warning( repo: &str, current_rev: &str, mismatch: &FrozenCommentMismatch<'_>, - dry_run: bool, ) -> String { let label = match mismatch.reason { FrozenMismatchReason::ResolvesToDifferentCommit => { @@ -779,21 +833,20 @@ fn render_frozen_mismatch_warning( } }; let details = match &mismatch.mismatch { - FrozenMismatch::ReplaceWith(replacement) => { - format!( - "{} frozen comment to `{replacement}`", - if dry_run { "would update" } else { "updating" } - ) - } - FrozenMismatch::Remove => { - format!( - "{} frozen comment because no tag points at the pinned commit", - if dry_run { "would remove" } else { "removing" } - ) - } - FrozenMismatch::NoReplacement => { - format!("pinned commit `{current_rev}` does not exist in the repo") + FrozenMismatch::ReplaceWith(replacement) => Some(format!( + "pinned commit `{current_rev}` is referenced by `{replacement}`" + )), + FrozenMismatch::Remove => Some(format!( + "no tag points at the pinned commit `{current_rev}`" + )), + FrozenMismatch::NoReplacement + if matches!(mismatch.current_rev_presence, CommitPresence::Absent) => + { + Some(format!( + "pinned commit `{current_rev}` is not present in the repo" + )) } + FrozenMismatch::NoReplacement => None, }; let title = format!( "[{repo}] frozen ref `{}` does not match `{current_rev}`", @@ -804,15 +857,15 @@ fn render_frozen_mismatch_warning( .frozen_site .as_ref() .expect("frozen comment site must exist when rendering a frozen mismatch warning"); - let report = Level::WARNING - .primary_title(title) - .element( - Snippet::source(&site.source_line) - .line_start(site.line_number) - .path(mismatch.project.config_file().user_display().to_string()) - .annotation(AnnotationKind::Primary.span(site.span.clone()).label(label)), - ) - .element(Level::NOTE.message(details)); + let mut report = Level::WARNING.primary_title(title).element( + Snippet::source(&site.source_line) + .line_start(site.line_number) + .path(mismatch.project.config_file().user_display().to_string()) + .annotation(AnnotationKind::Primary.span(site.span.clone()).label(label)), + ); + if let Some(details) = details { + report = report.element(Level::NOTE.message(details)); + } let renderer = Renderer::styled().decor_style(DecorStyle::Ascii); format!("{}\n", renderer.render(&[report])) @@ -1610,4 +1663,14 @@ mod tests { let tags: Vec<&str> = timestamps.iter().map(|tag| tag.tag.as_str()).collect(); assert_eq!(tags, vec!["alpha", "beta", "gamma"]); } + + #[test] + fn test_no_lazy_fetch_unsupported() { + assert!(no_lazy_fetch_unsupported( + b"unknown option: --no-lazy-fetch\n" + )); + assert!(!no_lazy_fetch_unsupported( + b"fatal: Not a valid object name 1234567890abcdef1234567890abcdef12345678^{commit}\n" + )); + } } diff --git a/crates/prek/tests/auto_update.rs b/crates/prek/tests/auto_update.rs index 7faaffd44..3dccc5a5d 100644 --- a/crates/prek/tests/auto_update.rs +++ b/crates/prek/tests/auto_update.rs @@ -789,7 +789,7 @@ fn auto_update_with_existing_frozen_comment() -> Result<()> { 3 | rev: [COMMIT_SHA] # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: pinned commit `[COMMIT_SHA]` does not exist in the repo + = note: pinned commit `[COMMIT_SHA]` is not present in the repo "); insta::with_settings!( @@ -850,7 +850,7 @@ fn auto_update_updates_mismatched_frozen_comment() -> Result<()> { 3 | rev: [COMMIT_SHA] # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: updating frozen comment to `v1.1.0` + = note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0` "); insta::with_settings!( @@ -915,7 +915,7 @@ fn auto_update_updates_unresolvable_frozen_comment() -> Result<()> { 3 | rev: [COMMIT_SHA] # frozen: does-not-exist | ^^^^^^^^^^^^^^ `does-not-exist` could not be resolved | - = note: updating frozen comment to `v1.1.0` + = note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0` "); insta::with_settings!( @@ -980,7 +980,7 @@ fn auto_update_removes_frozen_comment_when_pinned_commit_has_no_tag() -> Result< 3 | rev: [COMMIT_SHA] # frozen: v1.1.0 | ^^^^^^ `v1.1.0` resolves to a different commit | - = note: removing frozen comment because no tag points at the pinned commit + = note: no tag points at the pinned commit `[COMMIT_SHA]` "); insta::with_settings!( @@ -999,6 +999,93 @@ fn auto_update_removes_frozen_comment_when_pinned_commit_has_no_tag() -> Result< Ok(()) } +#[test] +fn auto_update_warns_for_branch_only_pinned_commit_with_frozen_comment() -> Result<()> { + let context = TestContext::new(); + context.init_project(); + + let repo_path = create_local_git_repo( + &context, + "check-branch-only-pinned-frozen-repo", + &["v1.0.0", "v1.1.0"], + )?; + + git_cmd(&repo_path) + .arg("checkout") + .arg("-b") + .arg("side") + .arg("v1.0.0^{}") + .assert() + .success(); + git_cmd(&repo_path) + .arg("commit") + .arg("-m") + .arg("side") + .arg("--allow-empty") + .assert() + .success(); + let branch_commit = git_cmd(&repo_path) + .args(["rev-parse", "HEAD"]) + .output()? + .stdout; + let branch_commit = str::from_utf8(&branch_commit)?.trim().to_string(); + git_cmd(&repo_path) + .arg("checkout") + .arg("master") + .assert() + .success(); + + context.write_pre_commit_config(&indoc::formatdoc! {r" + repos: + - repo: {} + rev: {} # frozen: v1.0.0 + hooks: + - id: test-hook + ", repo_path, branch_commit}); + + context.git_add("."); + + let filters = context + .filters() + .into_iter() + .chain([ + (branch_commit.as_str(), "[BRANCH_ONLY_COMMIT]"), + (r"[a-f0-9]{40}", r"[COMMIT_SHA]"), + ]) + .collect::>(); + + cmd_snapshot!(filters.clone(), context.auto_update().arg("--freeze").arg("--dry-run"), @" + success: true + exit_code: 0 + ----- stdout ----- + [[HOME]/test-repos/check-branch-only-pinned-frozen-repo] would update `v1.0.0@[BRANCH_ONLY_COMMIT]` -> `v1.1.0@[COMMIT_SHA]` + + ----- stderr ----- + warning: [[HOME]/test-repos/check-branch-only-pinned-frozen-repo] frozen ref `v1.0.0` does not match `[BRANCH_ONLY_COMMIT]` + --> .pre-commit-config.yaml:3:62 + | + 3 | rev: [BRANCH_ONLY_COMMIT] # frozen: v1.0.0 + | ^^^^^^ `v1.0.0` resolves to a different commit + | + = note: pinned commit `[BRANCH_ONLY_COMMIT]` is not present in the repo + "); + + insta::with_settings!( + { filters => filters.clone() }, + { + assert_snapshot!(context.read(PRE_COMMIT_CONFIG_YAML), @" + repos: + - repo: [HOME]/test-repos/check-branch-only-pinned-frozen-repo + rev: [BRANCH_ONLY_COMMIT] # frozen: v1.0.0 + hooks: + - id: test-hook + "); + } + ); + + Ok(()) +} + #[test] fn auto_update_warns_for_invalid_pinned_commit_with_frozen_comment() -> Result<()> { let context = TestContext::new(); @@ -1044,7 +1131,7 @@ fn auto_update_warns_for_invalid_pinned_commit_with_frozen_comment() -> Result<( 3 | rev: [INVALID_COMMIT] # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: pinned commit `[INVALID_COMMIT]` does not exist in the repo + = note: pinned commit `[INVALID_COMMIT]` is not present in the repo "); insta::with_settings!( @@ -1105,7 +1192,7 @@ fn auto_update_dry_run_warns_for_mismatched_frozen_comment() -> Result<()> { 3 | rev: [COMMIT_SHA] # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: would update frozen comment to `v1.1.0` + = note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0` "); insta::with_settings!( @@ -1166,7 +1253,7 @@ fn auto_update_check_fails_for_mismatched_frozen_comment() -> Result<()> { 3 | rev: [COMMIT_SHA] # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: would update frozen comment to `v1.1.0` + = note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0` "); insta::with_settings!( @@ -1232,7 +1319,7 @@ fn auto_update_updates_mismatched_frozen_comment_toml() -> Result<()> { 3 | rev = "[COMMIT_SHA]" # frozen: v1.0.0 | ^^^^^^ `v1.0.0` resolves to a different commit | - = note: updating frozen comment to `v1.1.0` + = note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0` "#); insta::with_settings!(