Skip to content
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
61b7dd7
fix: handle PathError gracefully in add_files_from_stdout
devin-ai-integration[bot] Apr 30, 2025
91441b5
fix: format warn! macro according to Rust style guidelines
devin-ai-integration[bot] Apr 30, 2025
30bb433
test: add test coverage for handling unancorable paths
devin-ai-integration[bot] Apr 30, 2025
29eed78
style: fix formatting in test code
devin-ai-integration[bot] Apr 30, 2025
656c20f
Update crates/turborepo-scm/src/git.rs
anthonyshew Apr 30, 2025
517a5d3
fix: bubble up errors from add_files_from_stdout and handle them in c…
devin-ai-integration[bot] Apr 30, 2025
39bb312
style: fix formatting issues to pass Rust lints
devin-ai-integration[bot] Apr 30, 2025
2bbcf47
fix: use GitRefNotFound instead of non-existent Other variant
devin-ai-integration[bot] Apr 30, 2025
0b6951e
fix: handle path errors with warnings instead of bubbling up errors
devin-ai-integration[bot] Apr 30, 2025
aa8f5e9
fix: bubble up errors from add_files_from_stdout and handle them in c…
devin-ai-integration[bot] Apr 30, 2025
b91af4f
fix: return error directly instead of wrapping in Error::Path
devin-ai-integration[bot] Apr 30, 2025
7970096
style: fix formatting issues to pass Rust lints
devin-ai-integration[bot] Apr 30, 2025
9e52ecf
fix: log warnings for problematic paths instead of returning errors
devin-ai-integration[bot] Apr 30, 2025
91bf779
test: update test_add_files_from_stdout_with_unancorable_path to expe…
devin-ai-integration[bot] Apr 30, 2025
225d993
fix: format warning macro to pass Rust lints
devin-ai-integration[bot] Apr 30, 2025
2bc0866
fix: address PR feedback from chris-olszewski
devin-ai-integration[bot] May 1, 2025
7762841
style: format warning macro to match rustfmt style
devin-ai-integration[bot] May 1, 2025
c5bed4e
fix: fix type mismatches in change_detector.rs
devin-ai-integration[bot] May 1, 2025
6d59b8c
fix: remove ? operator to fix mismatched types in change_detector.rs
devin-ai-integration[bot] May 1, 2025
a085d64
test: update test to expect success instead of error
devin-ai-integration[bot] May 1, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 42 additions & 3 deletions crates/turborepo-lib/src/run/scope/change_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use turborepo_repository::{
},
package_graph::{PackageGraph, PackageName},
};
use turborepo_scm::{git::InvalidRange, SCM};
use turborepo_scm::{git::InvalidRange, Error as ScmError, SCM};

use crate::run::scope::ResolutionError;

Expand Down Expand Up @@ -109,8 +109,47 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> {
(
name.to_owned(),
PackageInclusionReason::All(AllPackageChangeReason::GitRefNotFound {
from_ref: from_ref.clone(),
to_ref: to_ref.clone(),
from_ref,
to_ref,
}),
)
})
.collect());
}
Err(ScmError::Path(err, _)) => {
Copy link
Contributor

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?

warn!(
"Could not process some file paths: {}. Defaulting to all packages changed.",
err
);
debug!("path error: {}, defaulting to all packages changed", err);
return Ok(self
.pkg_graph
.packages()
.map(|(name, _)| {
(
name.to_owned(),
PackageInclusionReason::All(AllPackageChangeReason::GitRefNotFound {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this variant now that it is for many different git related failures instead of just the ref not being found?

from_ref: from_ref.map(String::from),
to_ref: to_ref.map(String::from),
}),
)
})
.collect());
}
Err(err) => {
debug!(
"unexpected error: {}, defaulting to all packages changed",
err
);
return Ok(self
.pkg_graph
.packages()
.map(|(name, _)| {
(
name.to_owned(),
PackageInclusionReason::All(AllPackageChangeReason::GitRefNotFound {
from_ref: from_ref.map(String::from),
to_ref: to_ref.map(String::from),
}),
)
})
Expand Down
75 changes: 65 additions & 10 deletions crates/turborepo-scm/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -350,15 +360,29 @@ 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_lossy(&stdout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want a lossy conversion as this will cause silent failures upstream. Please report up any non-UTF8 output as an error.

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 RelativeUnixPath::new(line) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to handle the error case for RelativeUnixPath::new failing. We can just unwrap() it as it will only fail if given a path that starts with / and git will not output absolute paths for the command we're parsing.

Ok(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) => {
warn!(
"Skipping file that could not be anchored to turbo root: {} ({})",
line, err
);
}
}
}
Err(err) => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 change_detector.rs we should catch it and behave as if every package changed since we cannot identify which exact packages changed. The warning you added should be moved there with an updated message.

warn!("Skipping file with invalid path format: {} ({})", line, err);
}
}
}
Ok(())
}

fn reanchor_path_from_git_root_to_turbo_root(
Expand Down Expand Up @@ -1312,4 +1336,35 @@ 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/\\321\\216.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(())
}
}