Merge pull request #4348 from tmccombs/cp-pathbuf

cp: Use PathBuf instead of String for paths

src/uu/cp/src/cp.rs

@@ -26,7 +26,7 @@ use std::os::unix::fs::{FileTypeExt, PermissionsExt};
 use std::path::{Path, PathBuf, StripPrefixError};
 use std::string::ToString;
 
-use clap::{crate_version, Arg, ArgAction, ArgMatches, Command};
+use clap::{builder::ValueParser, crate_version, Arg, ArgAction, ArgMatches, Command};
 use filetime::FileTime;
 use indicatif::{ProgressBar, ProgressStyle};
 #[cfg(unix)]
@@ -91,7 +91,7 @@ quick_error! {
  /// Invalid arguments to backup
  Backup(description: String) { display("{}\nTry '{} --help' for more information.", description, uucore::execution_phrase()) }
 
- NotADirectory(path: String) { display("'{}' is not a directory", path) }
+ NotADirectory(path: PathBuf) { display("'{}' is not a directory", path.display()) }
  }
 }
 
@@ -222,7 +222,7 @@ pub struct Options {
  attributes: Attributes,
  recursive: bool,
  backup_suffix: String,
- target_dir: Option<String>,
+ target_dir: Option<PathBuf>,
  update: bool,
  verbose: bool,
  progress_bar: bool,
@@ -302,6 +302,7 @@ pub fn uu_app() -> Command {
  .long(options::TARGET_DIRECTORY)
  .value_name(options::TARGET_DIRECTORY)
  .value_hint(clap::ValueHint::DirPath)
+ .value_parser(ValueParser::path_buf())
  .help("copy all SOURCE arguments into target-directory"),
  )
  .arg(
@@ -555,7 +556,8 @@ pub fn uu_app() -> Command {
  .arg(
  Arg::new(options::PATHS)
  .action(ArgAction::Append)
- .value_hint(clap::ValueHint::AnyPath),
+ .value_hint(clap::ValueHint::AnyPath)
+ .value_parser(ValueParser::path_buf()),
  )
 }
 
@@ -576,7 +578,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
  clap::error::ErrorKind::DisplayVersion => print!("{}", app.render_version()),
  _ => return Err(Box::new(e.with_exit_code(1))),
  };
- } else if let Ok(matches) = matches {
+ } else if let Ok(mut matches) = matches {
  let options = Options::from_matches(&matches)?;
 
  if options.overwrite == OverwriteMode::NoClobber && options.backup != BackupMode::NoBackup {
@@ -586,12 +588,12 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
  ));
  }
 
- let paths: Vec<String> = matches
- .get_many::<String>(options::PATHS)
- .map(|v| v.map(ToString::to_string).collect())
+ let paths: Vec<PathBuf> = matches
+ .remove_many::<PathBuf>(options::PATHS)
+ .map(|v| v.collect())
  .unwrap_or_default();
 
