Skip to content

Commit

Permalink
chore: no longer convert in relative unix path constructor (#5552)
Browse files Browse the repository at this point in the history
### Description

To quote @gsoltis:
> In general:
> - constructors should validate, to the extent they can (not much for
`unix` paths, but can verify relative)
> - conversions should be explicit. You need to know where you're
starting from. If this were an AnchoredSystemPath on windows, the `\` ->
`/` makes sense. If it's a literal from e.g. a tar file, then it
doesn't.

Reviewers Notes:
- Opening up this PR in VSCode and using `Find All References` on the
constructor is useful for double checking that I didn't miss a
conversion.
- Clippy error appeared on local when I made these changes. Fixed it
just in case that would block CI
- Moved `to_unix` to `AnchoredSystemPath` instead of
`AnchoredSystemPathBuf` now that we have deref coercion which will
automatically convert `&AnchroedSystemPathBuf` to `&AnchoredSystemPath`
and moving the method allows it to be called from either type.

### Testing Instructions

Looked through all uses of `RelativeUnixPathBuf::new` to see if there
were places that depended on the conversion. The only use that was
obvious was the usage in `cache_archive/create.rs`. `dotEnv` was the
only other place where we possibly were converting a system path to a
relative unix. We don't specify that `dotEnv` entries should be unix
relative, so we might've been accidentally supporting system paths, but

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski committed Jul 18, 2023
1 parent 3eb3a5f commit fc5e2b0
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 23 deletions.
2 changes: 1 addition & 1 deletion crates/turborepo-cache/src/cache_archive/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl<'a> CacheWriter<'a> {
let file_info = source_path.symlink_metadata()?;

// Normalize the path within the cache
let mut file_path = RelativeUnixPathBuf::new(file_path.as_str())?;
let mut file_path = file_path.to_unix()?;
file_path.make_canonical_for_tar(file_info.is_dir());

let mut header = Self::create_header(&source_path, &file_info)?;
Expand Down
15 changes: 14 additions & 1 deletion crates/turborepo-paths/src/anchored_system_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{fmt, path::Path};

use camino::{Utf8Component, Utf8Path};

use crate::{AnchoredSystemPathBuf, PathError};
use crate::{AnchoredSystemPathBuf, PathError, RelativeUnixPathBuf};

pub struct AnchoredSystemPath(Utf8Path);

Expand Down Expand Up @@ -71,4 +71,17 @@ impl AnchoredSystemPath {
pub fn as_path(&self) -> &Path {
self.0.as_std_path()
}

pub fn to_unix(&self) -> Result<RelativeUnixPathBuf, PathError> {
#[cfg(unix)]
{
return RelativeUnixPathBuf::new(self.0.as_str());
}
#[cfg(not(unix))]
{
use crate::IntoUnix;
let unix_buf = self.0.into_unix();
RelativeUnixPathBuf::new(unix_buf)
}
}
}
18 changes: 1 addition & 17 deletions crates/turborepo-paths/src/anchored_system_path_buf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ use std::{
use camino::{Utf8Component, Utf8Components, Utf8Path, Utf8PathBuf};
use serde::{Deserialize, Serialize};

use crate::{
check_path, AbsoluteSystemPath, AnchoredSystemPath, PathError, PathValidation,
RelativeUnixPathBuf,
};
use crate::{check_path, AbsoluteSystemPath, AnchoredSystemPath, PathError, PathValidation};

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Default, Serialize, Deserialize)]
pub struct AnchoredSystemPathBuf(pub(crate) Utf8PathBuf);
Expand Down Expand Up @@ -192,19 +189,6 @@ impl AnchoredSystemPathBuf {
self.0.as_std_path()
}

pub fn to_unix(&self) -> Result<RelativeUnixPathBuf, PathError> {
#[cfg(unix)]
{
return RelativeUnixPathBuf::new(self.0.as_str());
}
#[cfg(not(unix))]
{
use crate::IntoUnix;
let unix_buf = self.0.as_path().into_unix();
RelativeUnixPathBuf::new(unix_buf)
}
}

pub fn push(&mut self, path: impl AsRef<Utf8Path>) {
self.0.push(path.as_ref());
}
Expand Down
6 changes: 2 additions & 4 deletions crates/turborepo-paths/src/relative_unix_path_buf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::{
use camino::Utf8Path;
use serde::Serialize;

use crate::{IntoUnix, PathError, RelativeUnixPath};
use crate::{PathError, RelativeUnixPath};

#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default, Serialize)]
#[serde(transparent)]
Expand All @@ -27,9 +27,7 @@ impl RelativeUnixPathBuf {
return Err(PathError::NotRelative(path_string));
}

let unix_path = path_string.into_unix();

Ok(Self(unix_path.into()))
Ok(Self(path_string))
}

pub fn into_inner(self) -> String {
Expand Down
2 changes: 2 additions & 0 deletions crates/turborepo-scm/src/ls_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ mod tests {
&[("package.json", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391")],
),
(
// We aren't attempting to use octal escapes here, it just looks like it
#[allow(clippy::octal_escapes)]
"100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\t\t\000100644 blob \
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\t\"\000100644 blob \
5b999efa470b056e329b4c23a73904e0794bdc2f\t\n\000100644 blob \
Expand Down

0 comments on commit fc5e2b0

Please sign in to comment.