Skip to content

Commit 2b719ef

Browse files
committed
fix: preserve additional_dependencies order and use synthetic local repo
Replace FxHashSet<String> with Vec<String> for additional_dependencies throughout the codebase. This preserves user-specified ordering and duplicates, matching pre-commit semantics. Previously, FxHashSet caused arbitrary reordering that broke hooks passing non-dependency arguments like `--index-url <url>` in additional_dependencies. For `repo: local` Python hooks, install from a synthetic placeholder repository (containing a minimal setup.py) instead of the project root. This prevents `.` in additional_dependencies from depending on project contents and matches pre-commit's behavior. Closes #1602 Closes #1603
1 parent 4406340 commit 2b719ef

File tree

23 files changed

+282
-116
lines changed

23 files changed

+282
-116
lines changed

crates/prek/src/cli/cache_gc.rs

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ fn truncate_end(s: &str, max_chars: usize) -> String {
694694
out
695695
}
696696

697-
fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<String>) {
697+
fn split_repo_dependency(deps: &[String]) -> (Option<String>, Vec<String>) {
698698
// Best-effort: the remote repo dependency is typically `repo@rev`.
699699
// Prefer URL-like values to avoid accidentally treating PEP508 deps as repo identifiers.
700700
let mut repo_dep: Option<String> = None;
@@ -714,7 +714,6 @@ fn split_repo_dependency(deps: &FxHashSet<String>) -> (Option<String>, Vec<Strin
714714
}
715715
}
716716

717-
rest.sort_unstable();
718717
(repo_dep, rest)
719718
}
720719