- let (sources, target) = parse_path_args(&paths, &options)?;
+ let (sources, target) = parse_path_args(paths, &options)?;
 
  if let Err(error) = copy(&sources, &target, &options) {
  match error {
@@ -754,11 +756,11 @@ impl Options {
  // Parse target directory options
  let no_target_dir = matches.get_flag(options::NO_TARGET_DIRECTORY);
  let target_dir = matches
- .get_one::<String>(options::TARGET_DIRECTORY)
- .map(ToString::to_string);
+ .get_one::<PathBuf>(options::TARGET_DIRECTORY)
+ .cloned();
 
  if let Some(dir) = &target_dir {
- if !Path::new(dir).is_dir() {
+ if !dir.is_dir() {
  return Err(Error::NotADirectory(dir.clone()));
  }
  };
@@ -915,9 +917,7 @@ impl TargetType {
 }
 
 /// Returns tuple of (Source paths, Target)
-fn parse_path_args(path_args: &[String], options: &Options) -> CopyResult<(Vec<Source>, Target)> {
- let mut paths = path_args.iter().map(PathBuf::from).collect::<Vec<_>>();
-
+fn parse_path_args(mut paths: Vec<Source>, options: &Options) -> CopyResult<(Vec<Source>, Target)> {
  if paths.is_empty() {
  // No files specified
  return Err("missing file operand".into());
@@ -933,7 +933,7 @@ fn parse_path_args(path_args: &[String], options: &Options) -> CopyResult<(Vec<S
  Some(ref target) => {
  // All path args are sources, and the target dir was
  // specified separately
- PathBuf::from(target)
+ target.clone()
  }
  None => {
  // If there was no explicit target-dir, then use the last

tests/by-util/test_chown.rs

@@ -662,10 +662,10 @@ fn test_chown_recursive() {
 
  at.mkdir_all("a/b/c");
  at.mkdir("z");
- at.touch(&at.plus_as_string("a/a"));
- at.touch(&at.plus_as_string("a/b/b"));
- at.touch(&at.plus_as_string("a/b/c/c"));
- at.touch(&at.plus_as_string("z/y"));
+ at.touch(at.plus_as_string("a/a"));
+ at.touch(at.plus_as_string("a/b/b"));
+ at.touch(at.plus_as_string("a/b/c/c"));
+ at.touch(at.plus_as_string("z/y"));
 
  let result = scene
  .ucmd()

tests/by-util/test_chroot.rs

@@ -39,7 +39,7 @@ fn test_enter_chroot_fails() {
 fn test_no_such_directory() {
  let (at, mut ucmd) = at_and_ucmd!();
 
- at.touch(&at.plus_as_string("a"));
+ at.touch(at.plus_as_string("a"));
 
  ucmd.arg("a")
  .fails()

tests/by-util/test_cp.rs

@@ -15,11 +15,15 @@ use std::os::unix::fs::PermissionsExt;
 use std::os::windows::fs::symlink_file;
 #[cfg(not(windows))]
 use std::path::Path;
+#[cfg(target_os = "linux")]
+use std::path::PathBuf;
 
 #[cfg(any(target_os = "linux", target_os = "android"))]
 use filetime::FileTime;
 #[cfg(any(target_os = "linux", target_os = "android"))]
 use rlimit::Resource;
+#[cfg(target_os = "linux")]
+use std::ffi::OsString;
 #[cfg(any(target_os = "linux", target_os = "android"))]
 use std::fs as std_fs;
 use std::thread::sleep;
@@ -91,7 +95,7 @@ fn test_cp_existing_target() {
  assert_eq!(at.read(TEST_EXISTING_FILE), "Hello, World!\n");
 
  // No backup should have been created
- assert!(!at.file_exists(&format!("{TEST_EXISTING_FILE}~")));
+ assert!(!at.file_exists(format!("{TEST_EXISTING_FILE}~")));
 }
 
 #[test]
@@ -636,7 +640,7 @@ fn test_cp_backup_none() {
  .no_stderr();
 
  assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "Hello, World!\n");
- assert!(!at.file_exists(&format!("{TEST_HOW_ARE_YOU_SOURCE}~")));
+ assert!(!at.file_exists(format!("{TEST_HOW_ARE_YOU_SOURCE}~")));
 }
 
 #[test]
@@ -650,7 +654,7 @@ fn test_cp_backup_off() {
  .no_stderr();
 
  assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "Hello, World!\n");
- assert!(!at.file_exists(&format!("{TEST_HOW_ARE_YOU_SOURCE}~")));
+ assert!(!at.file_exists(format!("{TEST_HOW_ARE_YOU_SOURCE}~")));
 }
 
 #[test]
@@ -700,7 +704,7 @@ fn test_cp_deref() {
  .join(TEST_HELLO_WORLD_SOURCE_SYMLINK);
  // unlike -P/--no-deref, we expect a file, not a link
  assert!(at.file_exists(
- &path_to_new_symlink
+ path_to_new_symlink
  .clone()
  .into_os_string()
  .into_string()
@@ -1062,7 +1066,7 @@ fn test_cp_deref_folder_to_folder() {
  .join(TEST_COPY_TO_FOLDER_NEW)
  .join(TEST_HELLO_WORLD_SOURCE_SYMLINK);
  assert!(at.file_exists(
- &path_to_new_symlink
+ path_to_new_symlink
  .clone()
  .into_os_string()
  .into_string()
@@ -1225,8 +1229,8 @@ fn test_cp_archive_recursive() {
  let file_2 = at.subdir.join(TEST_COPY_TO_FOLDER).join("2");
  let file_2_link = at.subdir.join(TEST_COPY_TO_FOLDER).join("2.link");
 
- at.touch(&file_1.to_string_lossy());
- at.touch(&file_2.to_string_lossy());
+ at.touch(file_1);
+ at.touch(file_2);
 
  at.symlink_file("1", &file_1_link.to_string_lossy());
  at.symlink_file("2", &file_2_link.to_string_lossy());
@@ -1252,18 +1256,8 @@ fn test_cp_archive_recursive() {
  .run();
 
  println!("ls dest {}", result.stdout_str());
- assert!(at.file_exists(
- &at.subdir
- .join(TEST_COPY_TO_FOLDER_NEW)
- .join("1")
- .to_string_lossy()
- ));
- assert!(at.file_exists(
- &at.subdir
- .join(TEST_COPY_TO_FOLDER_NEW)
- .join("2")
- .to_string_lossy()
- ));
+ assert!(at.file_exists(at.subdir.join(TEST_COPY_TO_FOLDER_NEW).join("1")));
+ assert!(at.file_exists(at.subdir.join(TEST_COPY_TO_FOLDER_NEW).join("2")));
 
  assert!(at.is_symlink(
  &at.subdir
@@ -1672,7 +1666,7 @@ fn test_cp_reflink_always_override() {
  let dst_path: &str = &vec![MOUNTPOINT, USERDIR, "dst"].concat();
 
  scene.fixtures.mkdir(ROOTDIR);
- scene.fixtures.mkdir(&vec![ROOTDIR, USERDIR].concat());
+ scene.fixtures.mkdir(vec![ROOTDIR, USERDIR].concat());
 
  // Setup:
  // Because neither `mkfs.btrfs` not btrfs `mount` options allow us to have a mountpoint owned
@@ -2536,6 +2530,54 @@ fn test_src_base_dot() {
  assert!(!at.dir_exists("y/x"));
 }
 
+#[cfg(target_os = "linux")]
+fn non_utf8_name(suffix: &str) -> OsString {
+ use std::os::unix::ffi::OsStringExt;
+ let mut name = OsString::from_vec(vec![0xff, 0xff]);
+ name.push(suffix);
+ name
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_non_utf8_src() {
+ let (at, mut ucmd) = at_and_ucmd!();
+ let src = non_utf8_name("src");
+ std::fs::File::create(at.plus(&src)).unwrap();
+ ucmd.args(&[src, "dest".into()])
+ .succeeds()
+ .no_stderr()
+ .no_stdout();
+ assert!(at.file_exists("dest"));
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_non_utf8_dest() {
+ let (at, mut ucmd) = at_and_ucmd!();
+ let dest = non_utf8_name("dest");
+ ucmd.args(&[TEST_HELLO_WORLD_SOURCE.as_ref(), &*dest])
+ .succeeds()
+ .no_stderr()
+ .no_stdout();
+ assert!(at.file_exists(dest));
+}
+
+#[cfg(target_os = "linux")]
+#[test]
+fn test_non_utf8_target() {
+ let (at, mut ucmd) = at_and_ucmd!();
+ let dest = non_utf8_name("dest");
+ at.mkdir(&dest);
+ ucmd.args(&["-t".as_ref(), &*dest, TEST_HELLO_WORLD_SOURCE.as_ref()])
+ .succeeds()
+ .no_stderr()
+ .no_stdout();
+ let mut copied_file = PathBuf::from(dest);
+ copied_file.push(TEST_HELLO_WORLD_SOURCE);
+ assert!(at.file_exists(copied_file));
+}
+
 #[test]
 #[cfg(not(windows))]
 fn test_cp_archive_on_directory_ending_dot() {