Skip to content

Commit

Permalink
mv: gnu test case mv-n compatibility (uutils#6599)
Browse files Browse the repository at this point in the history 
* uucore: add update control `none-fail`

* uucore: show suggestion when parse errors occurs because of an ambiguous value

* added tests for fail-none and ambiguous parse error

* uucore: ambiguous value code refractor

* cp: no-clobber fail silently and outputs skipped message in debug

* mv: add --debug support

* minor changes

---------

Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>
  • Loading branch information
@matrixhead @sylvestre
matrixhead and sylvestre authored Sep 14, 2024
1 parent db40287 commit 8a9fb84
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 63 deletions.
37 changes: 23 additions & 14 deletions src/uu/cp/src/cp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use uucore::{backup_control, update_control};
pub use uucore::{backup_control::BackupMode, update_control::UpdateMode};
use uucore::{
format_usage, help_about, help_section, help_usage, prompt_yes,
shortcut_value_parser::ShortcutValueParser, show_error, show_warning, util_name,
shortcut_value_parser::ShortcutValueParser, show_error, show_warning,
};

use crate::copydir::copy_directory;
Expand Down Expand Up @@ -79,8 +79,10 @@ quick_error! {
StripPrefixError(err: StripPrefixError) { from() }

/// Result of a skipped file
/// Currently happens when "no" is selected in interactive mode
Skipped { }
/// Currently happens when "no" is selected in interactive mode or when
/// `no-clobber` flag is set and destination is already present.
/// `exit with error` is used to determine which exit code should be returned.
Skipped(exit_with_error:bool) { }

/// Result of a skipped file
InvalidArgument(description: String) { display("{}", description) }
Expand Down Expand Up @@ -1210,7 +1212,7 @@ fn show_error_if_needed(error: &Error) {
Error::NotAllFilesCopied => {
// Need to return an error code
}
Error::Skipped => {
Error::Skipped(_) => {
// touch a b && echo "n"|cp -i a b && echo $?
// should return an error from GNU 9.2
}
Expand Down Expand Up @@ -1295,7 +1297,9 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult
&mut copied_files,
) {
show_error_if_needed(&error);
non_fatal_errors = true;
if !matches!(error, Error::Skipped(false)) {
non_fatal_errors = true;
}
} else {
copied_destinations.insert(dest.clone());
}
Expand Down Expand Up @@ -1397,17 +1401,19 @@ fn copy_source(
}

impl OverwriteMode {
fn verify(&self, path: &Path) -> CopyResult<()> {
fn verify(&self, path: &Path, debug: bool) -> CopyResult<()> {
match *self {
Self::NoClobber => {
eprintln!("{}: not replacing {}", util_name(), path.quote());
Err(Error::NotAllFilesCopied)
if debug {
println!("skipped {}", path.quote());
}
Err(Error::Skipped(false))
}
Self::Interactive(_) => {
if prompt_yes!("overwrite {}?", path.quote()) {
Ok(())
} else {
Err(Error::Skipped)
Err(Error::Skipped(true))
}
}
Self::Clobber(_) => Ok(()),
Expand Down Expand Up @@ -1654,7 +1660,7 @@ fn handle_existing_dest(
}

if options.update != UpdateMode::ReplaceIfOlder {
options.overwrite.verify(dest)?;
options.overwrite.verify(dest, options.debug)?;
}

let mut is_dest_removed = false;
Expand Down Expand Up @@ -1892,6 +1898,9 @@ fn handle_copy_mode(

return Ok(());
}
update_control::UpdateMode::ReplaceNoneFail => {
return Err(Error::Error(format!("not replacing '{}'", dest.display())));
}
update_control::UpdateMode::ReplaceIfOlder => {
let dest_metadata = fs::symlink_metadata(dest)?;

Expand All @@ -1900,7 +1909,7 @@ fn handle_copy_mode(
if src_time <= dest_time {
return Ok(());
} else {
options.overwrite.verify(dest)?;
options.overwrite.verify(dest, options.debug)?;

copy_helper(
source,
Expand Down Expand Up @@ -2262,7 +2271,7 @@ fn copy_helper(
File::create(dest).context(dest.display().to_string())?;
} else if source_is_fifo && options.recursive && !options.copy_contents {
#[cfg(unix)]
copy_fifo(dest, options.overwrite)?;
copy_fifo(dest, options.overwrite, options.debug)?;
} else if source_is_symlink {
copy_link(source, dest, symlinked_files)?;
} else {
Expand All @@ -2287,9 +2296,9 @@ fn copy_helper(
// "Copies" a FIFO by creating a new one. This workaround is because Rust's
// built-in fs::copy does not handle FIFOs (see rust-lang/rust/issues/79390).
#[cfg(unix)]
fn copy_fifo(dest: &Path, overwrite: OverwriteMode) -> CopyResult<()> {
fn copy_fifo(dest: &Path, overwrite: OverwriteMode, debug: bool) -> CopyResult<()> {
if dest.exists() {
overwrite.verify(dest)?;
overwrite.verify(dest, debug)?;
fs::remove_file(dest)?;
}

Expand Down
35 changes: 30 additions & 5 deletions src/uu/mv/src/mv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ pub struct Options {

/// '-g, --progress'
pub progress_bar: bool,

/// `--debug`
pub debug: bool,
}

/// specifies behavior of the overwrite flag
Expand All @@ -109,6 +112,7 @@ static OPT_NO_TARGET_DIRECTORY: &str = "no-target-directory";
static OPT_VERBOSE: &str = "verbose";
static OPT_PROGRESS: &str = "progress";
static ARG_FILES: &str = "files";
static OPT_DEBUG: &str = "debug";

#[uucore::main]
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
Expand All @@ -135,10 +139,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let backup_mode = backup_control::determine_backup_mode(&matches)?;
let update_mode = update_control::determine_update_mode(&matches);

if overwrite_mode == OverwriteMode::NoClobber && backup_mode != BackupMode::NoBackup {
if backup_mode != BackupMode::NoBackup
&& (overwrite_mode == OverwriteMode::NoClobber
|| update_mode == UpdateMode::ReplaceNone
|| update_mode == UpdateMode::ReplaceNoneFail)
{
return Err(UUsageError::new(
1,
"options --backup and --no-clobber are mutually exclusive",
"cannot combine --backup with -n/--no-clobber or --update=none-fail",
));
}

Expand All @@ -161,9 +169,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
update: update_mode,
target_dir,
no_target_dir: matches.get_flag(OPT_NO_TARGET_DIRECTORY),
verbose: matches.get_flag(OPT_VERBOSE),
verbose: matches.get_flag(OPT_VERBOSE) || matches.get_flag(OPT_DEBUG),
strip_slashes: matches.get_flag(OPT_STRIP_TRAILING_SLASHES),
progress_bar: matches.get_flag(OPT_PROGRESS),
debug: matches.get_flag(OPT_DEBUG),
};

mv(&files[..], &opts)
Expand Down Expand Up @@ -256,6 +265,12 @@ pub fn uu_app() -> Command {
.value_parser(ValueParser::os_string())
.value_hint(clap::ValueHint::AnyPath),
)
.arg(
Arg::new(OPT_DEBUG)
.long(OPT_DEBUG)
.help("explain how a file is copied. Implies -v")
.action(ArgAction::SetTrue),
)
}

fn determine_overwrite_mode(matches: &ArgMatches) -> OverwriteMode {
Expand Down Expand Up @@ -521,6 +536,9 @@ fn rename(
}

if opts.update == UpdateMode::ReplaceNone {
if opts.debug {
println!("skipped {}", to.quote());
}
return Ok(());
}

Expand All @@ -530,10 +548,17 @@ fn rename(
return Ok(());
}

if opts.update == UpdateMode::ReplaceNoneFail {
let err_msg = format!("not replacing {}", to.quote());
return Err(io::Error::new(io::ErrorKind::Other, err_msg));
}

match opts.overwrite {
OverwriteMode::NoClobber => {
let err_msg = format!("not replacing {}", to.quote());
return Err(io::Error::new(io::ErrorKind::Other, err_msg));
if opts.debug {
println!("skipped {}", to.quote());
}
return Ok(());
}
OverwriteMode::Interactive => {
if !prompt_yes!("overwrite {}?", to.quote()) {
Expand Down
4 changes: 3 additions & 1 deletion src/uucore/src/lib/features/update_control.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ pub enum UpdateMode {
/// --update=`older`
/// -u
ReplaceIfOlder,
ReplaceNoneFail,
}

pub mod arguments {
Expand All @@ -76,7 +77,7 @@ pub mod arguments {
clap::Arg::new(OPT_UPDATE)
.long("update")
.help("move only when the SOURCE file is newer than the destination file or when the destination file is missing")
.value_parser(ShortcutValueParser::new(["none", "all", "older"]))
.value_parser(ShortcutValueParser::new(["none", "all", "older","none-fail"]))
.num_args(0..=1)
.default_missing_value("older")
.require_equals(true)
Expand Down Expand Up @@ -130,6 +131,7 @@ pub fn determine_update_mode(matches: &ArgMatches) -> UpdateMode {
"all" => UpdateMode::ReplaceAll,
"none" => UpdateMode::ReplaceNone,
"older" => UpdateMode::ReplaceIfOlder,
"none-fail" => UpdateMode::ReplaceNoneFail,
_ => unreachable!("other args restricted by clap"),
}
} else if matches.get_flag(arguments::OPT_UPDATE_NO_ARG) {
Expand Down
42 changes: 38 additions & 4 deletions src/uucore/src/lib/parser/shortcut_value_parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.
// spell-checker:ignore abcdefgh abef
// spell-checker:ignore abcdefgh abef Strs

//! A parser that accepts shortcuts for values.
//! `ShortcutValueParser` is similar to clap's `PossibleValuesParser`
Expand Down Expand Up @@ -32,6 +32,7 @@ impl ShortcutValueParser {
cmd: &clap::Command,
arg: Option<&clap::Arg>,
value: &str,
possible_values: &[&PossibleValue],
) -> clap::Error {
let mut err = clap::Error::new(ErrorKind::InvalidValue).with_cmd(cmd);

Expand All @@ -52,10 +53,39 @@ impl ShortcutValueParser {
ContextValue::Strings(self.0.iter().map(|x| x.get_name().to_string()).collect()),
);

// if `possible_values` is not empty then that means this error is because of an ambiguous value.
if !possible_values.is_empty() {
add_ambiguous_value_tip(possible_values, &mut err, value);
}
err
}
}

/// Adds a suggestion when error is because of ambiguous values based on the provided possible values.
fn add_ambiguous_value_tip(
possible_values: &[&PossibleValue],
err: &mut clap::error::Error,
value: &str,
) {
let mut formatted_possible_values = String::new();
for (i, s) in possible_values.iter().enumerate() {
formatted_possible_values.push_str(&format!("'{}'", s.get_name()));
if i < possible_values.len() - 2 {
formatted_possible_values.push_str(", ");
} else if i < possible_values.len() - 1 {
formatted_possible_values.push_str(" or ");
}
}
err.insert(
ContextKind::Suggested,
ContextValue::StyledStrs(vec![format!(
"It looks like '{}' could match several values. Did you mean {}?",
value, formatted_possible_values
)
.into()]),
);
}

impl TypedValueParser for ShortcutValueParser {
type Value = String;

Expand All @@ -76,13 +106,13 @@ impl TypedValueParser for ShortcutValueParser {
.collect();

match matched_values.len() {
0 => Err(self.generate_clap_error(cmd, arg, value)),
0 => Err(self.generate_clap_error(cmd, arg, value, &[])),
1 => Ok(matched_values[0].get_name().to_string()),
_ => {
if let Some(direct_match) = matched_values.iter().find(|x| x.get_name() == value) {
Ok(direct_match.get_name().to_string())
} else {
Err(self.generate_clap_error(cmd, arg, value))
Err(self.generate_clap_error(cmd, arg, value, &matched_values))
}
}
}
Expand Down Expand Up @@ -143,7 +173,11 @@ mod tests {

for ambiguous_value in ambiguous_values {
let result = parser.parse_ref(&cmd, None, OsStr::new(ambiguous_value));
assert_eq!(ErrorKind::InvalidValue, result.unwrap_err().kind());
assert_eq!(ErrorKind::InvalidValue, result.as_ref().unwrap_err().kind());
assert!(result.unwrap_err().to_string().contains(&format!(
"It looks like '{}' could match several values. Did you mean 'abcd' or 'abef'?",
ambiguous_value
)));
}

let result = parser.parse_ref(&cmd, None, OsStr::new("abc"));
Expand Down
Loading

0 comments on commit 8a9fb84

Please sign in to comment.