From fc5e2b0d254fafb63aa1a72b407f36165711b004 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 18 Jul 2023 13:16:16 -0700 Subject: [PATCH] chore: no longer convert in relative unix path constructor (#5552) ### 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 --- .../src/cache_archive/create.rs | 2 +- .../src/anchored_system_path.rs | 15 ++++++++++++++- .../src/anchored_system_path_buf.rs | 18 +----------------- .../src/relative_unix_path_buf.rs | 6 ++---- crates/turborepo-scm/src/ls_tree.rs | 2 ++ 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/crates/turborepo-cache/src/cache_archive/create.rs b/crates/turborepo-cache/src/cache_archive/create.rs index dac5aa731e38e..44e6a0c93fa2a 100644 --- a/crates/turborepo-cache/src/cache_archive/create.rs +++ b/crates/turborepo-cache/src/cache_archive/create.rs @@ -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)?; diff --git a/crates/turborepo-paths/src/anchored_system_path.rs b/crates/turborepo-paths/src/anchored_system_path.rs index 34025cd6ec0b3..febad8f96d4fc 100644 --- a/crates/turborepo-paths/src/anchored_system_path.rs +++ b/crates/turborepo-paths/src/anchored_system_path.rs @@ -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); @@ -71,4 +71,17 @@ impl AnchoredSystemPath { pub fn as_path(&self) -> &Path { self.0.as_std_path() } + + pub fn to_unix(&self) -> Result { + #[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) + } + } } diff --git a/crates/turborepo-paths/src/anchored_system_path_buf.rs b/crates/turborepo-paths/src/anchored_system_path_buf.rs index ed10e204492a7..0710468146864 100644 --- a/crates/turborepo-paths/src/anchored_system_path_buf.rs +++ b/crates/turborepo-paths/src/anchored_system_path_buf.rs @@ -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); @@ -192,19 +189,6 @@ impl AnchoredSystemPathBuf { self.0.as_std_path() } - pub fn to_unix(&self) -> Result { - #[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) { self.0.push(path.as_ref()); } diff --git a/crates/turborepo-paths/src/relative_unix_path_buf.rs b/crates/turborepo-paths/src/relative_unix_path_buf.rs index 479120ea12031..c7c33ea480411 100644 --- a/crates/turborepo-paths/src/relative_unix_path_buf.rs +++ b/crates/turborepo-paths/src/relative_unix_path_buf.rs @@ -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)] @@ -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 { diff --git a/crates/turborepo-scm/src/ls_tree.rs b/crates/turborepo-scm/src/ls_tree.rs index 92602bb2d0c1b..2ab382ff9076c 100644 --- a/crates/turborepo-scm/src/ls_tree.rs +++ b/crates/turborepo-scm/src/ls_tree.rs @@ -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 \