Skip to content

Commit dbca904

Browse files
j178Copilot
andauthored
Fix workspace-relative added file paths (#1852)
Fix workspace relative path for `check-case-conflict` and `check-added-large-files` --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 24f2d62 commit dbca904

File tree

3 files changed

+166
-21
lines changed

3 files changed

+166
-21
lines changed

crates/prek/src/git.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ pub(crate) async fn get_added_files(root: &Path) -> Result<Vec<PathBuf>, Error>
104104
.current_dir(root)
105105
.arg("diff")
106106
.arg("--staged")
107+
// `git diff --name-only` reports paths relative to the repository root by default,
108+
// even when it runs inside a subdirectory. `--relative` keeps the output aligned
109+
// with hooks, which receive filenames relative to their project root.
110+
.arg("--relative")
107111
.arg("--name-only")
108112
.arg("--diff-filter=A")
109113
.arg("-z") // Use NUL as line terminator
@@ -634,12 +638,16 @@ pub(crate) async fn get_shared_repository_file_mode(mode: u32) -> Result<u32> {
634638
}
635639
}
636640

637-
pub(crate) async fn get_lfs_files(paths: &[&Path]) -> Result<FxHashSet<PathBuf>, Error> {
641+
pub(crate) async fn get_lfs_files(
642+
current_dir: &Path,
643+
paths: &[&Path],
644+
) -> Result<FxHashSet<PathBuf>, Error> {
638645
if paths.is_empty() {
639646
return Ok(FxHashSet::default());
640647
}
641648

642649
let mut child = git_cmd("git check-attr")?
650+
.current_dir(current_dir)
643651
.arg("check-attr")
644652
.arg("filter")
645653
.arg("-z")

crates/prek/src/hooks/pre_commit_hooks/check_added_large_files.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ pub(crate) async fn check_added_large_files(
4949
FileFilter::Files(add_files)
5050
};
5151

52-
let lfs_files = get_lfs_files(filenames).await?;
52+
// Builtin hooks receive project-relative filenames, so git attribute lookups need to run
53+
// from the project root for nested `.gitattributes` files to apply.
54+
let lfs_files = get_lfs_files(hook.work_dir(), filenames).await?;
5355

5456
let filenames = filenames
5557
.iter()

crates/prek/tests/builtin_hooks.rs

Lines changed: 154 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,85 @@ fn check_added_large_files_hook() -> Result<()> {
687687
Ok(())
688688
}
689689

690+
#[test]
691+
fn check_added_large_files_workspace_mode_respects_project_relative_lfs_paths() -> Result<()> {
692+
let context = TestContext::new();
693+
context.init_project();
694+
695+
context.write_pre_commit_config("repos: []\n");
696+
697+
// Regression: builtin hooks receive project-relative filenames even in workspace mode.
698+
// `check-added-large-files` must therefore resolve git-lfs attributes relative to the
699+
// nested project root, not the workspace root.
700+
let app = context.work_dir().child("app");
701+
app.create_dir_all()?;
702+
// Use `--enforce-all` so this regression isolates git-lfs attribute lookup in workspace
703+
// mode instead of depending on the separate staged-file path filtering behavior.
704+
app.child(PRE_COMMIT_CONFIG_YAML)
705+
.write_str(indoc::indoc! {r"
706+
repos:
707+
- repo: builtin
708+
hooks:
709+
- id: check-added-large-files
710+
args: ['--maxkb', '1', '--enforce-all']
711+
"})?;
712+
app.child(".gitattributes")
713+
.write_str("*.dat filter=lfs diff=lfs merge=lfs -text")?;
714+
app.child("large.dat").write_binary(&[0; 2048])?;
715+
716+
context.git_add(".");
717+
718+
cmd_snapshot!(context.filters(), context.run(), @r"
719+
success: true
720+
exit_code: 0
721+
----- stdout -----
722+
Running hooks for `app`:
723+
check for added large files..............................................Passed
724+
725+
----- stderr -----
726+
");
727+
728+
Ok(())
729+
}
730+
731+
#[test]
732+
fn check_added_large_files_workspace_mode_respects_project_relative_added_files() -> Result<()> {
733+
let context = TestContext::new();
734+
context.init_project();
735+
736+
context.write_pre_commit_config("repos: []\n");
737+
738+
let app = context.work_dir().child("app");
739+
app.create_dir_all()?;
740+
app.child(PRE_COMMIT_CONFIG_YAML)
741+
.write_str(indoc::indoc! {r"
742+
repos:
743+
- repo: builtin
744+
hooks:
745+
- id: check-added-large-files
746+
args: ['--maxkb', '1']
747+
"})?;
748+
app.child("large.bin").write_binary(&[0; 2048])?;
749+
750+
context.git_add(".");
751+
752+
cmd_snapshot!(context.filters(), context.run(), @r"
753+
success: false
754+
exit_code: 1
755+
----- stdout -----
756+
Running hooks for `app`:
757+
check for added large files..............................................Failed
758+
- hook id: check-added-large-files
759+
- exit code: 1
760+
761+
large.bin (2 KB) exceeds 1 KB
762+
763+
----- stderr -----
764+
");
765+
766+
Ok(())
767+
}
768+
690769
#[test]
691770
fn tracked_file_exceeds_large_file_limit() -> Result<()> {
692771
let context = TestContext::new();
@@ -845,7 +924,11 @@ fn builtin_hooks_workspace_mode() -> Result<()> {
845924
- files were modified by this hook
846925
847926
Fixing trailing_ws.txt
848-
check for added large files..............................................Passed
927+
check for added large files..............................................Failed
928+
- hook id: check-added-large-files
929+
- exit code: 1
930+
931+
large.bin (2 KB) exceeds 1 KB
849932
850933
Running hooks for `.`:
851934
identity.................................................................Passed
@@ -870,18 +953,20 @@ fn builtin_hooks_workspace_mode() -> Result<()> {
870953
----- stderr -----
871954
");
872955

873-
// Fix YAML and JSON issues, then stage.
874-
app.child("invalid.yaml").write_str("a:\n b: c")?;
875-
app.child("duplicate.yaml").write_str("a: 1\nb: 2")?;
876-
app.child("invalid.json").write_str(r#"{"a": 1}"#)?;
956+
// Manually fix the files that can't be auto-fixed.
957+
app.child("invalid.yaml").write_str("a:\n b: c\n")?;
958+
app.child("duplicate.yaml").write_str("a: 1\nb: 2\n")?;
959+
app.child("invalid.json")
960+
.write_str(concat!(r#"{"a": 1}"#, "\n"))?;
877961
app.child("duplicate.json")
878-
.write_str(r#"{"a": 1, "b": 2}"#)?;
962+
.write_str(concat!(r#"{"a": 1, "b": 2}"#, "\n"))?;
963+
app.child("large.bin").write_binary(&[0u8; 100])?;
879964
context.git_add(".");
880965

881-
// Second run: all hooks should pass.
882-
cmd_snapshot!(context.filters(), context.run(), @r"
883-
success: false
884-
exit_code: 1
966+
// Second run: all hooks should now pass.
967+
cmd_snapshot!(context.filters(), context.run(), @"
968+
success: true
969+
exit_code: 0
885970
----- stdout -----
886971
Running hooks for `app`:
887972
identity.................................................................Passed
@@ -901,15 +986,7 @@ fn builtin_hooks_workspace_mode() -> Result<()> {
901986
invalid.json
902987
.pre-commit-config.yaml
903988
eof_no_newline.txt
904-
fix end of files.........................................................Failed
905-
- hook id: end-of-file-fixer
906-
- exit code: 1
907-
- files were modified by this hook
908-
909-
Fixing invalid.yaml
910-
Fixing duplicate.json
911-
Fixing duplicate.yaml
912-
Fixing invalid.json
989+
fix end of files.........................................................Passed
913990
check yaml...............................................................Passed
914991
check json...............................................................Passed
915992
mixed line ending........................................................Passed
@@ -2450,6 +2527,64 @@ fn check_case_conflict_among_new_files() -> Result<()> {
24502527
Ok(())
24512528
}
24522529

2530+
#[test]
2531+
fn check_case_conflict_workspace_mode_includes_added_files() -> Result<()> {
2532+
let context = TestContext::new();
2533+
context.init_project();
2534+
2535+
if !is_case_sensitive_filesystem(&context)? {
2536+
return Ok(());
2537+
}
2538+
2539+
context.write_pre_commit_config("repos: []\n");
2540+
2541+
let app = context.work_dir().child("app");
2542+
app.create_dir_all()?;
2543+
app.child("foo.txt").write_str("existing file")?;
2544+
app.child("trigger.txt").write_str("tracked trigger")?;
2545+
context.git_add(".");
2546+
context.git_commit("Initial commit");
2547+
2548+
app.child(PRE_COMMIT_CONFIG_YAML)
2549+
.write_str(indoc::indoc! {r"
2550+
repos:
2551+
- repo: builtin
2552+
hooks:
2553+
- id: check-case-conflict
2554+
"})?;
2555+
2556+
app.child("FOO.txt").write_str("conflicting case")?;
2557+
context.git_add("app/FOO.txt");
2558+
2559+
// Regression: in workspace mode, `get_added_files()` must return paths relative to the
2560+
// nested project root so added files still participate in conflict detection even when
2561+
// `--files` only names some other file in that project.
2562+
cmd_snapshot!(
2563+
context.filters(),
2564+
context
2565+
.run()
2566+
.arg("check-case-conflict")
2567+
.arg("--files")
2568+
.arg("app/trigger.txt"),
2569+
@r#"
2570+
success: false
2571+
exit_code: 1
2572+
----- stdout -----
2573+
Running hooks for `app`:
2574+
check for case conflicts.................................................Failed
2575+
- hook id: check-case-conflict
2576+
- exit code: 1
2577+
2578+
Case-insensitivity conflict found: FOO.txt
2579+
Case-insensitivity conflict found: foo.txt
2580+
2581+
----- stderr -----
2582+
"#
2583+
);
2584+
2585+
Ok(())
2586+
}
2587+
24532588
#[test]
24542589
fn check_json5() -> Result<()> {
24552590
let context = TestContext::new();

0 commit comments

Comments
 (0)