From 99aae1935dceb2b254edf848e62c59ad4cc2e400 Mon Sep 17 00:00:00 2001 From: Shaan Majid <70789625+shaanmajid@users.noreply.github.com> Date: Fri, 26 Jun 2026 18:23:02 +0000 Subject: [PATCH 1/2] perf: skip submodule update when clones have none --- crates/prek/src/git.rs | 160 ++++++++++++++++++++++++++++++++++----- crates/prek/tests/run.rs | 95 +++++++++++++++++++++++ 2 files changed, 234 insertions(+), 21 deletions(-) diff --git a/crates/prek/src/git.rs b/crates/prek/src/git.rs index d70096c11..a4ed90f59 100644 --- a/crates/prek/src/git.rs +++ b/crates/prek/src/git.rs @@ -561,21 +561,7 @@ async fn shallow_clone( .output() .await?; - git_cmd("update git submodules")? - .current_dir(path) - .arg("-c") - .arg("protocol.version=2") - .arg("submodule") - .arg("update") - .arg("--init") - .arg("--recursive") - .arg("--depth=1") - .remove_git_envs() - .env(EnvVars::LC_ALL, "C") - .env(EnvVars::GIT_TERMINAL_PROMPT, terminal_prompt.env_value()) - .check(true) - .output() - .await?; + update_submodules(path, terminal_prompt, true).await?; Ok(()) } @@ -605,13 +591,33 @@ async fn full_clone(rev: &str, path: &Path, terminal_prompt: TerminalPrompt) -> .output() .await?; - git_cmd("update git submodules")? - .current_dir(path) - .arg("submodule") + update_submodules(path, terminal_prompt, false).await?; + + Ok(()) +} + +async fn update_submodules( + path: &Path, + terminal_prompt: TerminalPrompt, + shallow: bool, +) -> Result<(), Error> { + if !should_update_submodules(path).await? { + return Ok(()); + } + + let mut cmd = git_cmd("update git submodules")?; + cmd.current_dir(path); + if shallow { + cmd.arg("-c").arg("protocol.version=2"); + } + cmd.arg("submodule") .arg("update") .arg("--init") - .arg("--recursive") - .remove_git_envs() + .arg("--recursive"); + if shallow { + cmd.arg("--depth=1"); + } + cmd.remove_git_envs() .env(EnvVars::LC_ALL, "C") .env(EnvVars::GIT_TERMINAL_PROMPT, terminal_prompt.env_value()) .check(true) @@ -621,6 +627,28 @@ async fn full_clone(rev: &str, path: &Path, terminal_prompt: TerminalPrompt) -> Ok(()) } +async fn should_update_submodules(path: &Path) -> Result { + if path.join(".gitmodules").exists() { + return Ok(true); + } + + let output = git_cmd("list gitlinks")? + .current_dir(path) + .arg("ls-files") + .arg("-z") + .arg("-s") + .remove_git_envs() + .env(EnvVars::LC_ALL, "C") + .check(true) + .output() + .await?; + + Ok(output + .stdout + .split(|&byte| byte == b'\0') + .any(|entry| entry.starts_with(b"160000 "))) +} + async fn clone_repo_attempt( rev: &str, path: &Path, @@ -934,9 +962,99 @@ pub(crate) fn list_submodules(git_root: &Path) -> Result, Error> { #[cfg(test)] mod tests { - use super::shared_repository_file_mode; #[cfg(unix)] use super::zsplit; + use super::{ + Error, GIT, TerminalPrompt, full_clone, init_repo, shared_repository_file_mode, + should_update_submodules, update_submodules, + }; + use assert_cmd::assert::OutputAssertExt; + use std::path::Path; + use std::process::Command; + + fn run_git(path: &Path, args: &[&str]) { + let mut command = Command::new(GIT.as_ref().unwrap()); + command.current_dir(path).args(args); + + command.assert().success(); + } + + #[tokio::test] + async fn should_update_submodules_when_gitmodules_exists() { + let tmp = tempfile::tempdir().unwrap(); + run_git(tmp.path(), &["init"]); + fs_err::write(tmp.path().join(".gitmodules"), "").unwrap(); + + assert!(should_update_submodules(tmp.path()).await.unwrap()); + } + + #[tokio::test] + async fn full_clone_skips_submodule_update_when_repo_has_no_submodules() { + let remote = tempfile::tempdir().unwrap(); + run_git(remote.path(), &["init"]); + fs_err::write(remote.path().join("file.txt"), "content\n").unwrap(); + run_git(remote.path(), &["add", "."]); + run_git( + remote.path(), + &[ + "-c", + "user.name=prek", + "-c", + "user.email=prek@example.com", + "commit", + "-m", + "initial commit", + ], + ); + let output = Command::new(GIT.as_ref().unwrap()) + .current_dir(remote.path()) + .arg("rev-parse") + .arg("HEAD") + .output() + .unwrap(); + assert!(output.status.success()); + let rev = String::from_utf8_lossy(&output.stdout); + + let clone = tempfile::tempdir().unwrap(); + init_repo(remote.path().to_str().unwrap(), clone.path()) + .await + .unwrap(); + + full_clone(rev.trim(), clone.path(), TerminalPrompt::Disabled) + .await + .unwrap(); + } + + #[tokio::test] + async fn update_submodules_runs_when_gitlinks_exist_without_gitmodules() { + let tmp = tempfile::tempdir().unwrap(); + run_git(tmp.path(), &["init"]); + run_git( + tmp.path(), + &[ + "update-index", + "--add", + "--cacheinfo", + "160000,1111111111111111111111111111111111111111,sub", + ], + ); + + assert!(should_update_submodules(tmp.path()).await.unwrap()); + + let err = update_submodules(tmp.path(), TerminalPrompt::Disabled, true) + .await + .unwrap_err(); + + assert!(matches!(err, Error::Command(_))); + assert!(err.to_string().contains("update git submodules")); + + let err = update_submodules(tmp.path(), TerminalPrompt::Disabled, false) + .await + .unwrap_err(); + + assert!(matches!(err, Error::Command(_))); + assert!(err.to_string().contains("update git submodules")); + } #[cfg(unix)] #[test] diff --git a/crates/prek/tests/run.rs b/crates/prek/tests/run.rs index 7db65033a..f015f41b9 100644 --- a/crates/prek/tests/run.rs +++ b/crates/prek/tests/run.rs @@ -2151,6 +2151,101 @@ fn skipped_remote_repo_is_not_cloned() { ); } +#[test] +#[cfg(unix)] +fn remote_repo_without_submodules_skips_submodule_update() -> Result<()> { + use std::os::unix::fs::PermissionsExt; + + let context = TestContext::new(); + context.init_project(); + + let hook_repo = context.home_dir().child("no-submodules-hook-repo"); + hook_repo.create_dir_all()?; + + git_cmd(&hook_repo) + .arg("-c") + .arg("init.defaultBranch=master") + .arg("init") + .assert() + .success(); + + hook_repo + .child(PRE_COMMIT_HOOKS_YAML) + .write_str(indoc::indoc! {r#" + - id: noop + name: noop + entry: "true" + language: system + pass_filenames: false + "#})?; + + git_cmd(&hook_repo).arg("add").arg(".").assert().success(); + git_cmd(&hook_repo) + .arg("commit") + .arg("-m") + .arg("Initial commit") + .assert() + .success(); + let rev = git_cmd(&hook_repo) + .arg("rev-parse") + .arg("HEAD") + .output()? + .stdout; + let rev = String::from_utf8_lossy(&rev).trim().to_string(); + + context.write_pre_commit_config(&indoc::formatdoc! {r" + repos: + - repo: {repo} + rev: {rev} + hooks: + - id: noop + ", repo = hook_repo.display()}); + context + .work_dir() + .child("tracked.txt") + .write_str("content\n")?; + context.git_add("."); + + let fake_bin = context.home_dir().child("fake-git-bin"); + fake_bin.create_dir_all()?; + let fake_git = fake_bin.child("git"); + fake_git.write_str(indoc::indoc! {r#" + #!/bin/sh + case " $* " in + *" submodule update "*) + printf '%s\n' "$*" >> "$PREK_TEST_GIT_LOG" + ;; + esac + exec "$PREK_TEST_REAL_GIT" "$@" + "#})?; + fs_err::set_permissions(fake_git.path(), std::fs::Permissions::from_mode(0o755))?; + + let original_path = EnvVars::var_os(EnvVars::PATH).expect("PATH must be set"); + let new_path = std::env::join_paths( + std::iter::once(fake_bin.path().to_path_buf()).chain(std::env::split_paths(&original_path)), + )?; + let log_path = context.home_dir().child("git-wrapper.log"); + let real_git = which::which("git")?; + + context + .run() + .arg("--all-files") + .arg("noop") + .env(EnvVars::PATH, new_path) + .env("PREK_TEST_GIT_LOG", log_path.path()) + .env("PREK_TEST_REAL_GIT", real_git) + .assert() + .success(); + + let submodule_updates = fs_err::read_to_string(log_path.path()).unwrap_or_default(); + assert_eq!( + submodule_updates, "", + "repo without .gitmodules or gitlinks should not run git submodule update", + ); + + Ok(()) +} + #[test] fn skipped_same_key_remote_repo_entry_is_not_initialized() -> Result<()> { let context = TestContext::new(); From db7cf71fdd08b5e7214fceee1c5d1a9224bf9d67 Mon Sep 17 00:00:00 2001 From: Jo <10510431+j178@users.noreply.github.com> Date: Sat, 27 Jun 2026 21:21:11 +0800 Subject: [PATCH 2/2] Tweak tests --- crates/prek/src/git.rs | 8 ++-- crates/prek/tests/run.rs | 95 ---------------------------------------- 2 files changed, 3 insertions(+), 100 deletions(-) diff --git a/crates/prek/src/git.rs b/crates/prek/src/git.rs index 875457bd4..49d1b5bb2 100644 --- a/crates/prek/src/git.rs +++ b/crates/prek/src/git.rs @@ -590,10 +590,8 @@ async fn update_submodules( } let mut cmd = git_cmd()?; - cmd.current_dir(path); - if shallow { - cmd.hidden_args(["-c", "protocol.version=2"]); - } + cmd.current_dir(path) + .hidden_args(["-c", "protocol.version=2"]); cmd.arg("submodule") .arg("update") .arg("--init") @@ -612,7 +610,7 @@ async fn update_submodules( } async fn should_update_submodules(path: &Path) -> Result { - if path.join(".gitmodules").exists() { + if path.join(".gitmodules").try_exists()? { return Ok(true); } diff --git a/crates/prek/tests/run.rs b/crates/prek/tests/run.rs index 00836affd..1f93c19b2 100644 --- a/crates/prek/tests/run.rs +++ b/crates/prek/tests/run.rs @@ -2159,101 +2159,6 @@ fn skipped_remote_repo_is_not_cloned() { ); } -#[test] -#[cfg(unix)] -fn remote_repo_without_submodules_skips_submodule_update() -> Result<()> { - use std::os::unix::fs::PermissionsExt; - - let context = TestContext::new(); - context.init_project(); - - let hook_repo = context.home_dir().child("no-submodules-hook-repo"); - hook_repo.create_dir_all()?; - - git_cmd(&hook_repo) - .arg("-c") - .arg("init.defaultBranch=master") - .arg("init") - .assert() - .success(); - - hook_repo - .child(PRE_COMMIT_HOOKS_YAML) - .write_str(indoc::indoc! {r#" - - id: noop - name: noop - entry: "true" - language: system - pass_filenames: false - "#})?; - - git_cmd(&hook_repo).arg("add").arg(".").assert().success(); - git_cmd(&hook_repo) - .arg("commit") - .arg("-m") - .arg("Initial commit") - .assert() - .success(); - let rev = git_cmd(&hook_repo) - .arg("rev-parse") - .arg("HEAD") - .output()? - .stdout; - let rev = String::from_utf8_lossy(&rev).trim().to_string(); - - context.write_pre_commit_config(&indoc::formatdoc! {r" - repos: - - repo: {repo} - rev: {rev} - hooks: - - id: noop - ", repo = hook_repo.display()}); - context - .work_dir() - .child("tracked.txt") - .write_str("content\n")?; - context.git_add("."); - - let fake_bin = context.home_dir().child("fake-git-bin"); - fake_bin.create_dir_all()?; - let fake_git = fake_bin.child("git"); - fake_git.write_str(indoc::indoc! {r#" - #!/bin/sh - case " $* " in - *" submodule update "*) - printf '%s\n' "$*" >> "$PREK_TEST_GIT_LOG" - ;; - esac - exec "$PREK_TEST_REAL_GIT" "$@" - "#})?; - fs_err::set_permissions(fake_git.path(), std::fs::Permissions::from_mode(0o755))?; - - let original_path = EnvVars::var_os(EnvVars::PATH).expect("PATH must be set"); - let new_path = std::env::join_paths( - std::iter::once(fake_bin.path().to_path_buf()).chain(std::env::split_paths(&original_path)), - )?; - let log_path = context.home_dir().child("git-wrapper.log"); - let real_git = which::which("git")?; - - context - .run() - .arg("--all-files") - .arg("noop") - .env(EnvVars::PATH, new_path) - .env("PREK_TEST_GIT_LOG", log_path.path()) - .env("PREK_TEST_REAL_GIT", real_git) - .assert() - .success(); - - let submodule_updates = fs_err::read_to_string(log_path.path()).unwrap_or_default(); - assert_eq!( - submodule_updates, "", - "repo without .gitmodules or gitlinks should not run git submodule update", - ); - - Ok(()) -} - #[test] fn skipped_same_key_remote_repo_entry_is_not_initialized() -> Result<()> { let context = TestContext::new();