-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: handle PathError gracefully in add_files_from_stdout #10407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
61b7dd7
91441b5
30bb433
29eed78
656c20f
517a5d3
39bb312
2bbcf47
0b6951e
aa8f5e9
b91af4f
7970096
9e52ecf
91bf779
225d993
2bc0866
7762841
c5bed4e
6d59b8c
a085d64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,16 @@ pub struct InvalidRange { | |
| pub to_ref: Option<String>, | ||
| } | ||
|
|
||
| impl std::fmt::Display for InvalidRange { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!( | ||
| f, | ||
| "Invalid git range: from_ref={:?}, to_ref={:?}", | ||
| self.from_ref, self.to_ref | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| impl SCM { | ||
| pub fn get_current_branch(&self, path: &AbsoluteSystemPath) -> Result<String, Error> { | ||
| match self { | ||
|
|
@@ -303,7 +313,7 @@ impl GitRepo { | |
| } | ||
|
|
||
| let output = self.execute_git_command(&args, pathspec)?; | ||
| self.add_files_from_stdout(&mut files, turbo_root, output); | ||
| self.add_files_from_stdout(&mut files, turbo_root, output)?; | ||
|
|
||
| // We only care about non-tracked files if we haven't specified both ends up the | ||
| // comparison | ||
|
|
@@ -314,11 +324,11 @@ impl GitRepo { | |
| &["ls-files", "--others", "--modified", "--exclude-standard"], | ||
| pathspec, | ||
| )?; | ||
| self.add_files_from_stdout(&mut files, turbo_root, ls_files_output); | ||
| self.add_files_from_stdout(&mut files, turbo_root, ls_files_output)?; | ||
| // Include any files that have been staged, but not committed | ||
| let diff_output = | ||
| self.execute_git_command(&["diff", "--name-only", "--cached"], pathspec)?; | ||
| self.add_files_from_stdout(&mut files, turbo_root, diff_output); | ||
| self.add_files_from_stdout(&mut files, turbo_root, diff_output)?; | ||
| } | ||
|
|
||
| Ok(files) | ||
|
|
@@ -350,15 +360,23 @@ impl GitRepo { | |
| files: &mut HashSet<AnchoredSystemPathBuf>, | ||
| turbo_root: &AbsoluteSystemPath, | ||
| stdout: Vec<u8>, | ||
| ) { | ||
| let stdout = String::from_utf8(stdout).unwrap(); | ||
| ) -> Result<(), Error> { | ||
| let stdout = String::from_utf8(stdout)?; | ||
| for line in stdout.lines() { | ||
| let path = RelativeUnixPath::new(line).unwrap(); | ||
| let anchored_to_turbo_root_file_path = self | ||
| .reanchor_path_from_git_root_to_turbo_root(turbo_root, path) | ||
| .unwrap(); | ||
| files.insert(anchored_to_turbo_root_file_path); | ||
| match self.reanchor_path_from_git_root_to_turbo_root(turbo_root, path) { | ||
| Ok(anchored_to_turbo_root_file_path) => { | ||
| files.insert(anchored_to_turbo_root_file_path); | ||
| } | ||
| Err(err) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bubble up this error so it reaches the caller and they can decide what to do with it. In our case when this error is eventually returned in |
||
| warn!( | ||
| "Skipping file that could not be anchored to turbo root: {} ({})", | ||
anthonyshew marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| line, err | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn reanchor_path_from_git_root_to_turbo_root( | ||
|
|
@@ -1312,4 +1330,34 @@ mod tests { | |
|
|
||
| assert_eq!(None, actual); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_add_files_from_stdout_with_unancorable_path() -> Result<(), Error> { | ||
| let (repo_root, _repo) = setup_repository(None)?; | ||
| let git_root = AbsoluteSystemPathBuf::try_from(repo_root.path()).unwrap(); | ||
|
|
||
| // Create a separate directory that is not a subdirectory of git_root | ||
| let turbo_root = tempfile::tempdir()?; | ||
| let turbo_root_path = AbsoluteSystemPathBuf::try_from(turbo_root.path()).unwrap(); | ||
|
|
||
| // Create a GitRepo instance directly | ||
| let git_repo = GitRepo { | ||
| root: git_root.clone(), | ||
| bin: GitRepo::find_bin()?, | ||
| }; | ||
|
|
||
| // Create a HashSet to collect files | ||
| let mut files = HashSet::new(); | ||
|
|
||
| // Create stdout with a path that cannot be anchored | ||
| let problematic_path = "some/path/with/special/characters/test.spec.ts"; | ||
| let stdout = problematic_path.as_bytes().to_vec(); | ||
|
|
||
| let result = git_repo.add_files_from_stdout(&mut files, &turbo_root_path, stdout); | ||
|
|
||
| assert!(result.is_ok()); | ||
| assert!(files.is_empty()); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you deduplicate the generation of "all packages changed due to error" logic?