Preserve additional dependency order (#2311)

Preserve the user-specified order and duplicates of hook
`additional_dependencies` for installation and environment reuse.

Hook environment markers now use schema version 1 and store remote
repository identity as structured `{ url, rev }` data. Legacy markers
are invalidated, and cache matching distinguishes dependency order,
duplicates, and repository identity.

Closes #1602
Partially supersedes #1603

Author: j178

crates/prek/src/cli/cache_gc.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::fmt::Write;
 use std::fmt::{Display, Formatter};
 use std::ops::AddAssign;
@@ -15,7 +16,9 @@ use crate::cli::ExitStatus;
 use crate::cli::cache_size::{dir_size_bytes, human_readable_bytes};
 use crate::cli::run::InstallCache;
 use crate::config::{self, Error as ConfigError, Repo as ConfigRepo, load_config};
-use crate::hook::{HOOK_MARKER, HookEnvKey, HookSpec, InstallInfo, Repo as HookRepo};
+use crate::hook::{
+    HOOK_MARKER, HookEnvRequirement, HookSpec, InstallInfo, Repo as HookRepo, RepoIdentityRef,
+};
 use crate::printer::Printer;
 use crate::store::{CacheBucket, REPO_MARKER, Store, ToolBucket};
 
@@ -160,7 +163,7 @@ pub(crate) async fn cache_gc(
     let mut used_tools: FxHashSet<ToolBucket> = FxHashSet::default();
     let mut used_tool_versions: FxHashMap<ToolBucket, FxHashSet<String>> = FxHashMap::default();
     let mut used_cache: FxHashSet<CacheBucket> = FxHashSet::default();
-    let mut used_env_keys: Vec<HookEnvKey> = Vec::new();
+    let mut used_env_requirements: Vec<HookEnvRequirement> = Vec::new();
 
     // Always keep Prek's own cache.
     used_cache.insert(CacheBucket::Prek);
@@ -187,7 +190,7 @@ pub(crate) async fn cache_gc(
         };
         kept_configs.insert(config_path);
 
-        used_env_keys.extend(hook_env_keys_from_config(store, &config));
+        used_env_requirements.extend(hook_env_requirements_from_config(store, &config));
 
         // Mark repos referenced by this config (if present in store).
         // We do this via config parsing (no clone), so GC won't keep repos for missing configs.
@@ -206,17 +209,20 @@ pub(crate) async fn cache_gc(
     }
 
     // Mark tools/caches from hook languages.
-    for key in &used_env_keys {
-        used_tools.extend(key.language.tool_buckets());
-        used_cache.extend(key.language.cache_buckets());
+    for requirement in &used_env_requirements {
+        used_tools.extend(requirement.language.tool_buckets());
+        used_cache.extend(requirement.language.cache_buckets());
     }
 
     // Mark hook environments by matching already-installed env metadata.
     // While doing this, try to derive the specific tool *version* directories in use from
     // `InstallInfo.toolchain` (which is persisted in `.prek-hook.json`).
     for installed in install_cache.installed_hooks(store).await {
         let info = installed.info_ref();
-        if used_env_keys.iter().any(|k| k.matches_install_info(info)) {
+        if used_env_requirements
+            .iter()
+            .any(|requirement| requirement.as_ref().is_satisfied_by(info))
+        {
             if let Some(dir) = info
                 .env_path
                 .file_name()
@@ -352,8 +358,11 @@ fn print_removed_details(printer: Printer, verb: &str, removal: &Removal) -> Res
     Ok(())
 }
 
-fn hook_env_keys_from_config(store: &Store, config: &config::Config) -> Vec<HookEnvKey> {
-    let mut keys = Vec::new();
+fn hook_env_requirements_from_config(
+    store: &Store,
+    config: &config::Config,
+) -> Vec<HookEnvRequirement> {
+    let mut requirements = Vec::new();
 
     for repo_config in &config.repos {
         match repo_config {
@@ -375,7 +384,7 @@ fn hook_env_keys_from_config(store: &Store, config: &config::Config) -> Vec<Hook
                     }
                 };
 
-                let remote_dep = repo_config.to_string();
+                let remote_repo = RepoIdentityRef::new(&repo_config.repo, &repo_config.rev);
 
                 for hook_config in &repo_config.hooks {
                     let Some(manifest_hook) = repo.get_hook(&hook_config.id) else {
@@ -385,23 +394,23 @@ fn hook_env_keys_from_config(store: &Store, config: &config::Config) -> Vec<Hook
                     let mut hook_spec = manifest_hook.clone();
                     hook_spec.apply_remote_hook_overrides(hook_config);
 
-                    match HookEnvKey::from_hook_spec(config, hook_spec, Some(&remote_dep)) {
-                        Ok(Some(key)) => keys.push(key),
+                    match HookEnvRequirement::from_hook_spec(config, hook_spec, Some(remote_repo)) {
+                        Ok(Some(requirement)) => requirements.push(requirement),
                         Ok(None) => {}
                         Err(err) => {
-                            warn!(hook = %hook_config.id, repo = %remote_dep, %err, "Failed to compute hook env key, skipping");
+                            warn!(hook = %hook_config.id, repo = %remote_repo, %err, "Failed to compute hook environment requirement, skipping");
                         }
                     }
                 }
             }
             ConfigRepo::Local(repo_config) => {
                 for hook in &repo_config.hooks {
                     let hook_spec = HookSpec::from(hook.clone());
-                    match HookEnvKey::from_hook_spec(config, hook_spec, None) {
-                        Ok(Some(key)) => keys.push(key),
+                    match HookEnvRequirement::from_hook_spec(config, hook_spec, None) {
+                        Ok(Some(requirement)) => requirements.push(requirement),
                         Ok(None) => {}
                         Err(err) => {
-                            warn!(hook = %hook.id, %err, "Failed to compute hook env key, skipping");
+                            warn!(hook = %hook.id, %err, "Failed to compute hook environment requirement, skipping");
                         }
                     }
                 }
@@ -410,7 +419,7 @@ fn hook_env_keys_from_config(store: &Store, config: &config::Config) -> Vec<Hook
         }
     }
 
-    keys
+    requirements
 }
 
 fn mark_tool_versions_from_install_info(
@@ -731,7 +740,7 @@ fn detail_lines_for_entry(
                 info.language_version
             ));
 
-            let (repo_dep, deps) = split_repo_dependency(&info.dependencies);
+            let (repo_dep, deps) = marker_repo_and_dependencies(info);
             if let Some(repo_dep) = repo_dep {
                 lines.push(format!(
                     "{}: {}",
@@ -780,8 +789,8 @@ fn truncate_end(s: &str, max_chars: usize) -> String {
     out
 }
 
-fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<String>) {
-    // Best-effort: the remote repo dependency is typically `repo@rev`.
+fn split_repo_dependency(deps: &[String]) -> (Option<String>, Vec<String>) {
+    // Legacy markers stored the remote repo identity as a `repo@rev` dependency.
     // Prefer URL-like values to avoid accidentally treating PEP508 deps as repo identifiers.
     let mut repo_dep: Option<String> = None;
     let mut rest = Vec::new();
@@ -804,6 +813,18 @@ fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<Strin
     (repo_dep, rest)
 }
 
+fn marker_repo_and_dependencies(info: &InstallInfo) -> (Option<String>, Cow<'_, [String]>) {
+    if info.schema_version() == 0 {
+        let (repo, dependencies) = split_repo_dependency(&info.dependencies);
+        (repo, Cow::Owned(dependencies))
+    } else {
+        (
+            info.repo().map(|repo| repo.to_string()),
+            Cow::Borrowed(&info.dependencies),
+        )
+    }
+}
+
 fn format_dependency_list(deps: &[String], max_items: usize, max_chars: usize) -> String {
     if deps.is_empty() {
         return String::new();
@@ -843,10 +864,11 @@ mod tests {
 
     #[test]
     fn split_repo_dependency_prefers_url_like_repo_at_rev() {
-        let mut deps = FxHashSet::default();
-        deps.insert("requests==2.32.0".to_string());
-        deps.insert("black==24.1.0".to_string());
-        deps.insert("https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string());
+        let deps = vec![
+            "requests==2.32.0".to_string(),
+            "black==24.1.0".to_string(),
+            "https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string(),
+        ];
 
         let (repo_dep, rest) = split_repo_dependency(&deps);
 
@@ -859,15 +881,39 @@ mod tests {
 
     #[test]
     fn split_repo_dependency_returns_none_when_no_repo_like_dep() {
-        let mut deps = FxHashSet::default();
-        deps.insert("requests==2.32.0".to_string());
-        deps.insert("black==24.1.0".to_string());
+        let deps = vec!["requests==2.32.0".to_string(), "black==24.1.0".to_string()];
 
         let (repo_dep, rest) = split_repo_dependency(&deps);
         assert!(repo_dep.is_none());
         assert_eq!(rest, vec!["black==24.1.0", "requests==2.32.0"]);
     }
 
+    #[test]
+    fn marker_repo_and_dependencies_uses_structured_repo() {
+        let info: InstallInfo = serde_json::from_value(serde_json::json!({
+            "schema_version": 1,
+            "language": "python",
+            "language_version": "3.12.0",
+            "repo": {
+                "url": "https://example.com/repo",
+                "rev": "v1.0.0",
+            },
+            "dependencies": ["https://example.com/dependency@v2.0.0"],
+            "env_path": "/tmp/hook-env",
+            "toolchain": "/usr/bin/python3",
+            "extra": {},
+        }))
+        .expect("deserialize install info");
+
+        let (repo, dependencies) = marker_repo_and_dependencies(&info);
+
+        assert_eq!(repo.as_deref(), Some("https://example.com/repo@v1.0.0"));
+        assert_eq!(
+            dependencies.as_ref(),
+            &["https://example.com/dependency@v2.0.0".to_string()]
+        );
+    }
+
     #[test]
     fn format_dependency_list_includes_more_suffix() {
         let deps = vec!["a".to_string(), "b".to_string(), "c".to_string()];

crates/prek/src/cli/run/install.rs

@@ -5,7 +5,6 @@ use anyhow::{Context, Result};
 use futures_util::stream::{FuturesUnordered, StreamExt};
 use mea::once::OnceCell;
 use mea::semaphore::Semaphore;
-use rustc_hash::FxHashMap;
 use tracing::{debug, warn};
 
 use crate::cli::reporter::HookInstallReporter;
@@ -73,17 +72,24 @@ async fn install_partition(
     for hook in hooks {
         debug_assert!(hook.needs_install_env());
 
-        let installed_hook = if let Some(info) = installed_hooks.iter().find_map(|installed| {
-            let InstalledHook::Installed { info, .. } = installed else {
-                return None;
-            };
-            info.matches(&hook).then(|| info.clone())
-        }) {
+        let reusable_info = hook.environment_requirement().and_then(|requirement| {
+            installed_hooks.iter().find_map(|installed| {
+                let InstalledHook::Installed { info, .. } = installed else {
+                    return None;
+                };
+                requirement.is_satisfied_by(info).then_some(info)
+            })
+        });
+
+        let installed_hook = if let Some(info) = reusable_info {
             debug!(
                 "Found installed environment for hook `{hook}` at `{}`",
                 info.env_path.display()
             );
-            InstalledHook::Installed { hook, info }
+            InstalledHook::Installed {
+                hook,
+                info: Arc::clone(info),
+            }
         } else {
             let _permit = semaphore.acquire(1).await;
 
@@ -117,43 +123,41 @@ async fn install_partition(
 
 /// Group hooks so each partition can install independently.
 ///
-/// Different languages can install concurrently. Hooks with the same language and dependency set
-/// stay in one partition so later hooks can reuse an environment installed by an earlier hook.
+/// Hooks with the same install language, repository, and dependency sequence stay in one
+/// partition so later hooks can reuse an environment installed by an earlier hook. Version
+/// requirements are checked by the full environment requirement and intentionally do not split
+/// partitions.
 fn partition_hooks(hooks: Vec<Arc<Hook>>) -> Vec<Vec<Arc<Hook>>> {
-    let mut hooks_by_language = FxHashMap::default();
+    let mut partitions: Vec<Vec<Arc<Hook>>> = Vec::new();
     for hook in hooks {
-        // `pygrep` hooks use Python installation and can share Python environments.
-        let language = if hook.language == Language::Pygrep {
-            Language::Python
+        if let Some(partition) = partitions
+            .iter_mut()
+            .find(|partition| same_install_partition(&partition[0], &hook))
+        {
+            partition.push(hook);
         } else {
-            hook.language
-        };
-        hooks_by_language
-            .entry(language)
-            .or_insert_with(Vec::new)
-            .push(hook);
-    }
-
-    let mut partitions = Vec::new();
-    for (_, hooks) in hooks_by_language {
-        let mut groups: Vec<Vec<Arc<Hook>>> = Vec::new();
-        for hook in hooks {
-            let group_index = groups
-                .iter()
-                .position(|group| group[0].env_key_dependencies() == hook.env_key_dependencies());
-
-            if let Some(index) = group_index {
-                groups[index].push(hook);
-            } else {
-                groups.push(vec![hook]);
-            }
+            partitions.push(vec![hook]);
         }
-        partitions.extend(groups);
     }
 
     partitions
 }
 
+fn same_install_partition(left: &Hook, right: &Hook) -> bool {
+    partition_language(left.language) == partition_language(right.language)
+        && left.repo().identity() == right.repo().identity()
+        && left.additional_dependencies == right.additional_dependencies
+}
+
+fn partition_language(language: Language) -> Language {
+    // Both `pygrep` and Python may provision Python, so schedule them as one language.
+    if language == Language::Pygrep {
+        Language::Python
+    } else {
+        language
+    }
+}
+
 /// Cached metadata for one environment found in the store hooks directory.
 ///
 /// Health is checked lazily because scanning the store can find many environments that will not
@@ -172,10 +176,6 @@ impl CachedInstallInfo {
         }
     }
 
-    fn matches(&self, hook: &Hook) -> bool {
-        self.info.matches(hook)
-    }
-
     fn info(&self) -> Arc<InstallInfo> {
         self.info.clone()
     }
@@ -238,15 +238,16 @@ impl InstallCache {
     /// Return a healthy installed environment from the store cache for this hook.
     ///
     /// This only looks at environments loaded from `store.hooks_dir()`. Environments created
-    /// during the current install call are reused inside `install_partition`, where hooks with
-    /// the same install key are installed sequentially.
+    /// during the current install call are reused inside `install_partition`, where hooks in the
+    /// same install partition are processed sequentially.
     pub(crate) async fn installed_hook(
         &self,
         store: &Store,
         hook: Arc<Hook>,
     ) -> Option<InstalledHook> {
+        let requirement = hook.environment_requirement()?;
         for env in self.installed_hooks(store).await {
-            if env.matches(&hook) && env.ensure_healthy().await {
+            if requirement.is_satisfied_by(env.info_ref()) && env.ensure_healthy().await {
                 return Some(InstalledHook::Installed {
                     hook,
                     info: env.info(),