@@ -757,29 +756,28 @@ mod tests {
757756

758757
#[test]
759758
fn split_repo_dependency_prefers_url_like_repo_at_rev() {
760-
let mut deps = FxHashSet::default();
761-
deps.insert("requests==2.32.0".to_string());
762-
deps.insert("black==24.1.0".to_string());
763-
deps.insert("https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string());
759+
let deps = vec![
760+
"requests==2.32.0".to_string(),
761+
"black==24.1.0".to_string(),
762+
"https://github.com/pre-commit/pre-commit-hooks@v1.0.0".to_string(),
763+
];
764764

765765
let (repo_dep, rest) = split_repo_dependency(&deps);
766766

767767
assert_eq!(
768768
repo_dep.as_deref(),
769769
Some("https://github.com/pre-commit/pre-commit-hooks@v1.0.0")
770770
);
771-
assert_eq!(rest, vec!["black==24.1.0", "requests==2.32.0"]);
771+
assert_eq!(rest, vec!["requests==2.32.0", "black==24.1.0"]);
772772
}
773773

774774
#[test]
775775
fn split_repo_dependency_returns_none_when_no_repo_like_dep() {
776-
let mut deps = FxHashSet::default();
777-
deps.insert("requests==2.32.0".to_string());
778-
deps.insert("black==24.1.0".to_string());
776+
let deps = vec!["requests==2.32.0".to_string(), "black==24.1.0".to_string()];
779777

780778
let (repo_dep, rest) = split_repo_dependency(&deps);
781779
assert!(repo_dep.is_none());
782-
assert_eq!(rest, vec!["black==24.1.0", "requests==2.32.0"]);
780+
assert_eq!(rest, vec!["requests==2.32.0", "black==24.1.0"]);
783781
}
784782

785783
#[test]

crates/prek/src/hook.rs

Lines changed: 102 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use std::sync::{Arc, OnceLock};
99
use anyhow::{Context, Result};
1010
use clap::ValueEnum;
1111
use prek_consts::PRE_COMMIT_HOOKS_YAML;
12-
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet};
12+
use rustc_hash::FxHashMap;
1313
use serde::{Deserialize, Serialize};
1414
use tempfile::TempDir;
1515
use thiserror::Error;
@@ -330,11 +330,7 @@ impl HookBuilder {
330330
let pass_filenames = options.pass_filenames.unwrap_or(true);
331331
let require_serial = options.require_serial.unwrap_or(false);
332332
let verbose = options.verbose.unwrap_or(false);
333-
let additional_dependencies = options
334-
.additional_dependencies
335-
.unwrap_or_default()
336-
.into_iter()
337-
.collect::<FxHashSet<_>>();
333+
let additional_dependencies = options.additional_dependencies.unwrap_or_default();
338334

339335
let language_request = LanguageRequest::parse(self.hook_spec.language, &language_version)
340336
.map_err(|e| Error::Hook {
@@ -482,7 +478,7 @@ pub(crate) struct Hook {
482478
project: Arc<Project>,
483479
repo: Arc<Repo>,
484480
// Cached computed dependencies.
485-
dependencies: OnceLock<FxHashSet<String>>,
481+
dependencies: OnceLock<Vec<String>>,
486482

487483
/// The index of the hook defined in the configuration file.
488484
pub idx: usize,
@@ -496,7 +492,7 @@ pub(crate) struct Hook {
496492
pub types: Vec<String>,
497493
pub types_or: Vec<String>,
498494
pub exclude_types: Vec<String>,
499-
pub additional_dependencies: FxHashSet<String>,
495+
pub additional_dependencies: Vec<String>,
500496
pub args: Vec<String>,
501497
pub env: FxHashMap<String, String>,
502498
pub always_run: bool,
@@ -558,7 +554,7 @@ impl Hook {
558554
///
559555
/// For remote hooks, the repo URL is included to avoid reusing an environment created
560556
/// from a different remote repository.
561-
pub(crate) fn env_key_dependencies(&self) -> &FxHashSet<String> {
557+
pub(crate) fn env_key_dependencies(&self) -> &[String] {
562558
if !self.is_remote() {
563559
return &self.additional_dependencies;
564560
}
@@ -586,10 +582,11 @@ impl Hook {
586582
///
587583
/// For remote hooks, this includes the local path to the cloned repository so that
588584
/// installers can install the hook's package/project itself.
589-
pub(crate) fn install_dependencies(&self) -> Cow<'_, FxHashSet<String>> {
585+
pub(crate) fn install_dependencies(&self) -> Cow<'_, [String]> {
590586
if let Some(repo_path) = self.repo_path() {
591-
let mut deps = self.additional_dependencies.clone();
592-
deps.insert(repo_path.to_string_lossy().to_string());
587+
let mut deps = Vec::with_capacity(self.additional_dependencies.len() + 1);
588+
deps.push(repo_path.to_string_lossy().to_string());
589+
deps.extend(self.additional_dependencies.iter().cloned());
593590
Cow::Owned(deps)
594591
} else {
595592
Cow::Borrowed(&self.additional_dependencies)
@@ -600,7 +597,7 @@ impl Hook {
600597
#[derive(Debug, Clone)]
601598
pub(crate) struct HookEnvKey {
602599
pub(crate) language: Language,
603-
pub(crate) dependencies: FxHashSet<String>,
600+
pub(crate) dependencies: Vec<String>,
604601
pub(crate) language_request: LanguageRequest,
605602
}
606603

@@ -609,25 +606,24 @@ pub(crate) struct HookEnvKey {
609606
#[derive(Debug, Clone, Copy)]
610607
pub(crate) struct HookEnvKeyRef<'a> {
611608
pub(crate) language: Language,
612-
pub(crate) dependencies: &'a FxHashSet<String>,
609+
pub(crate) dependencies: &'a [String],
613610
pub(crate) language_request: &'a LanguageRequest,
614611
}
615612

616-
/// Builds the dependency set used to identify a hook environment.
613+
/// Builds the dependency list used to identify a hook environment.
617614
///
618615
/// For remote hooks, `remote_repo_dependency` is included so environments from different
619616
/// repositories are not reused accidentally.
620617
fn env_key_dependencies(
621-
additional_dependencies: &FxHashSet<String>,
618+
additional_dependencies: &[String],
622619
remote_repo_dependency: Option<&str>,
623-
) -> FxHashSet<String> {
624-
let mut deps = FxHashSet::with_capacity_and_hasher(
620+
) -> Vec<String> {
621+
let mut deps = Vec::with_capacity(
625622
additional_dependencies.len() + usize::from(remote_repo_dependency.is_some()),
626-
FxBuildHasher,
627623
);
628624
deps.extend(additional_dependencies.iter().cloned());
629625
if let Some(dep) = remote_repo_dependency {
630-
deps.insert(dep.to_string());
626+
deps.push(dep.to_string());
631627
}
632628
deps
633629
}
@@ -636,7 +632,7 @@ fn env_key_dependencies(
636632
/// environment described by [`InstallInfo`].
637633
fn matches_install_info(
638634
language: Language,
639-
dependencies: &FxHashSet<String>,
635+
dependencies: &[String],
640636
language_request: &LanguageRequest,
641637
info: &InstallInfo,
642638
) -> bool {
@@ -674,11 +670,11 @@ impl HookEnvKey {
674670
)
675671
})?;
676672

677-
let additional_dependencies: FxHashSet<String> = hook_spec
673+
let additional_dependencies: Vec<String> = hook_spec
678674
.options
679675
.additional_dependencies
680-
.as_ref()
681-
.map_or_else(FxHashSet::default, |deps| deps.iter().cloned().collect());
676+
.clone()
677+
.unwrap_or_default();
682678

683679
let dependencies = env_key_dependencies(&additional_dependencies, remote_repo_dependency);
684680

@@ -786,7 +782,7 @@ impl InstalledHook {
786782
pub(crate) struct InstallInfo {
787783
pub(crate) language: Language,
788784
pub(crate) language_version: semver::Version,
789-
pub(crate) dependencies: FxHashSet<String>,
785+
pub(crate) dependencies: Vec<String>,
790786
pub(crate) env_path: PathBuf,
791787
pub(crate) toolchain: PathBuf,
792788
extra: FxHashMap<String, String>,
@@ -811,7 +807,7 @@ impl Clone for InstallInfo {
811807
impl InstallInfo {
812808
pub(crate) fn new(
813809
language: Language,
814-
dependencies: FxHashSet<String>,
810+
dependencies: Vec<String>,
815811
hooks_dir: &Path,
816812
) -> Result<Self, Error> {
817813
let env_path = tempfile::Builder::new()
@@ -1004,7 +1000,7 @@ mod tests {
10041000
],
10051001
types_or: [],
10061002
exclude_types: [],
1007-
additional_dependencies: {},
1003+
additional_dependencies: [],
10081004
args: [
10091005
"--flag",
10101006
],
@@ -1039,4 +1035,83 @@ mod tests {
10391035

10401036
Ok(())
10411037
}
1038+
1039+
#[tokio::test]
1040+
async fn hook_builder_preserves_additional_dependency_order_and_duplicates() -> Result<()> {
1041+
let temp = tempfile::tempdir()?;
1042+
let config_path = temp.path().join(PRE_COMMIT_CONFIG_YAML);
1043+
fs_err::write(&config_path, "repos: []\n")?;
1044+
1045+
let project = Arc::new(Project::from_config_file(
1046+
Cow::Borrowed(&config_path),
1047+
None,
1048+
)?);
1049+
let repo = Arc::new(Repo::Local { hooks: vec![] });
1050+
1051+
let expected = vec![
1052+
"--index-url".to_string(),
1053+
"https://pypi.org/simple".to_string(),
1054+
"pyecho-cli".to_string(),
1055+
"pyecho-cli".to_string(),
1056+
];
1057+
1058+
let hook_spec = HookSpec {
1059+
id: "ordered-deps".to_string(),
1060+
name: "ordered-deps".to_string(),
1061+
entry: "python -c 'print(1)'".to_string(),
1062+
language: Language::Python,
1063+
priority: None,
1064+
options: HookOptions {
1065+
additional_dependencies: Some(expected.clone()),
1066+
..Default::default()
1067+
},
1068+
};
1069+
1070+
let hook = HookBuilder::new(project.clone(), repo.clone(), hook_spec, 0)
1071+
.build()
1072+
.await?;
1073+
assert_eq!(hook.additional_dependencies, expected);
1074+
1075+
let hook_a = HookBuilder::new(
1076+
project.clone(),
1077+
repo.clone(),
1078+
HookSpec {
1079+
id: "a".to_string(),
1080+
name: "a".to_string(),
1081+
entry: "python -c 'print(1)'".to_string(),
1082+
language: Language::Python,
1083+
priority: None,
1084+
options: HookOptions {
1085+
additional_dependencies: Some(vec!["foo".to_string(), "bar".to_string()]),
1086+
..Default::default()
1087+
},
1088+
},
1089+
1,
1090+
)
1091+
.build()
1092+
.await?;
1093+
1094+
let hook_b = HookBuilder::new(
1095+
project,
1096+
repo,
1097+
HookSpec {
1098+
id: "b".to_string(),
1099+
name: "b".to_string(),
1100+
entry: "python -c 'print(1)'".to_string(),
1101+
language: Language::Python,
1102+
priority: None,
1103+
options: HookOptions {
1104+
additional_dependencies: Some(vec!["bar".to_string(), "foo".to_string()]),
1105+
..Default::default()
1106+
},
1107+
},
1108+
2,
1109+
)
1110+
.build()
1111+
.await?;
1112+
1113+
assert_ne!(hook_a.env_key_dependencies(), hook_b.env_key_dependencies());
1114+
1115+
Ok(())
1116+
}
10421117
}

crates/prek/src/languages/bun/bun.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ impl LanguageImpl for Bun {
5353

5454
let mut info = InstallInfo::new(
5555
hook.language,
56-
hook.env_key_dependencies().clone(),
56+
hook.env_key_dependencies().to_vec(),
5757
&store.hooks_dir(),
5858
)?;
5959

crates/prek/src/languages/docker.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use std::borrow::Cow;
2-
use std::collections::BTreeSet;
32
use std::collections::hash_map::DefaultHasher;
43
use std::fs;
54
use std::hash::{Hash, Hasher};
@@ -323,8 +322,7 @@ impl Docker {
323322

324323
info.language.hash(&mut hasher);
325324
info.language_version.hash(&mut hasher);
326-
let deps = info.dependencies.iter().collect::<BTreeSet<&String>>();
327-
deps.hash(&mut hasher);
325+
info.dependencies.hash(&mut hasher);
328326

329327
let digest = hex::encode(hasher.finish().to_le_bytes());
330328
format!("prek-{digest}")
@@ -436,7 +434,7 @@ impl LanguageImpl for Docker {
436434

437435
let mut info = InstallInfo::new(
438436
hook.language,
439-
hook.env_key_dependencies().clone(),
437+
hook.env_key_dependencies().to_vec(),
440438
&store.hooks_dir(),
441439
)?;
442440

crates/prek/src/languages/golang/golang.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ impl LanguageImpl for Golang {
4545

4646
let mut info = InstallInfo::new(
4747
hook.language,
48-
hook.env_key_dependencies().clone(),
48+
hook.env_key_dependencies().to_vec(),
4949
&store.hooks_dir(),
5050
)?;
5151
info.with_toolchain(go.bin().to_path_buf())

crates/prek/src/languages/haskell.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ impl LanguageImpl for Haskell {
3333

3434
let mut info = InstallInfo::new(
3535
hook.language,
36-
hook.env_key_dependencies().clone(),
36+
hook.env_key_dependencies().to_vec(),
3737
&store.hooks_dir(),
3838
)?;
3939

crates/prek/src/languages/julia.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ impl LanguageImpl for Julia {
2626

2727
let mut info = InstallInfo::new(
2828
hook.language,
29-
hook.env_key_dependencies().clone(),
29+
hook.env_key_dependencies().to_vec(),
3030
&store.hooks_dir(),
3131
)?;
3232

crates/prek/src/languages/lua.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ impl LanguageImpl for Lua {
6565

6666
let mut info = InstallInfo::new(
6767
hook.language,
68-
hook.env_key_dependencies().clone(),
68+
hook.env_key_dependencies().to_vec(),
6969
&store.hooks_dir(),
7070
)?;
7171

crates/prek/src/languages/node/node.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ impl LanguageImpl for Node {
5454

5555
let mut info = InstallInfo::new(
5656
hook.language,
57-
hook.env_key_dependencies().clone(),
57+
hook.env_key_dependencies().to_vec(),
5858
&store.hooks_dir(),
5959
)?;
6060

crates/prek/src/languages/node/version.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,6 @@ mod tests {
257257
use super::{EXTRA_KEY_LTS, NodeRequest};
258258
use crate::config::Language;
259259
use crate::hook::InstallInfo;
260-
use rustc_hash::FxHashSet;
261260
use std::path::PathBuf;
262261
use std::str::FromStr;
263262

@@ -310,8 +309,7 @@ mod tests {
310309
#[test]
311310
fn test_node_request_satisfied_by() -> anyhow::Result<()> {
312311
let temp_dir = tempfile::tempdir()?;
313-
let mut install_info =
314-
InstallInfo::new(Language::Node, FxHashSet::default(), temp_dir.path())?;
312+
let mut install_info = InstallInfo::new(Language::Node, Vec::new(), temp_dir.path())?;
315313
install_info
316314
.with_language_version(semver::Version::new(12, 18, 3))
317315
.with_toolchain(PathBuf::from("/usr/bin/node"))

0 commit comments

Comments
 (0)