Skip to content

Commit

Permalink
cp: Use PathBuf instead of String for paths
Browse files Browse the repository at this point in the history
I think this should avoid unnecessarily validating utf-8, and avoids a few allocations.
  • Loading branch information
tmccombs committed Feb 12, 2023
1 parent 816e0d5 commit 853905b
Showing 1 changed file with 16 additions and 16 deletions.
32 changes: 16 additions & 16 deletions src/uu/cp/src/cp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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()) }
}
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -305,6 +305,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(
Expand Down Expand Up @@ -558,7 +559,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()),
)
}

Expand All @@ -579,7 +581,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 {
Expand All @@ -589,12 +591,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 {
Expand Down Expand Up @@ -757,11 +759,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()));
}
};
Expand Down Expand Up @@ -918,9 +920,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());
Expand All @@ -936,7 +936,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
Expand Down

0 comments on commit 853905b

Please sign in to comment.