Skip to content

Commit

Permalink
chore: remove unnecessary result on join_unix_path (#7506)
Browse files Browse the repository at this point in the history
### Description

Realized that this shouldn't ever fail unless we bypass the checks done
in the constructors for both `AbsoluteSystemPath` and
`RelativeUnixPath`. The only failure point is converting the `PathBuf`
to `AbsoluteSystemPathBuf` which itself has [two failure
points](https://github.com/vercel/turbo/blob/main/crates/turborepo-paths/src/absolute_system_path_buf.rs#L208).
- Conversion from stdlib `PathBuf` to `Utf8PathBuf` which only fails if
the [path can't be represented in
utf8](https://docs.rs/camino/latest/src/camino/lib.rs.html#156)
- `AbsoluteSystemPathBuf` constructor which only fails if the input path
isn't absolute

### Testing Instructions

Existing test suite, my reasoning above

Closes TURBO-2428
  • Loading branch information
chris-olszewski committed Feb 23, 2024
1 parent 1edebbb commit 489aea7
Show file tree
Hide file tree
Showing 9 changed files with 26 additions and 28 deletions.
7 changes: 2 additions & 5 deletions crates/turborepo-lib/src/globwatcher/mod.rs
Expand Up @@ -394,15 +394,12 @@ mod test {

for dir in directories.iter() {
let dir = RelativeUnixPathBuf::new(*dir).unwrap();
tmp.join_unix_path(&dir).unwrap().create_dir_all().unwrap();
tmp.join_unix_path(&dir).create_dir_all().unwrap();
}

for file in files.iter() {
let file = RelativeUnixPathBuf::new(*file).unwrap();
tmp.join_unix_path(&file)
.unwrap()
.create_with_contents("")
.unwrap();
tmp.join_unix_path(&file).create_with_contents("").unwrap();
}
}

Expand Down
9 changes: 3 additions & 6 deletions crates/turborepo-lib/src/run/scope/filter.rs
Expand Up @@ -676,12 +676,9 @@ mod test {
.map(|package_path| {
let (_, name) = get_name(package_path);
(
turbo_root
.join_unix_path(
RelativeUnixPathBuf::new(format!("{package_path}/package.json"))
.unwrap(),
)
.unwrap(),
turbo_root.join_unix_path(
RelativeUnixPathBuf::new(format!("{package_path}/package.json")).unwrap(),
),
PackageJson {
name: Some(name.to_string()),
dependencies: dependencies.get(name).map(|v| {
Expand Down
20 changes: 13 additions & 7 deletions crates/turborepo-paths/src/absolute_system_path.rs
Expand Up @@ -232,14 +232,20 @@ impl AbsoluteSystemPath {
self.0.as_str()
}

pub fn join_unix_path(
&self,
unix_path: impl AsRef<RelativeUnixPath>,
) -> Result<AbsoluteSystemPathBuf, PathError> {
pub fn join_unix_path(&self, unix_path: impl AsRef<RelativeUnixPath>) -> AbsoluteSystemPathBuf {
let tail = unix_path.as_ref().to_system_path_buf();
Ok(AbsoluteSystemPathBuf(
self.0.join(tail).as_std_path().clean().try_into()?,
))
AbsoluteSystemPathBuf(
self.0
.join(tail)
.as_std_path()
.clean()
// The unwrap here should never panic as `try_into` will only panic if
// - path isn't absolute: self is already absolute, appending to it won't change
// that
// - path isn't valid utf8: self and unix_path are both utf8 already
.try_into()
.expect("joined path is absolute and valid utf8"),
)
}

pub fn anchor(&self, path: &AbsoluteSystemPath) -> Result<AnchoredSystemPathBuf, PathError> {
Expand Down
6 changes: 2 additions & 4 deletions crates/turborepo-paths/src/absolute_system_path_buf.rs
Expand Up @@ -269,8 +269,7 @@ mod tests {
assert_eq!(
AbsoluteSystemPathBuf::new("/some/dir")
.unwrap()
.join_unix_path(tail)
.unwrap(),
.join_unix_path(tail),
AbsoluteSystemPathBuf::new("/some/other").unwrap(),
);
}
Expand All @@ -297,8 +296,7 @@ mod tests {
assert_eq!(
AbsoluteSystemPathBuf::new("C:\\some\\dir")
.unwrap()
.join_unix_path(&tail)
.unwrap(),
.join_unix_path(&tail),
AbsoluteSystemPathBuf::new("C:\\some\\other").unwrap(),
);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-repository/src/package_graph/builder.rs
Expand Up @@ -640,7 +640,7 @@ impl<'a> DependencyVersion<'a> {
Some("file") | Some("link") => {
// Default to internal if we have the package but somehow cannot get the path
RelativeUnixPathBuf::new(self.version)
.and_then(|file_path| cwd.join_unix_path(file_path))
.map(|file_path| cwd.join_unix_path(file_path))
.map_or(true, |dep_path| root.contains(&dep_path))
}
Some(_) if self.is_external() => {
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-scm/src/git.rs
Expand Up @@ -167,7 +167,7 @@ impl Git {
turbo_root: &AbsoluteSystemPath,
path: &RelativeUnixPath,
) -> Result<AnchoredSystemPathBuf, Error> {
let absolute_file_path = self.root.join_unix_path(path)?;
let absolute_file_path = self.root.join_unix_path(path);
let anchored_to_turbo_root_file_path = turbo_root.anchor(&absolute_file_path)?;
Ok(anchored_to_turbo_root_file_path)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-scm/src/hash_object.rs
Expand Up @@ -15,7 +15,7 @@ pub(crate) fn hash_objects(
let span = tracing::info_span!(parent: &parent, "hash_object", ?filename);
let _enter = span.enter();

let full_file_path = git_root.join_unix_path(filename)?;
let full_file_path = git_root.join_unix_path(filename);
match git2::Oid::hash_file(git2::ObjectType::Blob, &full_file_path) {
Ok(hash) => {
let package_relative_path =
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-scm/src/lib.rs
Expand Up @@ -192,7 +192,7 @@ fn find_git_root(turbo_root: &AbsoluteSystemPath) -> Result<AbsoluteSystemPathBu
if let Some(line) = lines.next() {
let line = String::from_utf8(line?)?;
let tail = RelativeUnixPathBuf::new(line)?;
turbo_root.join_unix_path(tail).map_err(|e| e.into())
Ok(turbo_root.join_unix_path(tail))
} else {
let stderr = String::from_utf8_lossy(&rev_parse.stderr);
Err(Error::git_error(format!(
Expand Down
4 changes: 2 additions & 2 deletions crates/turborepo-scm/src/manual.rs
Expand Up @@ -395,7 +395,7 @@ mod tests {
let mut expected = GitHashes::new();
for (raw_unix_path, contents, expected_hash) in file_hash.iter() {
let unix_path = RelativeUnixPath::new(raw_unix_path).unwrap();
let file_path = turbo_root.join_unix_path(unix_path).unwrap();
let file_path = turbo_root.join_unix_path(unix_path);
file_path.ensure_dir().unwrap();
file_path.create_with_contents(contents).unwrap();
if let Some(hash) = expected_hash {
Expand Down Expand Up @@ -427,7 +427,7 @@ mod tests {
expected = GitHashes::new();
for (raw_unix_path, contents, expected_hash) in file_hash.iter() {
let unix_path = RelativeUnixPath::new(raw_unix_path).unwrap();
let file_path = turbo_root.join_unix_path(unix_path).unwrap();
let file_path = turbo_root.join_unix_path(unix_path);
file_path.ensure_dir().unwrap();
file_path.create_with_contents(contents).unwrap();
if let Some(hash) = expected_hash {
Expand Down

0 comments on commit 489aea7

Please sign in to comment.