From e71fda3506536d665c52c35740fba0062a3f3132 Mon Sep 17 00:00:00 2001 From: --global Date: Mon, 8 May 2023 15:57:13 -0400 Subject: [PATCH] Converted a lot of call sites to impl AsRef --- Cargo.lock | 1 + crates/turbopath/Cargo.toml | 3 ++ crates/turbopath/src/absolute_system_path.rs | 43 +++++++++++++++++++ .../turbopath/src/absolute_system_path_buf.rs | 9 ++-- .../turbopath/src/anchored_system_path_buf.rs | 7 ++- crates/turbopath/src/lib.rs | 1 + .../turbopath/src/relative_unix_path_buf.rs | 2 - crates/turborepo-ffi/src/lib.rs | 4 +- crates/turborepo-fs/src/lib.rs | 43 +++++++++++++------ crates/turborepo-lib/src/config/repo.rs | 1 - crates/turborepo-scm/src/git.rs | 16 ++++--- 11 files changed, 100 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 88efe38120982..0669d620f4495 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9017,6 +9017,7 @@ dependencies = [ name = "turbopath" version = "0.1.0" dependencies = [ + "anyhow", "path-slash", "serde", "thiserror", diff --git a/crates/turbopath/Cargo.toml b/crates/turbopath/Cargo.toml index b6fb4d764961d..d2f775a0d3580 100644 --- a/crates/turbopath/Cargo.toml +++ b/crates/turbopath/Cargo.toml @@ -11,3 +11,6 @@ path-slash = "0.2.1" # TODO: Make this a crate feature serde = { workspace = true } thiserror = { workspace = true } + +[dev-dependencies] +anyhow = { workspace = true } diff --git a/crates/turbopath/src/absolute_system_path.rs b/crates/turbopath/src/absolute_system_path.rs index 601100899e202..ad7426420a6af 100644 --- a/crates/turbopath/src/absolute_system_path.rs +++ b/crates/turbopath/src/absolute_system_path.rs @@ -29,6 +29,12 @@ impl ToOwned for AbsoluteSystemPath { } } +impl AsRef for AbsoluteSystemPath { + fn as_ref(&self) -> &AbsoluteSystemPath { + self + } +} + impl fmt::Display for AbsoluteSystemPath { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.0.display().fmt(f) @@ -74,6 +80,10 @@ impl AbsoluteSystemPath { AbsoluteSystemPathBuf(path) } + pub fn join_literal(&self, segment: &str) -> AbsoluteSystemPathBuf { + AbsoluteSystemPathBuf(self.0.join(segment)) + } + pub fn join_unix_path( &self, unix_path: &RelativeUnixPath, @@ -131,3 +141,36 @@ impl AbsoluteSystemPath { fs::remove_file(&self.0) } } + +#[cfg(test)] +mod tests { + use anyhow::Result; + + use super::*; + + #[test] + fn test_create_absolute_path() -> Result<()> { + let absolute_path = AbsoluteSystemPath::new("/foo/bar")?; + + #[cfg(unix)] + { + 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()); + } + + Ok(()) + } +} diff --git a/crates/turbopath/src/absolute_system_path_buf.rs b/crates/turbopath/src/absolute_system_path_buf.rs index 387a9baecfdfb..6f4762ff29e0b 100644 --- a/crates/turbopath/src/absolute_system_path_buf.rs +++ b/crates/turbopath/src/absolute_system_path_buf.rs @@ -97,8 +97,11 @@ impl AbsoluteSystemPathBuf { /// assert_eq!(anchored_path.as_path(), Path::new("Documents")); /// } /// ``` - pub fn anchor(&self, path: &AbsoluteSystemPath) -> Result { - AnchoredSystemPathBuf::new(self.borrow(), path) + pub fn anchor( + &self, + path: impl AsRef, + ) -> Result { + AnchoredSystemPathBuf::new(self, path) } /// Resolves `path` with `self` as anchor. @@ -162,7 +165,7 @@ impl AbsoluteSystemPathBuf { } pub fn join_literal(&self, segment: &str) -> Self { - AbsoluteSystemPathBuf(self.0.join(Path::new(segment))) + AbsoluteSystemPathBuf(self.0.join(segment)) } pub fn ensure_dir(&self) -> Result<(), io::Error> { diff --git a/crates/turbopath/src/anchored_system_path_buf.rs b/crates/turbopath/src/anchored_system_path_buf.rs index 56ffcda694181..7a54d3e7922a8 100644 --- a/crates/turbopath/src/anchored_system_path_buf.rs +++ b/crates/turbopath/src/anchored_system_path_buf.rs @@ -20,7 +20,12 @@ impl TryFrom<&Path> for AnchoredSystemPathBuf { } impl AnchoredSystemPathBuf { - pub fn new(root: &AbsoluteSystemPath, path: &AbsoluteSystemPath) -> Result { + pub fn new( + root: impl AsRef, + path: impl AsRef, + ) -> Result { + let root = root.as_ref(); + let path = path.as_ref(); let stripped_path = path .as_path() .strip_prefix(root.as_path()) diff --git a/crates/turbopath/src/lib.rs b/crates/turbopath/src/lib.rs index aad066c5737c2..facb27034a523 100644 --- a/crates/turbopath/src/lib.rs +++ b/crates/turbopath/src/lib.rs @@ -1,4 +1,5 @@ #![feature(assert_matches)] +#![feature(cow_is_borrowed)] mod absolute_system_path; mod absolute_system_path_buf; diff --git a/crates/turbopath/src/relative_unix_path_buf.rs b/crates/turbopath/src/relative_unix_path_buf.rs index 95306ee28b763..1ab8b80e08471 100644 --- a/crates/turbopath/src/relative_unix_path_buf.rs +++ b/crates/turbopath/src/relative_unix_path_buf.rs @@ -42,8 +42,6 @@ impl RelativeUnixPathBuf { #[cfg(test)] mod tests { - use std::path::Path; - use super::*; #[test] diff --git a/crates/turborepo-ffi/src/lib.rs b/crates/turborepo-ffi/src/lib.rs index 48ef433fe6971..f1d34380a0be8 100644 --- a/crates/turborepo-ffi/src/lib.rs +++ b/crates/turborepo-ffi/src/lib.rs @@ -4,7 +4,7 @@ //! and in ffi.go before modifying this file. mod lockfile; -use std::{borrow::Borrow, mem::ManuallyDrop, path::PathBuf}; +use std::{mem::ManuallyDrop, path::PathBuf}; pub use lockfile::{patches, subgraph, transitive_closure}; use turbopath::AbsoluteSystemPathBuf; @@ -155,7 +155,7 @@ pub extern "C" fn recursive_copy(buffer: Buffer) -> Buffer { } }; - let response = match turborepo_fs::recursive_copy(src.borrow(), dst.borrow()) { + let response = match turborepo_fs::recursive_copy(src, dst) { Ok(()) => proto::RecursiveCopyResponse { error: None }, Err(e) => proto::RecursiveCopyResponse { error: Some(e.to_string()), diff --git a/crates/turborepo-fs/src/lib.rs b/crates/turborepo-fs/src/lib.rs index 6046639c260cc..ebe473d1e08e9 100644 --- a/crates/turborepo-fs/src/lib.rs +++ b/crates/turborepo-fs/src/lib.rs @@ -4,7 +4,12 @@ use anyhow::Result; use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; use walkdir::WalkDir; -pub fn recursive_copy(src: &AbsoluteSystemPath, dst: &AbsoluteSystemPath) -> Result<()> { +pub fn recursive_copy( + src: impl AsRef, + dst: impl AsRef, +) -> Result<()> { + let src = src.as_ref(); + let dst = dst.as_ref(); let src_metadata = src.symlink_metadata()?; if src_metadata.is_dir() { let walker = WalkDir::new(src.as_path()).follow_links(false); @@ -44,9 +49,9 @@ pub fn recursive_copy(src: &AbsoluteSystemPath, dst: &AbsoluteSystemPath) -> Res let target = dst.resolve(&suffix); if is_dir_or_symlink_to_dir { let src_metadata = entry.metadata()?; - make_dir_copy(target.as_absolute_path(), &src_metadata)?; + make_dir_copy(&target, &src_metadata)?; } else { - copy_file_with_type(&path, file_type, target.as_absolute_path())?; + copy_file_with_type(&path, file_type, &target)?; } } } @@ -57,7 +62,8 @@ pub fn recursive_copy(src: &AbsoluteSystemPath, dst: &AbsoluteSystemPath) -> Res } } -fn make_dir_copy(dir: &AbsoluteSystemPath, src_metadata: &Metadata) -> Result<()> { +fn make_dir_copy(dir: impl AsRef, src_metadata: &Metadata) -> Result<()> { + let dir = dir.as_ref(); let mut builder = DirBuilder::new(); #[cfg(not(windows))] { @@ -69,16 +75,22 @@ fn make_dir_copy(dir: &AbsoluteSystemPath, src_metadata: &Metadata) -> Result<() Ok(()) } -pub fn copy_file(from: &AbsoluteSystemPath, to: &AbsoluteSystemPath) -> Result<()> { +pub fn copy_file( + from: impl AsRef, + to: impl AsRef, +) -> Result<()> { + let from = from.as_ref(); let metadata = from.symlink_metadata()?; copy_file_with_type(from, metadata.file_type(), to) } fn copy_file_with_type( - from: &AbsoluteSystemPath, + from: impl AsRef, from_type: fs::FileType, - to: &AbsoluteSystemPath, + to: impl AsRef, ) -> Result<()> { + let from = from.as_ref(); + let to = to.as_ref(); if from_type.is_symlink() { let target = from.read_link()?; to.ensure_dir()?; @@ -98,13 +110,13 @@ fn copy_file_with_type( mod tests { use std::{io, path::Path}; - use turbopath::PathError; + use turbopath::{AbsoluteSystemPathBuf, PathError}; use super::*; - fn tmp_dir() -> Result<(tempfile::TempDir, AbsoluteSystemPathBuf)> { + fn tmp_dir<'a>() -> Result<(tempfile::TempDir, AbsoluteSystemPathBuf)> { let tmp_dir = tempfile::tempdir()?; - let dir = AbsoluteSystemPathBuf::new(tmp_dir.path().to_path_buf())?; + let dir = AbsoluteSystemPathBuf::new(tmp_dir.path())?; Ok((tmp_dir, dir)) } @@ -116,7 +128,7 @@ mod tests { let (_dst_tmp, dst_dir) = tmp_dir()?; let dst_file = dst_dir.join_literal("dest"); - let err = copy_file(&src_file, &dst_file).unwrap_err(); + let err = copy_file(src_file, dst_file).unwrap_err(); let err = err.downcast::()?; assert_eq!(err.is_io_error(io::ErrorKind::NotFound), true); Ok(()) @@ -153,7 +165,7 @@ mod tests { src_target.create_with_contents("target")?; src_symlink.symlink_to_file(src_target.as_path())?; - copy_file(src_symlink.as_ref(), dst_file.as_ref())?; + copy_file(&src_symlink, &dst_file)?; assert_target_matches(&dst_file, &src_target); Ok(()) } @@ -238,13 +250,16 @@ mod tests { Ok(()) } - fn assert_file_matches(a: &AbsoluteSystemPath, b: &AbsoluteSystemPath) { + fn assert_file_matches(a: impl AsRef, b: impl AsRef) { + let a = a.as_ref(); + let b = b.as_ref(); let a_contents = fs::read_to_string(a.as_path()).unwrap(); let b_contents = fs::read_to_string(b.as_path()).unwrap(); assert_eq!(a_contents, b_contents); } - fn assert_target_matches>(link: &AbsoluteSystemPath, expected: P) { + fn assert_target_matches(link: impl AsRef, expected: impl AsRef) { + let link = link.as_ref(); let path = link.read_link().unwrap(); assert_eq!(path.as_path(), expected.as_ref()); } diff --git a/crates/turborepo-lib/src/config/repo.rs b/crates/turborepo-lib/src/config/repo.rs index 5da6460794a8f..aea5df650f41d 100644 --- a/crates/turborepo-lib/src/config/repo.rs +++ b/crates/turborepo-lib/src/config/repo.rs @@ -180,7 +180,6 @@ mod test { use std::io::Write; use tempfile::NamedTempFile; - use turbopath::AbsoluteSystemPath; use super::*; diff --git a/crates/turborepo-scm/src/git.rs b/crates/turborepo-scm/src/git.rs index 5c6463d295f5a..79b45ad100948 100644 --- a/crates/turborepo-scm/src/git.rs +++ b/crates/turborepo-scm/src/git.rs @@ -33,7 +33,7 @@ pub fn changed_files( ) -> Result, Error> { let git_root = AbsoluteSystemPathBuf::new(git_root)?; let turbo_root = AbsoluteSystemPathBuf::new(turbo_root)?; - let turbo_root_relative_to_git_root = git_root.anchor(turbo_root.borrow())?; + let turbo_root_relative_to_git_root = git_root.anchor(&turbo_root)?; let pathspec = turbo_root_relative_to_git_root.to_str()?; let mut files = HashSet::new(); @@ -44,7 +44,7 @@ pub fn changed_files( pathspec, )?; - add_files_from_stdout(&mut files, git_root.borrow(), turbo_root.borrow(), output); + add_files_from_stdout(&mut files, &git_root, &turbo_root, output); if let Some(from_commit) = from_commit { let output = execute_git_command( @@ -57,7 +57,7 @@ pub fn changed_files( pathspec, )?; - add_files_from_stdout(&mut files, git_root.borrow(), turbo_root.borrow(), output); + add_files_from_stdout(&mut files, &git_root, &turbo_root, output); } let output = execute_git_command( @@ -66,7 +66,7 @@ pub fn changed_files( pathspec, )?; - add_files_from_stdout(&mut files, git_root.borrow(), turbo_root.borrow(), output); + add_files_from_stdout(&mut files, &git_root, &turbo_root, output); Ok(files) } @@ -99,10 +99,12 @@ fn add_pathspec(command: &mut Command, pathspec: &str) { fn add_files_from_stdout( files: &mut HashSet, - git_root: &AbsoluteSystemPath, - turbo_root: &AbsoluteSystemPath, + git_root: impl AsRef, + turbo_root: impl AsRef, stdout: Vec, ) { + let git_root = git_root.as_ref(); + let turbo_root = turbo_root.as_ref(); let stdout = String::from_utf8(stdout).unwrap(); for line in stdout.lines() { let path = RelativeUnixPath::new(&line).unwrap(); @@ -149,7 +151,7 @@ pub fn previous_content( // Note that we assume any relative file path is relative to the git root let anchored_file_path = if file_path.is_absolute() { let absolute_file_path = AbsoluteSystemPathBuf::new(file_path)?; - git_root.anchor(absolute_file_path.borrow())? + git_root.anchor(&absolute_file_path)? } else { file_path.as_path().try_into()? };