From 4eb1e847e984f6293e0d919109a1f5cdd16b2f7a Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 31 May 2023 23:53:21 +0200 Subject: [PATCH 1/2] cp --no-clobber should fail --- src/uu/cp/src/cp.rs | 15 ++++++--------- tests/by-util/test_cp.rs | 6 +++--- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 513fb8380..e9fb56ef3 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1102,23 +1102,21 @@ fn preserve_hardlinks( } /// When handling errors, we don't always want to show them to the user. This function handles that. -/// If the error is printed, returns true, false otherwise. -fn show_error_if_needed(error: &Error) -> bool { +fn show_error_if_needed(error: &Error) { match error { // When using --no-clobber, we don't want to show // an error message - Error::NotAllFilesCopied => (), + Error::NotAllFilesCopied => { + // Need to return an error code + } Error::Skipped => { // touch a b && echo "n"|cp -i a b && echo $? // should return an error from GNU 9.2 - return true; } _ => { show_error!("{}", error); - return true; } } - false } /// Copy all `sources` to `target`. Returns an @@ -1175,9 +1173,8 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu options, &mut symlinked_files, ) { - if show_error_if_needed(&error) { - non_fatal_errors = true; - } + show_error_if_needed(&error); + non_fatal_errors = true; } } seen_sources.insert(source); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 1feeb0ede..690aa8b4e 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -487,7 +487,7 @@ fn test_cp_arg_no_clobber() { ucmd.arg(TEST_HELLO_WORLD_SOURCE) .arg(TEST_HOW_ARE_YOU_SOURCE) .arg("--no-clobber") - .succeeds(); + .fails(); assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); } @@ -498,7 +498,7 @@ fn test_cp_arg_no_clobber_inferred_arg() { ucmd.arg(TEST_HELLO_WORLD_SOURCE) .arg(TEST_HOW_ARE_YOU_SOURCE) .arg("--no-clob") - .succeeds(); + .fails(); assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); } @@ -525,7 +525,7 @@ fn test_cp_arg_no_clobber_twice() { .arg("--no-clobber") .arg("source.txt") .arg("dest.txt") - .succeeds(); + .fails(); assert_eq!(at.read("source.txt"), "some-content"); // Should be empty as the "no-clobber" should keep From 20ce7accf250b07a4e0fe2f1bfe29f6363581cd6 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 1 Jun 2023 00:23:34 +0200 Subject: [PATCH 2/2] cp: -i prompts in the right place Should fix tests/cp/cp-i.sh --- src/uu/cp/src/cp.rs | 25 ++++++++++++++++++------- tests/by-util/test_cp.rs | 26 +++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index e9fb56ef3..e8552a179 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -43,7 +43,8 @@ use uucore::fs::{ }; use uucore::update_control::{self, UpdateMode}; use uucore::{ - crash, format_usage, help_about, help_section, help_usage, prompt_yes, show_error, show_warning, + crash, format_usage, help_about, help_section, help_usage, prompt_yes, show_error, + show_warning, util_name, }; use crate::copydir::copy_directory; @@ -1251,13 +1252,23 @@ fn copy_source( } impl OverwriteMode { - fn verify(&self, path: &Path) -> CopyResult<()> { + fn verify(&self, path: &Path, verbose: bool) -> CopyResult<()> { match *self { - Self::NoClobber => Err(Error::NotAllFilesCopied), + Self::NoClobber => { + if verbose { + println!("skipped {}", path.quote()); + } else { + eprintln!("{}: not replacing {}", util_name(), path.quote()); + } + Err(Error::NotAllFilesCopied) + } Self::Interactive(_) => { if prompt_yes!("overwrite {}?", path.quote()) { Ok(()) } else { + if verbose { + println!("skipped {}", path.quote()); + } Err(Error::Skipped) } } @@ -1465,7 +1476,7 @@ fn handle_existing_dest( return Err(format!("{} and {} are the same file", source.quote(), dest.quote()).into()); } - options.overwrite.verify(dest)?; + options.overwrite.verify(dest, options.verbose)?; let backup_path = backup_control::get_backup_path(options.backup, dest, &options.backup_suffix); if let Some(backup_path) = backup_path { @@ -1823,7 +1834,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.verbose)?; } else if source_is_symlink { copy_link(source, dest, symlinked_files)?; } else { @@ -1848,9 +1859,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, verbose: bool) -> CopyResult<()> { if dest.exists() { - overwrite.verify(dest)?; + overwrite.verify(dest, verbose)?; fs::remove_file(dest)?; } diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 690aa8b4e..7f153343d 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -456,6 +456,29 @@ fn test_cp_arg_interactive_update() { .no_stdout(); } +#[test] +#[cfg(not(target_os = "android"))] +fn test_cp_arg_interactive_verbose() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("a"); + at.touch("b"); + ucmd.args(&["-vi", "a", "b"]) + .pipe_in("N\n") + .fails() + .stdout_is("skipped 'b'\n"); +} + +#[test] +#[cfg(not(target_os = "android"))] +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() + .stdout_is("skipped 'b'\n"); +} + #[test] #[cfg(target_os = "linux")] fn test_cp_arg_link() { @@ -487,7 +510,8 @@ fn test_cp_arg_no_clobber() { ucmd.arg(TEST_HELLO_WORLD_SOURCE) .arg(TEST_HOW_ARE_YOU_SOURCE) .arg("--no-clobber") - .fails(); + .fails() + .stderr_contains("not replacing"); assert_eq!(at.read(TEST_HOW_ARE_YOU_SOURCE), "How are you?\n"); }