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
This commit is contained in:
Kevin Del Castillo 2020-04-18 18:05:24 -05:00 committed by GitHub
parent a16a91ede8
commit 2b0212880e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 88 additions and 222 deletions

View File

@ -124,13 +124,13 @@ impl Shell for FilesystemShell {
}; };
let mut paths = glob::glob(&path.to_string_lossy()) 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(); .peekable();
if paths.peek().is_none() { if paths.peek().is_none() {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
"Invalid File or Pattern", "No matches found",
"invalid file or pattern", "no matches found",
&p_tag, &p_tag,
)); ));
} }
@ -256,221 +256,79 @@ impl Shell for FilesystemShell {
let path = Path::new(path); let path = Path::new(path);
let source = path.join(&src.item); 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()) { let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) {
Ok(files) => files.collect(), Ok(files) => files.collect(),
Err(_) => { Err(e) => {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
"Invalid pattern", e.to_string(),
"invalid pattern", "invalid pattern",
src.tag, src.tag,
)) ))
} }
}; };
if sources.len() == 1 { if sources.is_empty() {
if let Ok(entry) = &sources[0] {
if entry.is_dir() && !recursive.item {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
"is a directory (not copied). Try using \"--recursive\".", "No matches found",
"is a directory (not copied). Try using \"--recursive\".", "no matches found",
src.tag, src.tag,
)); ));
} }
let mut sources: FileStructure = FileStructure::new(); if sources.len() > 1 && !destination.is_dir() {
sources.walk_decorate(&entry)?;
if entry.is_file() {
let strategy = |(source_file, _depth_level)| {
if destination.is_dir() {
let mut new_dst = dunce::canonicalize(destination.clone())?;
if let Some(name) = entry.file_name() {
new_dst.push(name);
}
Ok((source_file, new_dst))
} else {
Ok((source_file, destination.clone()))
}
};
let sources = sources.paths_applying_with(strategy)?;
for (ref src, ref dst) in sources {
if src.is_file() {
match std::fs::copy(src, dst) {
Err(e) => {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
e.to_string(), "Destination must be a directory when copying multiple files",
e.to_string(), "is not a directory",
name_tag,
));
}
Ok(o) => o,
};
}
}
}
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, dst.tag,
)); ));
} }
Ok(o) => o,
};
let strategy = |(source_file, depth_level)| { let any_source_is_dir = sources.iter().any(|f| match f {
let mut new_dst = destination.clone(); Ok(f) => f.is_dir(),
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 {
match entry.file_name() {
Some(name) => destination.push(name),
None => {
return Err(ShellError::labeled_error(
"Copy aborted. Not a valid path",
"not a valid path",
dst.tag,
))
}
}
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 = dunce::canonicalize(&destination)?;
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 destination.exists() {
if !sources.iter().all(|x| match x {
Ok(f) => f.is_file(),
Err(_) => false, Err(_) => false,
}) { });
if any_source_is_dir && !recursive.item {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
"Copy aborted (directories found). Recursive copying in patterns not supported yet (try copying the directory directly)", "Directories must be copied using \"--recursive\"",
"recursive copying in patterns not supported", "resolves to a directory (not copied)",
src.tag, src.tag,
)); ));
} }
for entry in sources { for entry in sources {
if let Ok(entry) = entry { if let Ok(entry) = entry {
let mut to = PathBuf::from(&destination); let mut sources = FileStructure::new();
sources.walk_decorate(&entry)?;
if entry.is_file() {
let sources = sources.paths_applying_with(|(source_file, _depth_level)| {
if destination.is_dir() {
let mut dest = canonicalize(&path, &dst.item)?;
if let Some(name) = entry.file_name() {
dest.push(name);
}
Ok((source_file, dest))
} else {
Ok((source_file, destination.clone()))
}
})?;
for (src, dst) in sources {
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 entry.is_dir() {
let destination = if !destination.exists() {
destination.clone()
} else {
match entry.file_name() { match entry.file_name() {
Some(name) => to.push(name), Some(name) => destination.join(name),
None => { None => {
return Err(ShellError::labeled_error( return Err(ShellError::labeled_error(
"Copy aborted. Not a valid path", "Copy aborted. Not a valid path",
@ -479,40 +337,46 @@ impl Shell for FilesystemShell {
)) ))
} }
} }
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( std::fs::create_dir_all(&destination).map_err(|e| {
format!("Copy aborted. (Does {:?} exist?)", destination_file_name), ShellError::labeled_error(e.to_string(), e.to_string(), &dst.tag)
format!("copy aborted (does {:?} exist?)", destination_file_name), })?;
dst.tag,
)); let sources = sources.paths_applying_with(|(source_file, depth_level)| {
let mut dest = destination.clone();
let path = canonicalize(&path, &source_file)?;
let comps: Vec<_> = path
.components()
.map(|fragment| fragment.as_os_str())
.rev()
.take(1 + depth_level)
.collect();
for fragment in comps.into_iter().rev() {
dest.push(fragment);
}
Ok((PathBuf::from(&source_file), dest))
})?;
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)
})?;
}
if src.is_file() {
std::fs::copy(&src, &dst).map_err(|e| {
ShellError::labeled_error(e.to_string(), e.to_string(), &name_tag)
})?;
}
}
}
}
} }
Ok(OutputStream::empty()) Ok(OutputStream::empty())

View File

@ -1,5 +1,6 @@
pub mod data_processing; pub mod data_processing;
use crate::path::canonicalize;
use nu_errors::ShellError; use nu_errors::ShellError;
use nu_protocol::{UntaggedValue, Value}; use nu_protocol::{UntaggedValue, Value};
use std::path::{Component, Path, PathBuf}; use std::path::{Component, Path, PathBuf};
@ -138,7 +139,7 @@ impl FileStructure {
} }
fn build(&mut self, src: &Path, lvl: usize) -> Result<(), ShellError> { 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() { if source.is_dir() {
for entry in std::fs::read_dir(src)? { for entry in std::fs::read_dir(src)? {

View File

@ -40,7 +40,7 @@ fn error_if_attempting_to_copy_a_directory_to_another_directory() {
); );
assert!(actual.contains("../formats")); 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", "jonathan.json",
"andres.xml", "andres.xml",
"kevin.txt", "kevin.txt",
"many_more.ppl" "many_more.ppl",
], ],
dirs.test() dirs.test()
)); ));
@ -217,20 +217,21 @@ fn copy_files_using_glob_two_parents_up_using_multiple_dots() {
} }
#[test] #[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| { Playground::setup("cp_test_10", |dirs, sandbox| {
sandbox.with_files(vec![EmptyFile("hello_there")]); sandbox.with_files(vec![EmptyFile("hello_there")]);
sandbox.mkdir("hello_again");
sandbox.within("foo").mkdir("bar"); sandbox.within("foo").mkdir("bar");
nu!( nu!(
cwd: dirs.test().join("foo/bar"), cwd: dirs.test().join("foo/bar"),
r#" 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));
}) })
} }

View File

@ -128,7 +128,7 @@ fn fails_when_glob_doesnt_match() {
"ls root3*" "ls root3*"
); );
assert!(actual.contains("invalid file or pattern")); assert!(actual.contains("no matches found"));
}) })
} }