diff --git a/crates/prek/src/cli/install.rs b/crates/prek/src/cli/install.rs index 25eddd2b9..f10f52284 100644 --- a/crates/prek/src/cli/install.rs +++ b/crates/prek/src/cli/install.rs @@ -13,9 +13,9 @@ use same_file::is_same_file; use crate::cli::reporter::{HookInitReporter, HookInstallReporter}; use crate::cli::run; use crate::cli::run::InstallCache; -use crate::cli::run::{SelectorSource, Selectors}; +use crate::cli::run::{ConfiguredHook, SelectorSource, Selectors}; use crate::cli::{ExitStatus, HookType}; -use crate::config::load_config; +use crate::config::{Repo, Stage, Stages, load_config}; use crate::fs::{CWD, Simplified}; use crate::git::{GIT_ROOT, git_cmd}; use crate::printer::Printer; @@ -105,6 +105,17 @@ pub(crate) async fn install( None }; + if project.is_some() { + tip_unmatched_hook_stages( + store, + config.as_deref(), + selectors.as_ref(), + refresh, + &hook_types, + printer, + )?; + } + for hook_type in hook_types { install_hook_script( project.as_ref(), @@ -202,6 +213,122 @@ fn get_hook_types( hook_types } +/// Return whether the selectors persisted into the installed hook scripts will run this hook. +/// +/// Mirrors `install_hook_script`: every include is persisted, but only CLI-flag skips are +/// (env-var skips are ignored at install time). An include that only matches a remote hook +/// through its manifest alias can't be recognized from the config alone, so such a hook is +/// treated as not selected. +fn shim_selects_hook(selectors: Option<&Selectors>, hook: &ConfiguredHook<'_>) -> bool { + let Some(selectors) = selectors else { + return true; + }; + + if selectors + .skips() + .iter() + .filter(|skip| matches!(skip.source(), SelectorSource::CliFlag(_))) + .any(|skip| skip.matches_configured_hook(hook)) + { + return false; + } + + let includes = selectors.includes(); + includes.is_empty() + || includes + .iter() + .any(|include| include.matches_configured_hook(hook)) +} + +/// Print a tip about configured hooks whose stages aren't covered by the hook types being +/// installed, since their shims won't be installed and they won't run automatically. +/// +/// This is intentionally not a warning: skipping a stage (e.g. to only run a hook manually by +/// id, or from CI) is a valid setup, not something that needs fixing. +fn tip_unmatched_hook_stages( + store: &Store, + config: Option<&Path>, + selectors: Option<&Selectors>, + refresh: bool, + hook_types: &[HookType], + printer: Printer, +) -> Result<()> { + let installed = Stages::from( + hook_types + .iter() + .map(|&hook_type| Stage::from(hook_type)) + .collect::>(), + ); + + // A plain root install runs every workspace project, so scan them all, the same way `run` does. + let Ok(root) = Workspace::find_root(config, &CWD) else { + return Ok(()); + }; + let Ok(workspace) = + Workspace::discover(store, root, config.map(Path::to_path_buf), None, refresh) + else { + return Ok(()); + }; + + let mut unmatched: Vec<(&str, Stages)> = Vec::new(); + for project in workspace.projects() { + let config = project.config(); + for repo in &config.repos { + let is_remote = matches!(repo, Repo::Remote(_)); + for hook in repo.iter_hooks() { + let configured = ConfiguredHook::new( + project.relative_path(), + hook.id, + hook.options.alias.as_deref(), + hook.groups, + ); + if !shim_selects_hook(selectors, &configured) { + continue; + } + + // A remote hook that omits `stages` may inherit them from its manifest, which + // install never fetches, so skip it rather than risk a false positive. An explicit + // value (even an empty list) overrides the manifest and resolves through + // `default_stages`, the same way the hook builder does. + if is_remote && hook.options.stages.is_none() { + continue; + } + let stages = Stages::resolve(hook.options.stages, config.default_stages); + + // `Stage::Manual` has no git shim, so it is ignored here. + let required = Stages::from( + stages + .iter() + .filter(|&stage| stage != Stage::Manual) + .collect::>(), + ); + if !required.is_empty() && !required.intersects(installed) { + unmatched.push((hook.id, required)); + } + } + } + } + + if unmatched.is_empty() { + return Ok(()); + } + + let list = unmatched + .iter() + .map(|(id, stages)| format!(" - `{}` ({})", id.cyan(), stages.yellow())) + .collect::>() + .join("\n"); + writeln!( + printer.stdout(), + "{} the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically:\n{list}\nIf that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `{}`, or set `{}` in your config.", + "tip:".cyan().bold(), + "prek install --hook-type ".cyan(), + "default_install_hook_types".cyan(), + )?; + + Ok(()) +} + #[allow(clippy::fn_params_excessive_bools)] fn install_hook_script( project: Option<&Project>, diff --git a/crates/prek/src/cli/run/selector.rs b/crates/prek/src/cli/run/selector.rs index 6831f3ea0..855097968 100644 --- a/crates/prek/src/cli/run/selector.rs +++ b/crates/prek/src/cli/run/selector.rs @@ -164,7 +164,7 @@ impl Selector { } } - fn matches_configured_hook(&self, hook: &ConfiguredHook<'_>) -> bool { + pub(crate) fn matches_configured_hook(&self, hook: &ConfiguredHook<'_>) -> bool { let matches_hook_id = |selector: &str| { hook.id == selector || hook.alias.is_some_and(|alias| alias == selector) }; diff --git a/crates/prek/src/config.rs b/crates/prek/src/config.rs index 3faebb491..33f0193d1 100644 --- a/crates/prek/src/config.rs +++ b/crates/prek/src/config.rs @@ -424,6 +424,19 @@ impl Stages { pub(crate) fn contains(self, stage: Stage) -> bool { (self.0 & stage.bit()) != 0 } + + pub(crate) fn intersects(self, other: Self) -> bool { + (self.0 & other.0) != 0 + } + + /// A hook with no `stages` (or an empty list) inherits `default_stages`, + /// which itself falls back to [`Stages::ALL`] when unset. + pub(crate) fn resolve(hook: Option, default: Option) -> Self { + match hook { + Some(stages) if !stages.is_empty() => stages, + _ => default.unwrap_or(Self::ALL), + } + } } impl Display for Stages { @@ -1122,6 +1135,34 @@ impl<'de> Deserialize<'de> for Repo { } } +/// The parts of a config hook entry shared by every repo type. +pub(crate) struct ConfigHook<'a> { + pub id: &'a str, + pub groups: Option<&'a [String]>, + pub options: &'a HookOptions, +} + +impl Repo { + pub(crate) fn iter_hooks(&self) -> Box> + '_> { + macro_rules! iter { + ($repo:expr) => { + Box::new($repo.hooks.iter().map(|h| ConfigHook { + id: h.id.as_str(), + groups: h.groups.as_deref(), + options: &h.options, + })) + }; + } + + match self { + Repo::Remote(repo) => iter!(repo), + Repo::Local(repo) => iter!(repo), + Repo::Meta(repo) => iter!(repo), + Repo::Builtin(repo) => iter!(repo), + } + } +} + fn remote_hook_to_local(hook: RemoteHook) -> Result where E: DeError, diff --git a/crates/prek/src/hook.rs b/crates/prek/src/hook.rs index 6c569ec01..eb0337834 100644 --- a/crates/prek/src/hook.rs +++ b/crates/prek/src/hook.rs @@ -89,14 +89,7 @@ impl HookSpec { .and_then(|v| v.get(&language).cloned()); } - if self - .options - .stages - .as_ref() - .is_none_or(|stages| stages.is_empty()) - { - self.options.stages = Some(config.default_stages.unwrap_or(Stages::ALL)); - } + self.options.stages = Some(Stages::resolve(self.options.stages, config.default_stages)); if let Some(default_env) = &config.default_env { let mut env = default_env.clone(); diff --git a/crates/prek/tests/install.rs b/crates/prek/tests/install.rs index 52637b18f..72405f157 100644 --- a/crates/prek/tests/install.rs +++ b/crates/prek/tests/install.rs @@ -1810,3 +1810,332 @@ fn install_invalid_config_warning() { | "); } + +#[test] +fn install_tips_on_unmatched_hook_stages() { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + repos: + - repo: local + hooks: + - id: push-only + name: Push only + language: system + entry: echo push + stages: [pre-push] + - id: every-stage + name: Every stage + language: system + entry: echo all + "}); + + // Default install only sets up the `pre-commit` shim, so `push-only` won't run, + // while `every-stage` (no stages, runs everywhere) is not reported. + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `push-only` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); + + // Installing the `pre-push` shim covers `push-only`, so no tip is emitted. + cmd_snapshot!(context.filters(), context.install().arg("--hook-type").arg("pre-push"), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-push` + + ----- stderr ----- + "); +} + +#[test] +fn install_no_tip_for_manual_only_hook() { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + repos: + - repo: local + hooks: + - id: manual-only + name: Manual only + language: system + entry: echo manual + stages: [manual] + "}); + + // `manual` has no git shim, so a manual-only hook is never reported. + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_tips_on_unmatched_default_stages() { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + default_stages: [pre-push] + repos: + - repo: local + hooks: + - id: inherits-default + name: Inherits default + language: system + entry: echo default + "}); + + // The hook inherits `default_stages: [pre-push]`, which the default install doesn't cover. + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `inherits-default` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_skips_remote_hook_without_config_stages() { + let context = TestContext::new(); + context.init_project(); + + // A remote hook may set its stages in the manifest, so `default_stages` must not be + // assumed to apply here; the hook should not be reported. + context.write_pre_commit_config(indoc! {r" + default_stages: [pre-push] + repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v5.0.0 + hooks: + - id: trailing-whitespace + "}); + + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_tips_on_unmatched_remote_hook_with_config_stages() { + let context = TestContext::new(); + context.init_project(); + + // Explicit config-level stages override the manifest, so they can be trusted. + context.write_pre_commit_config(indoc! {r" + repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v5.0.0 + hooks: + - id: trailing-whitespace + stages: [pre-push] + "}); + + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `trailing-whitespace` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_tips_for_workspace_subproject_hooks() -> anyhow::Result<()> { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + repos: + - repo: local + hooks: + - id: root-hook + name: Root hook + language: system + entry: echo root + "}); + + let nested = context.work_dir().child("nested"); + nested.create_dir_all()?; + nested + .child(".pre-commit-config.yaml") + .write_str(indoc! {r" + repos: + - repo: local + hooks: + - id: nested-push-hook + name: Nested push hook + language: system + entry: echo nested + stages: [pre-push] + "})?; + context.git_add("."); + + // A plain root install runs the nested project too, so its pre-push-only hook is reported. + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `nested-push-hook` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); + + // Skipping the nested project excludes its hooks from the installed shims, so no tip. + cmd_snapshot!(context.filters(), context.install().arg("--skip").arg("nested/"), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); + + Ok(()) +} + +#[test] +fn install_tips_on_remote_hook_with_empty_config_stages() { + let context = TestContext::new(); + context.init_project(); + + // An explicit `stages: []` overrides the manifest and resolves through `default_stages`, + // so the hook is effectively pre-push-only and must be reported. + context.write_pre_commit_config(indoc! {r" + default_stages: [pre-push] + repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v5.0.0 + hooks: + - id: trailing-whitespace + stages: [] + "}); + + cmd_snapshot!(context.filters(), context.install(), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `trailing-whitespace` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_no_tip_for_unselected_hooks() { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + repos: + - repo: local + hooks: + - id: push-only + name: Push only + language: system + entry: echo push + stages: [pre-push] + - id: other-hook + name: Other hook + language: system + entry: echo other + "}); + + // The installed shim persists `--skip=push-only`, so the hook never runs and isn't reported. + cmd_snapshot!(context.filters(), context.install().arg("--skip").arg("push-only"), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); + + // `--include other-hook` rules out `push-only` from the shim entirely. + cmd_snapshot!(context.filters(), context.install().arg("other-hook"), @r" + success: true + exit_code: 0 + ----- stdout ----- + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); + + // Including the unmatched hook itself keeps the tip. + cmd_snapshot!(context.filters(), context.install().arg("push-only"), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `push-only` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + "); +} + +#[test] +fn install_tips_despite_env_var_skip() { + let context = TestContext::new(); + context.init_project(); + + context.write_pre_commit_config(indoc! {r" + repos: + - repo: local + hooks: + - id: push-only + name: Push only + language: system + entry: echo push + stages: [pre-push] + "}); + + // Env-var skips are not persisted into the shim, so the hook would still run on + // pre-push and the tip must not be suppressed. + cmd_snapshot!(context.filters(), context.install().env(EnvVars::PREK_SKIP, "push-only"), @r" + success: true + exit_code: 0 + ----- stdout ----- + tip: the following hooks are configured for stages that aren't installed, so they'll only run when invoked manually or from CI, not automatically: + - `push-only` (pre-push) + If that's intentional, no action is needed. Otherwise, install the missing hook type(s) with `prek install --hook-type `, or set `default_install_hook_types` in your config. + prek installed at `.git/hooks/pre-commit` + + ----- stderr ----- + warning: Skip selectors from environment variables `PREK_SKIP` are ignored during installing hooks. + "); +}