From 8a9fb84a8ed9826c8665b6826c025509de941190 Mon Sep 17 00:00:00 2001 From: sreehari prasad <52113972+matrixhead@users.noreply.github.com> Date: Sat, 14 Sep 2024 12:41:17 +0530 Subject: [PATCH] mv: gnu test case mv-n compatibility (#6599) * 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 --- src/uu/cp/src/cp.rs | 37 ++++++++----- src/uu/mv/src/mv.rs | 35 ++++++++++-- src/uucore/src/lib/features/update_control.rs | 4 +- .../src/lib/parser/shortcut_value_parser.rs | 42 ++++++++++++-- tests/by-util/test_cp.rs | 55 ++++++++++++------- tests/by-util/test_mv.rs | 37 +++++++------ 6 files changed, 147 insertions(+), 63 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 152dc8c73a9..06f49520f60 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -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; @@ -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) } @@ -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 } @@ -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()); } @@ -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(()), @@ -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; @@ -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)?; @@ -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, @@ -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 { @@ -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)?; } diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 7edecb960fc..5f1b717834b 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -83,6 +83,9 @@ pub struct Options { /// '-g, --progress' pub progress_bar: bool, + + /// `--debug` + pub debug: bool, } /// specifies behavior of the overwrite flag @@ -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<()> { @@ -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", )); } @@ -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) @@ -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 { @@ -521,6 +536,9 @@ fn rename( } if opts.update == UpdateMode::ReplaceNone { + if opts.debug { + println!("skipped {}", to.quote()); + } return Ok(()); } @@ -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()) { diff --git a/src/uucore/src/lib/features/update_control.rs b/src/uucore/src/lib/features/update_control.rs index 72927c8d06b..34cb8478bcc 100644 --- a/src/uucore/src/lib/features/update_control.rs +++ b/src/uucore/src/lib/features/update_control.rs @@ -59,6 +59,7 @@ pub enum UpdateMode { /// --update=`older` /// -u ReplaceIfOlder, + ReplaceNoneFail, } pub mod arguments { @@ -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) @@ -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) { diff --git a/src/uucore/src/lib/parser/shortcut_value_parser.rs b/src/uucore/src/lib/parser/shortcut_value_parser.rs index fe80bee3cd4..267cf5b756a 100644 --- a/src/uucore/src/lib/parser/shortcut_value_parser.rs +++ b/src/uucore/src/lib/parser/shortcut_value_parser.rs @@ -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` @@ -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); @@ -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; @@ -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)) } } } @@ -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")); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 22160a85f6a..5262d4856da 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -324,18 +324,25 @@ fn test_cp_arg_update_interactive_error() { #[test] fn test_cp_arg_update_none() { - for argument in ["--update=none", "--update=non", "--update=n"] { - let (at, mut ucmd) = at_and_ucmd!(); - - ucmd.arg(TEST_HELLO_WORLD_SOURCE) - .arg(TEST_HOW_ARE_YOU_SOURCE) - .arg(argument) - .succeeds() - .no_stderr() - .no_stdout(); + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_HOW_ARE_YOU_SOURCE) + .arg("--update=none") + .succeeds() + .no_stderr() + .no_stdout(); + assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); +} - assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); - } +#[test] +fn test_cp_arg_update_none_fail() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_HOW_ARE_YOU_SOURCE) + .arg("--update=none-fail") + .fails() + .stderr_contains(format!("not replacing '{}'", TEST_HOW_ARE_YOU_SOURCE)); + assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); } #[test] @@ -588,10 +595,9 @@ fn test_cp_arg_interactive_verbose_clobber() { let (at, mut ucmd) = at_and_ucmd!(); at.touch("a"); at.touch("b"); - ucmd.args(&["-vin", "a", "b"]) - .fails() - .stderr_is("cp: not replacing 'b'\n") - .no_stdout(); + ucmd.args(&["-vin", "--debug", "a", "b"]) + .succeeds() + .stdout_contains("skipped 'b'"); } #[test] @@ -690,8 +696,9 @@ fn test_cp_arg_no_clobber() { ucmd.arg(TEST_HELLO_WORLD_SOURCE) .arg(TEST_HOW_ARE_YOU_SOURCE) .arg("--no-clobber") - .fails() - .stderr_contains("not replacing"); + .arg("--debug") + .succeeds() + .stdout_contains("skipped 'how_are_you.txt'"); assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); } @@ -702,7 +709,9 @@ fn test_cp_arg_no_clobber_inferred_arg() { ucmd.arg(TEST_HELLO_WORLD_SOURCE) .arg(TEST_HOW_ARE_YOU_SOURCE) .arg("--no-clob") - .fails(); + .arg("--debug") + .succeeds() + .stdout_contains("skipped 'how_are_you.txt'"); assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); } @@ -718,6 +727,7 @@ fn test_cp_arg_no_clobber_twice() { .arg("--no-clobber") .arg("source.txt") .arg("dest.txt") + .arg("--debug") .succeeds() .no_stderr(); @@ -729,7 +739,9 @@ fn test_cp_arg_no_clobber_twice() { .arg("--no-clobber") .arg("source.txt") .arg("dest.txt") - .fails(); + .arg("--debug") + .succeeds() + .stdout_contains("skipped 'dest.txt'"); assert_eq!(at.read("source.txt"), "some-content"); // Should be empty as the "no-clobber" should keep @@ -1773,11 +1785,12 @@ fn test_cp_preserve_links_case_7() { ucmd.arg("-n") .arg("--preserve=links") + .arg("--debug") .arg("src/f") .arg("src/g") .arg("dest") - .fails() - .stderr_contains("not replacing"); + .succeeds() + .stdout_contains("skipped"); assert!(at.dir_exists("dest")); assert!(at.plus("dest").join("f").exists()); diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 82741d5f002..7b100fe5c3f 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -299,9 +299,9 @@ fn test_mv_interactive_no_clobber_force_last_arg_wins() { scene .ucmd() - .args(&[file_a, file_b, "-f", "-i", "-n"]) - .fails() - .stderr_is(format!("mv: not replacing '{file_b}'\n")); + .args(&[file_a, file_b, "-f", "-i", "-n", "--debug"]) + .succeeds() + .stdout_contains("skipped 'b.txt'"); scene .ucmd() @@ -352,9 +352,9 @@ fn test_mv_no_clobber() { ucmd.arg("-n") .arg(file_a) .arg(file_b) - .fails() - .code_is(1) - .stderr_only(format!("mv: not replacing '{file_b}'\n")); + .arg("--debug") + .succeeds() + .stdout_contains("skipped 'test_mv_no_clobber_file_b"); assert!(at.file_exists(file_a)); assert!(at.file_exists(file_b)); @@ -863,14 +863,16 @@ fn test_mv_backup_off() { } #[test] -fn test_mv_backup_no_clobber_conflicting_options() { - new_ucmd!() - .arg("--backup") - .arg("--no-clobber") - .arg("file1") - .arg("file2") - .fails() - .usage_error("options --backup and --no-clobber are mutually exclusive"); +fn test_mv_backup_conflicting_options() { + for conflicting_opt in ["--no-clobber", "--update=none-fail", "--update=none"] { + new_ucmd!() + .arg("--backup") + .arg(conflicting_opt) + .arg("file1") + .arg("file2") + .fails() + .usage_error("cannot combine --backup with -n/--no-clobber or --update=none-fail"); + } } #[test] @@ -1400,10 +1402,9 @@ fn test_mv_arg_interactive_skipped_vin() { let (at, mut ucmd) = at_and_ucmd!(); at.touch("a"); at.touch("b"); - ucmd.args(&["-vin", "a", "b"]) - .fails() - .stderr_is("mv: not replacing 'b'\n") - .no_stdout(); + ucmd.args(&["-vin", "a", "b", "--debug"]) + .succeeds() + .stdout_contains("skipped 'b'"); } #[test]