这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

Description

This PR fixes an issue where crates/turborepo-scm/src/git.rs panics in GitHub Actions when using the --affected flag with paths containing special characters (particularly Cyrillic symbols).

The panic occurs in the add_files_from_stdout method where it unwraps the result of reanchor_path_from_git_root_to_turbo_root, which can fail with a PathError::NotParent error.

This PR modifies the code to handle these errors gracefully by skipping problematic paths and logging a warning instead of panicking.

Fixes #10403

Verification

  • Verified that the code compiles
  • Verified that existing tests pass

Link to Devin run: https://app.devin.ai/sessions/8bc1623f365c4c5fab3d83c6c1c7279f
Requested by: anthony.shew@vercel.com

Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner April 30, 2025 04:13
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@vercel
Copy link
Contributor

vercel bot commented Apr 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-gatsby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-svelte-web ❌ Failed (Inspect) May 1, 2025 3:57pm
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 3:57pm

Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
turbo_root: &AbsoluteSystemPath,
stdout: Vec<u8>,
) {
let stdout = String::from_utf8(stdout).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can still panic if there's a non-UTF8 path in the output.

Ok(anchored_to_turbo_root_file_path) => {
files.insert(anchored_to_turbo_root_file_path);
}
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.

devin-ai-integration bot and others added 7 commits April 30, 2025 12:34
…hange_detector

Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
…hange_detector

Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
devin-ai-integration bot and others added 3 commits April 30, 2025 14:15
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
…ct success

Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
})
.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?

.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?

) {
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.

.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.

devin-ai-integration bot and others added 5 commits May 1, 2025 15:34
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
Co-Authored-By: anthony.shew@vercel.com <anthonyshew@gmail.com>
@anthonyshew
Copy link
Contributor

Closing this one. Sorry, Devin, I think I prompted you poorly. Maybe some other time.

@anthonyshew anthonyshew closed this May 2, 2025
@biru-codeastromer
Copy link
Contributor

Hey @anthonyshew I have tried an approach to solve this .Let me know what you think of the new PR in regards to how it handles it.. new PR
Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crates/turborepo-scm/src/git.rs panics in GitHub Actions

4 participants