Skip to content

Commit ff0769a

Browse files
j178j178
committed
Handle impostor commits in auto-update
1 parent 200b6b8 commit ff0769a

2 files changed

Lines changed: 137 additions & 42 deletions

File tree

‎crates/prek/src/cli/auto_update.rs‎

Lines changed: 42 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ enum FrozenMismatch {
116116
ReplaceWith(String),
117117
/// Remove the stale comment because no ref points at the pinned commit.
118118
Remove,
119-
/// Warn only because the pinned commit itself could not be resolved.
119+
/// Warn only because the pinned commit is not present in the fetched repository view.
120120
NoReplacement,
121121
}
122122

@@ -260,7 +260,7 @@ pub(crate) async fn auto_update(
260260
.then_with(|| a.target.required_hook_ids.cmp(&b.target.required_hook_ids))
261261
});
262262

263-
warn_frozen_mismatches(&outcomes, dry_run, printer)?;
263+
warn_frozen_mismatches(&outcomes, printer)?;
264264

265265
// Group results by project config file
266266
#[expect(clippy::mutable_key_type)]
@@ -360,11 +360,7 @@ fn collect_repo_sources(workspace: &Workspace) -> Result<Vec<RepoSource<'_>>> {
360360
}
361361

362362
/// Emits all frozen-comment warnings before the normal update output.
363-
fn warn_frozen_mismatches(
364-
updates: &[RepoUpdate<'_>],
365-
dry_run: bool,
366-
printer: Printer,
367-
) -> Result<()> {
363+
fn warn_frozen_mismatches(updates: &[RepoUpdate<'_>], printer: Printer) -> Result<()> {
368364
for update in updates {
369365
let Ok(resolved) = &update.result else {
370366
continue;
@@ -377,8 +373,7 @@ fn warn_frozen_mismatches(
377373
render_frozen_mismatch_warning(
378374
update.target.repo,
379375
update.target.current_rev,
380-
mismatch,
381-
dry_run
376+
mismatch
382377
)
383378
)?;
384379
}
@@ -536,10 +531,8 @@ async fn collect_frozen_mismatches<'a>(
536531
return Ok(Vec::new());
537532
}
538533

539-
let current_rev_is_valid = resolve_revision_to_commit(repo_path, target.current_rev)
540-
.await
541-
.is_ok();
542-
let rev_tags = if current_rev_is_valid {
534+
let current_rev_is_present = is_commit_present(repo_path, target.current_rev).await?;
535+
let rev_tags = if current_rev_is_present {
543536
get_tags_pointing_at_revision(tag_timestamps, target.current_rev)
544537
} else {
545538
Vec::new()
@@ -566,7 +559,7 @@ async fn collect_frozen_mismatches<'a>(
566559
};
567560
let mismatch = select_best_tag(&rev_tags, current_frozen, true).map_or_else(
568561
|| {
569-
if current_rev_is_valid {
562+
if current_rev_is_present {
570563
FrozenMismatch::Remove
571564
} else {
572565
FrozenMismatch::NoReplacement
@@ -708,16 +701,6 @@ async fn evaluate_repo_target<'a>(
708701
/// Initializes a temporary git repo and fetches the remote HEAD plus tags.
709702
async fn setup_and_fetch_repo(repo_url: &str, repo_path: &Path) -> Result<()> {
710703
git::init_repo(repo_url, repo_path).await?;
711-
git::git_cmd("git config")?
712-
.arg("config")
713-
.arg("extensions.partialClone")
714-
.arg("true")
715-
.current_dir(repo_path)
716-
.remove_git_envs()
717-
.stdout(Stdio::null())
718-
.stderr(Stdio::null())
719-
.status()
720-
.await?;
721704
git::git_cmd("git fetch")?
722705
.arg("fetch")
723706
.arg("origin")
@@ -749,6 +732,38 @@ async fn resolve_revision_to_commit(repo_path: &Path, rev: &str) -> Result<Strin
749732
Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
750733
}
751734

735+
/// Returns whether a pinned commit SHA is already present in the refs fetched for `auto-update`.
736+
///
737+
/// `auto-update` fetches only `origin/HEAD` and tags, using `--filter=blob:none`. That filter
738+
/// still downloads commits and trees reachable from those refs, but omits blobs. We intentionally
739+
/// use `git --no-lazy-fetch cat-file -e` here instead of `rev-parse`: in a partial clone,
740+
/// `rev-parse` may lazily fetch a missing commit from the promisor remote on demand. On GitHub,
741+
/// that can make a fork-only "impostor commit" appear to belong to the parent repository.
742+
///
743+
/// `auto-update` only selects updates from tags, or from `HEAD` in `--bleeding-edge` mode. It
744+
/// does not normally update to arbitrary branches, so we currently fetch only those refs here.
745+
///
746+
/// So this helper answers a narrower question than "is this SHA valid anywhere on the remote?".
747+
/// It only checks whether the commit is already available from the refs we fetched for update
748+
/// selection. That means branch-only commits outside `HEAD` and tags are treated as absent for
749+
/// now. If that leads to false positives in practice, we can revisit this and fetch branches too.
750+
async fn is_commit_present(repo_path: &Path, commit: &str) -> Result<bool> {
751+
let status = git::git_cmd("git cat-file")?
752+
.arg("--no-lazy-fetch")
753+
.arg("cat-file")
754+
.arg("-e")
755+
.arg(format!("{commit}^{{commit}}"))
756+
.check(false)
757+
.current_dir(repo_path)
758+
.remove_git_envs()
759+
.stdout(Stdio::null())
760+
.stderr(Stdio::null())
761+
.status()
762+
.await?;
763+
764+
Ok(status.success())
765+
}
766+
752767
fn get_tags_pointing_at_revision<'a>(
753768
tag_timestamps: &'a [TagTimestamp],
754769
rev: &str,
@@ -765,7 +780,6 @@ fn render_frozen_mismatch_warning(
765780
repo: &str,
766781
current_rev: &str,
767782
mismatch: &FrozenCommentMismatch<'_>,
768-
dry_run: bool,
769783
) -> String {
770784
let label = match mismatch.reason {
771785
FrozenMismatchReason::ResolvesToDifferentCommit => {
@@ -780,19 +794,13 @@ fn render_frozen_mismatch_warning(
780794
};
781795
let details = match &mismatch.mismatch {
782796
FrozenMismatch::ReplaceWith(replacement) => {
783-
format!(
784-
"{} frozen comment to `{replacement}`",
785-
if dry_run { "would update" } else { "updating" }
786-
)
797+
format!("pinned commit `{current_rev}` is referenced by `{replacement}`")
787798
}
788799
FrozenMismatch::Remove => {
789-
format!(
790-
"{} frozen comment because no tag points at the pinned commit",
791-
if dry_run { "would remove" } else { "removing" }
792-
)
800+
format!("no tag points at the pinned commit `{current_rev}`")
793801
}
794802
FrozenMismatch::NoReplacement => {
795-
format!("pinned commit `{current_rev}` does not exist in the repo")
803+
format!("pinned commit `{current_rev}` is not present in the repo")
796804
}
797805
};
798806
let title = format!(

‎crates/prek/tests/auto_update.rs‎

Lines changed: 95 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -789,7 +789,7 @@ fn auto_update_with_existing_frozen_comment() -> Result<()> {
789789
3 | rev: [COMMIT_SHA] # frozen: v1.0.0
790790
| ^^^^^^ `v1.0.0` resolves to a different commit
791791
|
792-
= note: pinned commit `[COMMIT_SHA]` does not exist in the repo
792+
= note: pinned commit `[COMMIT_SHA]` is not present in the repo
793793
");
794794

795795
insta::with_settings!(
@@ -850,7 +850,7 @@ fn auto_update_updates_mismatched_frozen_comment() -> Result<()> {
850850
3 | rev: [COMMIT_SHA] # frozen: v1.0.0
851851
| ^^^^^^ `v1.0.0` resolves to a different commit
852852
|
853-
= note: updating frozen comment to `v1.1.0`
853+
= note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0`
854854
");
855855

856856
insta::with_settings!(
@@ -915,7 +915,7 @@ fn auto_update_updates_unresolvable_frozen_comment() -> Result<()> {
915915
3 | rev: [COMMIT_SHA] # frozen: does-not-exist
916916
| ^^^^^^^^^^^^^^ `does-not-exist` could not be resolved
917917
|
918-
= note: updating frozen comment to `v1.1.0`
918+
= note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0`
919919
");
920920

921921
insta::with_settings!(
@@ -980,7 +980,7 @@ fn auto_update_removes_frozen_comment_when_pinned_commit_has_no_tag() -> Result<
980980
3 | rev: [COMMIT_SHA] # frozen: v1.1.0
981981
| ^^^^^^ `v1.1.0` resolves to a different commit
982982
|
983-
= note: removing frozen comment because no tag points at the pinned commit
983+
= note: no tag points at the pinned commit `[COMMIT_SHA]`
984984
");
985985

986986
insta::with_settings!(
@@ -999,6 +999,93 @@ fn auto_update_removes_frozen_comment_when_pinned_commit_has_no_tag() -> Result<
999999
Ok(())
10001000
}
10011001

1002+
#[test]
1003+
fn auto_update_warns_for_branch_only_pinned_commit_with_frozen_comment() -> Result<()> {
1004+
let context = TestContext::new();
1005+
context.init_project();
1006+
1007+
let repo_path = create_local_git_repo(
1008+
&context,
1009+
"check-branch-only-pinned-frozen-repo",
1010+
&["v1.0.0", "v1.1.0"],
1011+
)?;
1012+
1013+
git_cmd(&repo_path)
1014+
.arg("checkout")
1015+
.arg("-b")
1016+
.arg("side")
1017+
.arg("v1.0.0^{}")
1018+
.assert()
1019+
.success();
1020+
git_cmd(&repo_path)
1021+
.arg("commit")
1022+
.arg("-m")
1023+
.arg("side")
1024+
.arg("--allow-empty")
1025+
.assert()
1026+
.success();
1027+
let branch_commit = git_cmd(&repo_path)
1028+
.args(["rev-parse", "HEAD"])
1029+
.output()?
1030+
.stdout;
1031+
let branch_commit = str::from_utf8(&branch_commit)?.trim().to_string();
1032+
git_cmd(&repo_path)
1033+
.arg("checkout")
1034+
.arg("master")
1035+
.assert()
1036+
.success();
1037+
1038+
context.write_pre_commit_config(&indoc::formatdoc! {r"
1039+
repos:
1040+
- repo: {}
1041+
rev: {} # frozen: v1.0.0
1042+
hooks:
1043+
- id: test-hook
1044+
", repo_path, branch_commit});
1045+
1046+
context.git_add(".");
1047+
1048+
let filters = context
1049+
.filters()
1050+
.into_iter()
1051+
.chain([
1052+
(branch_commit.as_str(), "[BRANCH_ONLY_COMMIT]"),
1053+
(r"[a-f0-9]{40}", r"[COMMIT_SHA]"),
1054+
])
1055+
.collect::<Vec<_>>();
1056+
1057+
cmd_snapshot!(filters.clone(), context.auto_update().arg("--freeze").arg("--dry-run"), @"
1058+
success: true
1059+
exit_code: 0
1060+
----- stdout -----
1061+
[[HOME]/test-repos/check-branch-only-pinned-frozen-repo] would update `v1.0.0@[BRANCH_ONLY_COMMIT]` -> `v1.1.0@[COMMIT_SHA]`
1062+
1063+
----- stderr -----
1064+
warning: [[HOME]/test-repos/check-branch-only-pinned-frozen-repo] frozen ref `v1.0.0` does not match `[BRANCH_ONLY_COMMIT]`
1065+
--> .pre-commit-config.yaml:3:62
1066+
|
1067+
3 | rev: [BRANCH_ONLY_COMMIT] # frozen: v1.0.0
1068+
| ^^^^^^ `v1.0.0` resolves to a different commit
1069+
|
1070+
= note: pinned commit `[BRANCH_ONLY_COMMIT]` is not present in the repo
1071+
");
1072+
1073+
insta::with_settings!(
1074+
{ filters => filters.clone() },
1075+
{
1076+
assert_snapshot!(context.read(PRE_COMMIT_CONFIG_YAML), @"
1077+
repos:
1078+
- repo: [HOME]/test-repos/check-branch-only-pinned-frozen-repo
1079+
rev: [BRANCH_ONLY_COMMIT] # frozen: v1.0.0
1080+
hooks:
1081+
- id: test-hook
1082+
");
1083+
}
1084+
);
1085+
1086+
Ok(())
1087+
}
1088+
10021089
#[test]
10031090
fn auto_update_warns_for_invalid_pinned_commit_with_frozen_comment() -> Result<()> {
10041091
let context = TestContext::new();
@@ -1044,7 +1131,7 @@ fn auto_update_warns_for_invalid_pinned_commit_with_frozen_comment() -> Result<(
10441131
3 | rev: [INVALID_COMMIT] # frozen: v1.0.0
10451132
| ^^^^^^ `v1.0.0` resolves to a different commit
10461133
|
1047-
= note: pinned commit `[INVALID_COMMIT]` does not exist in the repo
1134+
= note: pinned commit `[INVALID_COMMIT]` is not present in the repo
10481135
");
10491136

10501137
insta::with_settings!(
@@ -1105,7 +1192,7 @@ fn auto_update_dry_run_warns_for_mismatched_frozen_comment() -> Result<()> {
11051192
3 | rev: [COMMIT_SHA] # frozen: v1.0.0
11061193
| ^^^^^^ `v1.0.0` resolves to a different commit
11071194
|
1108-
= note: would update frozen comment to `v1.1.0`
1195+
= note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0`
11091196
");
11101197

11111198
insta::with_settings!(
@@ -1166,7 +1253,7 @@ fn auto_update_check_fails_for_mismatched_frozen_comment() -> Result<()> {
11661253
3 | rev: [COMMIT_SHA] # frozen: v1.0.0
11671254
| ^^^^^^ `v1.0.0` resolves to a different commit
11681255
|
1169-
= note: would update frozen comment to `v1.1.0`
1256+
= note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0`
11701257
");
11711258

11721259
insta::with_settings!(
@@ -1232,7 +1319,7 @@ fn auto_update_updates_mismatched_frozen_comment_toml() -> Result<()> {
12321319
3 | rev = "[COMMIT_SHA]" # frozen: v1.0.0
12331320
| ^^^^^^ `v1.0.0` resolves to a different commit
12341321
|
1235-
= note: updating frozen comment to `v1.1.0`
1322+
= note: pinned commit `[COMMIT_SHA]` is referenced by `v1.1.0`
12361323
"#);
12371324

12381325
insta::with_settings!(

0 commit comments

Comments
 (0)