From 2b0212880e6b3e22459faa4e55de869072686669 Mon Sep 17 00:00:00 2001 From: Kevin Del Castillo Date: Sat, 18 Apr 2020 18:05:24 -0500 Subject: [PATCH] Simplify cp command and allow multiple recursive copying (#1580) * Better message error * Use custom canonicalize in FileStructure build * Better glob error in ls * Use custom canonicalize, remove some duplicate code in cd. * Enable recursive copying with patterns. * Change test to fit new error message * Test recursive with glob pattern * Show that not matches were found in cp * Fix typo in message error * Change old canonicalize usage, follow newest changes --- crates/nu-cli/src/shell/filesystem_shell.rs | 292 ++++++-------------- crates/nu-cli/src/utils.rs | 3 +- crates/nu-cli/tests/commands/cp.rs | 13 +- crates/nu-cli/tests/commands/ls.rs | 2 +- 4 files changed, 88 insertions(+), 222 deletions(-) diff --git a/crates/nu-cli/src/shell/filesystem_shell.rs b/crates/nu-cli/src/shell/filesystem_shell.rs index 9bea13943..39b94e546 100644 --- a/crates/nu-cli/src/shell/filesystem_shell.rs +++ b/crates/nu-cli/src/shell/filesystem_shell.rs @@ -124,13 +124,13 @@ impl Shell for FilesystemShell { }; let mut paths = glob::glob(&path.to_string_lossy()) - .map_err(|e| ShellError::labeled_error("Glob error", e.to_string(), &p_tag))? + .map_err(|e| ShellError::labeled_error(e.to_string(), "invalid pattern", &p_tag))? .peekable(); if paths.peek().is_none() { return Err(ShellError::labeled_error( - "Invalid File or Pattern", - "invalid file or pattern", + "No matches found", + "no matches found", &p_tag, )); } @@ -256,130 +256,79 @@ impl Shell for FilesystemShell { let path = Path::new(path); let source = path.join(&src.item); - let mut destination = path.join(&dst.item); + let destination = path.join(&dst.item); let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) { Ok(files) => files.collect(), - Err(_) => { + Err(e) => { return Err(ShellError::labeled_error( - "Invalid pattern", + e.to_string(), "invalid pattern", src.tag, )) } }; - if sources.len() == 1 { - if let Ok(entry) = &sources[0] { - if entry.is_dir() && !recursive.item { - return Err(ShellError::labeled_error( - "is a directory (not copied). Try using \"--recursive\".", - "is a directory (not copied). Try using \"--recursive\".", - src.tag, - )); - } + if sources.is_empty() { + return Err(ShellError::labeled_error( + "No matches found", + "no matches found", + src.tag, + )); + } - let mut sources: FileStructure = FileStructure::new(); + if sources.len() > 1 && !destination.is_dir() { + return Err(ShellError::labeled_error( + "Destination must be a directory when copying multiple files", + "is not a directory", + dst.tag, + )); + } + let any_source_is_dir = sources.iter().any(|f| match f { + Ok(f) => f.is_dir(), + Err(_) => false, + }); + + if any_source_is_dir && !recursive.item { + return Err(ShellError::labeled_error( + "Directories must be copied using \"--recursive\"", + "resolves to a directory (not copied)", + src.tag, + )); + } + + for entry in sources { + if let Ok(entry) = entry { + let mut sources = FileStructure::new(); sources.walk_decorate(&entry)?; if entry.is_file() { - let strategy = |(source_file, _depth_level)| { + let sources = sources.paths_applying_with(|(source_file, _depth_level)| { if destination.is_dir() { - let mut new_dst = dunce::canonicalize(destination.clone())?; + let mut dest = canonicalize(&path, &dst.item)?; if let Some(name) = entry.file_name() { - new_dst.push(name); + dest.push(name); } - Ok((source_file, new_dst)) + Ok((source_file, dest)) } else { Ok((source_file, destination.clone())) } - }; + })?; - let sources = sources.paths_applying_with(strategy)?; - - for (ref src, ref dst) in sources { + for (src, dst) in sources { if src.is_file() { - match std::fs::copy(src, dst) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; + std::fs::copy(src, dst).map_err(|e| { + ShellError::labeled_error(e.to_string(), e.to_string(), &name_tag) + })?; } } - } - - if entry.is_dir() { - if !destination.exists() { - match std::fs::create_dir_all(&destination) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - dst.tag, - )); - } - Ok(o) => o, - }; - - let strategy = |(source_file, depth_level)| { - let mut new_dst = destination.clone(); - let path = dunce::canonicalize(&source_file)?; - - let mut comps: Vec<_> = path - .components() - .map(|fragment| fragment.as_os_str()) - .rev() - .take(1 + depth_level) - .collect(); - - comps.reverse(); - - for fragment in comps.iter() { - new_dst.push(fragment); - } - - Ok((PathBuf::from(&source_file), new_dst)) - }; - - let sources = sources.paths_applying_with(strategy)?; - - let dst_tag = dst.tag; - for (ref src, ref dst) in sources { - if src.is_dir() && !dst.exists() { - match std::fs::create_dir_all(dst) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - dst_tag, - )); - } - Ok(o) => o, - }; - } - - if src.is_file() { - match std::fs::copy(src, dst) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } - } + } else if entry.is_dir() { + let destination = if !destination.exists() { + destination.clone() } else { match entry.file_name() { - Some(name) => destination.push(name), + Some(name) => destination.join(name), None => { return Err(ShellError::labeled_error( "Copy aborted. Not a valid path", @@ -388,131 +337,46 @@ impl Shell for FilesystemShell { )) } } + }; - match std::fs::create_dir_all(&destination) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - dst.tag, - )); - } - Ok(o) => o, - }; + std::fs::create_dir_all(&destination).map_err(|e| { + ShellError::labeled_error(e.to_string(), e.to_string(), &dst.tag) + })?; - let strategy = |(source_file, depth_level)| { - let mut new_dst = dunce::canonicalize(&destination)?; - let path = dunce::canonicalize(&source_file)?; + let sources = sources.paths_applying_with(|(source_file, depth_level)| { + let mut dest = destination.clone(); + let path = canonicalize(&path, &source_file)?; - let mut comps: Vec<_> = path - .components() - .map(|fragment| fragment.as_os_str()) - .rev() - .take(1 + depth_level) - .collect(); + let comps: Vec<_> = path + .components() + .map(|fragment| fragment.as_os_str()) + .rev() + .take(1 + depth_level) + .collect(); - comps.reverse(); + for fragment in comps.into_iter().rev() { + dest.push(fragment); + } - for fragment in comps.iter() { - new_dst.push(fragment); - } + Ok((PathBuf::from(&source_file), dest)) + })?; - Ok((PathBuf::from(&source_file), new_dst)) - }; + let dst_tag = &dst.tag; + for (src, dst) in sources { + if src.is_dir() && !dst.exists() { + std::fs::create_dir_all(&dst).map_err(|e| { + ShellError::labeled_error(e.to_string(), e.to_string(), dst_tag) + })?; + } - let sources = sources.paths_applying_with(strategy)?; - - let dst_tag = dst.tag; - for (ref src, ref dst) in sources { - if src.is_dir() && !dst.exists() { - match std::fs::create_dir_all(dst) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - dst_tag, - )); - } - Ok(o) => o, - }; - } - - if src.is_file() { - match std::fs::copy(src, dst) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - name_tag, - )); - } - Ok(o) => o, - }; - } + if src.is_file() { + std::fs::copy(&src, &dst).map_err(|e| { + ShellError::labeled_error(e.to_string(), e.to_string(), &name_tag) + })?; } } } } - } else if destination.exists() { - if !sources.iter().all(|x| match x { - Ok(f) => f.is_file(), - Err(_) => false, - }) { - return Err(ShellError::labeled_error( - "Copy aborted (directories found). Recursive copying in patterns not supported yet (try copying the directory directly)", - "recursive copying in patterns not supported", - src.tag, - )); - } - - for entry in sources { - if let Ok(entry) = entry { - let mut to = PathBuf::from(&destination); - - match entry.file_name() { - Some(name) => to.push(name), - None => { - return Err(ShellError::labeled_error( - "Copy aborted. Not a valid path", - "not a valid path", - dst.tag, - )) - } - } - - if entry.is_file() { - match std::fs::copy(&entry, &to) { - Err(e) => { - return Err(ShellError::labeled_error( - e.to_string(), - e.to_string(), - src.tag, - )); - } - Ok(o) => o, - }; - } - } - } - } else { - let destination_file_name = { - match destination.file_name() { - Some(name) => PathBuf::from(name), - None => { - return Err(ShellError::labeled_error( - "Copy aborted. Not a valid destination", - "not a valid destination", - dst.tag, - )) - } - } - }; - - return Err(ShellError::labeled_error( - format!("Copy aborted. (Does {:?} exist?)", destination_file_name), - format!("copy aborted (does {:?} exist?)", destination_file_name), - dst.tag, - )); } Ok(OutputStream::empty()) diff --git a/crates/nu-cli/src/utils.rs b/crates/nu-cli/src/utils.rs index 94b3fb92a..4cbb3d83c 100644 --- a/crates/nu-cli/src/utils.rs +++ b/crates/nu-cli/src/utils.rs @@ -1,5 +1,6 @@ pub mod data_processing; +use crate::path::canonicalize; use nu_errors::ShellError; use nu_protocol::{UntaggedValue, Value}; use std::path::{Component, Path, PathBuf}; @@ -138,7 +139,7 @@ impl FileStructure { } fn build(&mut self, src: &Path, lvl: usize) -> Result<(), ShellError> { - let source = dunce::canonicalize(src)?; + let source = canonicalize(std::env::current_dir()?, src)?; if source.is_dir() { for entry in std::fs::read_dir(src)? { diff --git a/crates/nu-cli/tests/commands/cp.rs b/crates/nu-cli/tests/commands/cp.rs index 6de98a71f..21b00aaab 100644 --- a/crates/nu-cli/tests/commands/cp.rs +++ b/crates/nu-cli/tests/commands/cp.rs @@ -40,7 +40,7 @@ fn error_if_attempting_to_copy_a_directory_to_another_directory() { ); assert!(actual.contains("../formats")); - assert!(actual.contains("is a directory (not copied)")); + assert!(actual.contains("resolves to a directory (not copied)")); }); } @@ -209,7 +209,7 @@ fn copy_files_using_glob_two_parents_up_using_multiple_dots() { "jonathan.json", "andres.xml", "kevin.txt", - "many_more.ppl" + "many_more.ppl", ], dirs.test() )); @@ -217,20 +217,21 @@ fn copy_files_using_glob_two_parents_up_using_multiple_dots() { } #[test] -fn copy_file_from_two_parents_up_using_multiple_dots_to_current_dir() { +fn copy_file_and_dir_from_two_parents_up_using_multiple_dots_to_current_dir_recursive() { Playground::setup("cp_test_10", |dirs, sandbox| { sandbox.with_files(vec![EmptyFile("hello_there")]); + sandbox.mkdir("hello_again"); sandbox.within("foo").mkdir("bar"); nu!( cwd: dirs.test().join("foo/bar"), r#" - cp .../hello_there . + cp -r .../hello* . "# ); - let expected = dirs.test().join("foo/bar/hello_there"); + let expected = dirs.test().join("foo/bar"); - assert!(expected.exists()); + assert!(files_exist_at(vec!["hello_there", "hello_again"], expected)); }) } diff --git a/crates/nu-cli/tests/commands/ls.rs b/crates/nu-cli/tests/commands/ls.rs index 7b686f20e..70b853c3c 100644 --- a/crates/nu-cli/tests/commands/ls.rs +++ b/crates/nu-cli/tests/commands/ls.rs @@ -128,7 +128,7 @@ fn fails_when_glob_doesnt_match() { "ls root3*" ); - assert!(actual.contains("invalid file or pattern")); + assert!(actual.contains("no matches found")); }) }