PR feedback

crates/turbopath/src/absolute_system_path.rs

@@ -48,7 +48,36 @@ impl AsRef<Path> for AbsoluteSystemPath {
 }
 
 impl AbsoluteSystemPath {
-    pub fn new<P: AsRef<Path> + ?Sized>(value: &P) -> Result<Cow<Self>, PathError> {
+    /// Creates a path that is known to be absolute and a system path.
+    /// If either of these conditions are not met, we error.
+    /// Does *not* do automatic conversion like `AbsoluteSystemPathBuf::new`
+    /// does
+    ///
+    /// # Arguments
+    ///
+    /// * `value`: The path to convert to an absolute system path
+    ///
+    /// returns: Result<&AbsoluteSystemPath, PathError>
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use turbopath::AbsoluteSystemPath;
+    /// #[cfg(unix)]
+    /// {
+    ///   assert!(AbsoluteSystemPath::new("/foo/bar").is_ok());
+    ///   assert!(AbsoluteSystemPath::new("foo/bar").is_err());
+    ///   assert!(AbsoluteSystemPath::new("C:\\foo\\bar").is_err());
+    /// }
+    ///
+    /// #[cfg(windows)]
+    /// {
+    ///   assert!(AbsoluteSystemPath::new("C:\\foo\\bar").is_ok());
+    ///   assert!(AbsoluteSystemPath::new("foo\\bar").is_err());
+    ///   assert!(AbsoluteSystemPath::new("/foo/bar").is_err());
+    /// }
+    /// ```
+    pub fn new<P: AsRef<Path> + ?Sized>(value: &P) -> Result<&Self, PathError> {
         let path = value.as_ref();
         if path.is_relative() {
             return Err(PathValidationError::NotAbsolute(path.to_owned()).into());
@@ -59,15 +88,17 @@ impl AbsoluteSystemPath {
 
         let system_path = Cow::from_slash(path_str);
 
-        // copied from stdlib path.rs: relies on the representation of
-        // AbsoluteSystemPath being just a Path, the same way Path relies on
-        // just being an OsStr
         match system_path {
-            Cow::Owned(path) => Ok(Cow::Owned(AbsoluteSystemPathBuf::new(path)?)),
+            Cow::Owned(path) => {
+                Err(PathValidationError::NotSystem(path.to_string_lossy().to_string()).into())
+            }
             Cow::Borrowed(path) => {
                 let path = Path::new(path);
+                // copied from stdlib path.rs: relies on the representation of
+                // AbsoluteSystemPath being just a Path, the same way Path relies on
+                // just being an OsStr
                 let absolute_system_path = unsafe { &*(path as *const Path as *const Self) };
-                Ok(Cow::Borrowed(absolute_system_path))
+                Ok(absolute_system_path)
             }
         }
     }
@@ -151,25 +182,16 @@ mod tests {
 
     #[test]
     fn test_create_absolute_path() -> Result<()> {
-        let absolute_path = AbsoluteSystemPath::new("/foo/bar")?;
-
         #[cfg(unix)]
         {
+            let absolute_path = AbsoluteSystemPath::new("/foo/bar")?;
             assert_eq!(absolute_path.to_string(), "/foo/bar");
-            assert!(absolute_path.is_borrowed());
-        }
-
-        #[cfg(windows)]
-        {
-            assert_eq!(absolute_path.to_string(), r"\foo\bar");
-            assert!(absolute_path.is_owned());
         }
 
         #[cfg(windows)]
         {
-            let absolute_path = AbsoluteSystemPath::new(r"\foo\bar")?;
-            assert_eq!(absolute_path.to_string(), r"\foo\bar");
-            assert!(absolute_path.is_borrowed());
+            let absolute_path = AbsoluteSystemPath::new(r"C:\foo\bar")?;
+            assert_eq!(absolute_path.to_string(), r"C:\foo\bar");
         }
 
         Ok(())

crates/turbopath/src/lib.rs

@@ -51,6 +51,8 @@ pub enum PathValidationError {
     NotParent(String, String),
     #[error("Path {0} is not a unix path")]
     NotUnix(String),
+    #[error("Path {0} is not a system path")]
+    NotSystem(String),
 }
 
 trait IntoSystem {

crates/turborepo-fs/src/lib.rs

@@ -35,7 +35,7 @@ pub fn recursive_copy(
                     let is_dir_or_symlink_to_dir = if file_type.is_dir() {
                         true
                     } else if file_type.is_symlink() {
-                        if let Ok(metadata) = path.as_ref().stat() {
+                        if let Ok(metadata) = path.stat() {
                             metadata.is_dir()
                         } else {
                             // If we have a broken link, skip this entry