From 7827b1fb878300ceacee5d0b5d3e1bdfe5345d07 Mon Sep 17 00:00:00 2001 From: David Matos Date: Fri, 6 Oct 2023 18:59:46 +0200 Subject: [PATCH] ucp: Change error when directory is specified but not recursive (#10609) # Description Closes #10537. Basically error message was unhelpful, and this temporary measure adds back the nice previous nushell error message. Ideally, we would like to add a more permanent solution mentioned in the issue [comments](https://github.com/nushell/nushell/issues/10537#issuecomment-1743686122), but since we want to have `ucp` as `cp` on new release, this is hackier but way simpler so this fix should do it. Only downside is that now behavior differs from `uutils` in the sense that: ``` uutils: > cp a foo/ bar ls bar # foo/a nushell: >ucp a foo/ bar # directory error (not copied) .... ``` So, since its non fatal error, uutils copies a, but nushell errors out with nothing copied. If we go to option 3 mentioned above, then we can decide what we want to do, and perhaps continue on a non fatal error. # User-Facing Changes # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - [X] `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - [X] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - [X] `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - [X] `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` # After Submitting --------- Co-authored-by: amtoine --- crates/nu-command/src/filesystem/ucp.rs | 11 +++++++++++ crates/nu-command/tests/commands/ucp.rs | 11 ++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/nu-command/src/filesystem/ucp.rs b/crates/nu-command/src/filesystem/ucp.rs index a8603295ed..b33480de2d 100644 --- a/crates/nu-command/src/filesystem/ucp.rs +++ b/crates/nu-command/src/filesystem/ucp.rs @@ -166,6 +166,17 @@ impl Command for UCp { if f.is_empty() { return Err(ShellError::FileNotFound(p.span)); } + let any_source_is_dir = f.iter().any(|f| matches!(f, f if f.is_dir())); + if any_source_is_dir && !recursive { + return Err(ShellError::GenericError( + "could_not_copy_directory".into(), + "resolves to a directory (not copied)".into(), + Some(p.span), + Some("Directories must be copied using \"--recursive\"".into()), + Vec::new(), + )); + } + Ok(f) } Err(e) => Err(ShellError::GenericError( diff --git a/crates/nu-command/tests/commands/ucp.rs b/crates/nu-command/tests/commands/ucp.rs index c359f542be..0b04152042 100644 --- a/crates/nu-command/tests/commands/ucp.rs +++ b/crates/nu-command/tests/commands/ucp.rs @@ -91,15 +91,8 @@ fn error_if_attempting_to_copy_a_directory_to_another_directory_impl(progress: b dirs.test().display() ); - // Changing to GNU error like error - // Slight bug since it should say formats, but its saying "." due to the `strip_prefix` - // that i do I think - // assert!(actual.err.contains("formats")); - // assert!(actual.err.contains("resolves to a directory (not copied)")); - assert!(actual.err.contains("omitting directory")); - - // directories must be copied using --recursive - // gnu says "omitting directory", vbecause -r was not given + assert!(actual.err.contains("formats")); + assert!(actual.err.contains("resolves to a directory (not copied)")); }); }