diff --git a/crates/nu-command/src/filesystem/cp.rs b/crates/nu-command/src/filesystem/cp.rs index bdf5382bb4..92b0b5c940 100644 --- a/crates/nu-command/src/filesystem/cp.rs +++ b/crates/nu-command/src/filesystem/cp.rs @@ -1,16 +1,11 @@ -use std::collections::HashMap; use std::fs::read_link; use std::path::PathBuf; -use itertools::Itertools; use nu_engine::env::current_dir; use nu_engine::CallExt; -use nu_glob::GlobResult; -use nu_path::dots::expand_ndots; use nu_path::{canonicalize_with, expand_path_with}; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; -use nu_protocol::span::span as merge_spans; use nu_protocol::{ Category, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, Span, Spanned, SyntaxShape, Value, @@ -46,12 +41,7 @@ impl Command for Cp { fn signature(&self) -> Signature { Signature::build("cp") - .rest( - "source(s)", - SyntaxShape::String, - "the place(s) to copy from", - ) - // .required("source", SyntaxShape::GlobPattern, "the place to copy from") + .required("source", SyntaxShape::GlobPattern, "the place to copy from") .required("destination", SyntaxShape::Filepath, "the place to copy to") .switch( "recursive", @@ -81,16 +71,15 @@ impl Command for Cp { call: &Call, _input: PipelineData, ) -> Result { - let mut src_vec: Vec> = call.rest(engine_state, stack, 0)?; - // read dst as final argument - let dst: Spanned = src_vec.pop().expect("Final argument is destination"); - + let src: Spanned = call.req(engine_state, stack, 0)?; + let dst: Spanned = call.req(engine_state, stack, 1)?; let recursive = call.has_flag("recursive"); let verbose = call.has_flag("verbose"); let interactive = call.has_flag("interactive"); let current_dir_path = current_dir(engine_state, stack)?; - let destination = expand_ndots(current_dir_path.join(dst.item.as_str())); + let source = current_dir_path.join(src.item.as_str()); + let destination = current_dir_path.join(dst.item.as_str()); let path_last_char = destination.as_os_str().to_string_lossy().chars().last(); let is_directory = path_last_char == Some('/') || path_last_char == Some('\\'); @@ -103,54 +92,24 @@ impl Command for Cp { let ctrlc = engine_state.ctrlc.clone(); let span = call.head; - let mut sources: Vec = vec![]; - let mut path_to_span: HashMap = HashMap::new(); - - for src in &src_vec { - let source = current_dir_path.join(src.item.as_str()); - let glob_results: Vec = - match nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) { - Ok(files) => files.collect(), - Err(e) => { - return Err(ShellError::GenericError( - e.to_string(), - "invalid pattern".to_string(), - Some(src.span), - None, - Vec::new(), - )) - } - }; - - let mut new_sources: Vec = vec![]; - for glob_result in glob_results { - match glob_result { - Ok(path) => { - path_to_span.insert(path.clone(), src.span); - new_sources.push(path); - } - Err(e) => { - return Err(ShellError::GenericError( - e.to_string(), - "glob iteration error".to_string(), - Some(src.span), - None, - Vec::new(), - )) - } - } + let sources: Vec<_> = match nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) { + Ok(files) => files.collect(), + Err(e) => { + return Err(ShellError::GenericError( + e.to_string(), + "invalid pattern".to_string(), + Some(src.span), + None, + Vec::new(), + )) } - - sources.append(&mut new_sources); - } + }; if sources.is_empty() { return Err(ShellError::GenericError( "No matches found".into(), "no matches found".into(), - Some(merge_spans( - &src_vec.into_iter().map(|src| src.span).collect_vec(), - )), + Some(src.span), None, Vec::new(), )); @@ -166,25 +125,21 @@ impl Command for Cp { )); } - let any_source_is_dir = sources.iter().find(|f| f.is_dir()); + let any_source_is_dir = sources.iter().any(|f| matches!(f, Ok(f) if f.is_dir())); - if let Some(dir_source) = any_source_is_dir { - if !recursive { - return Err(ShellError::GenericError( - "Directories must be copied using \"--recursive\"".into(), - "resolves to a directory (not copied)".into(), - Some(*path_to_span.get(dir_source).unwrap_or_else(|| { - panic!("Key {:?} should exist", dir_source.as_os_str()) - })), - None, - Vec::new(), - )); - } + if any_source_is_dir && !recursive { + return Err(ShellError::GenericError( + "Directories must be copied using \"--recursive\"".into(), + "resolves to a directory (not copied)".into(), + Some(src.span), + None, + Vec::new(), + )); } let mut result = Vec::new(); - for entry in sources.into_iter() { + for entry in sources.into_iter().flatten() { let mut sources = FileStructure::new(); sources.walk_decorate(&entry, engine_state, stack)?; @@ -208,8 +163,7 @@ impl Command for Cp { let res = if src == dst { let message = format!( "src {:?} and dst {:?} are identical(not copied)", - src.as_os_str(), - destination + source, destination ); return Err(ShellError::GenericError( diff --git a/crates/nu-command/src/filesystem/mv.rs b/crates/nu-command/src/filesystem/mv.rs index f490111c63..1ddd3dc828 100644 --- a/crates/nu-command/src/filesystem/mv.rs +++ b/crates/nu-command/src/filesystem/mv.rs @@ -1,11 +1,8 @@ use std::path::{Path, PathBuf}; use super::util::try_interaction; -use itertools::Itertools; use nu_engine::env::current_dir; use nu_engine::CallExt; -use nu_glob::GlobResult; -use nu_path::dots::expand_ndots; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ @@ -39,16 +36,11 @@ impl Command for Mv { fn signature(&self) -> nu_protocol::Signature { Signature::build("mv") - .rest( - "source(s)", - SyntaxShape::String, - "the location(s) to move files/directories from", + .required( + "source", + SyntaxShape::GlobPattern, + "the location to move files/directories from", ) - // .required( - // "source", - // SyntaxShape::GlobPattern, - // "the location to move files/directories from", - // ) .required( "destination", SyntaxShape::Filepath, @@ -72,12 +64,8 @@ impl Command for Mv { _input: PipelineData, ) -> Result { // TODO: handle invalid directory or insufficient permissions when moving - let mut spanned_sources: Vec> = call.rest(engine_state, stack, 0)?; - // read destination as final argument - let spanned_destination: Spanned = - call.req(engine_state, stack, spanned_sources.len() - 1)?; - // don't read destination argument - spanned_sources.pop(); + let spanned_source: Spanned = call.req(engine_state, stack, 0)?; + let spanned_destination: Spanned = call.req(engine_state, stack, 1)?; let verbose = call.has_flag("verbose"); let interactive = call.has_flag("interactive"); // let force = call.has_flag("force"); @@ -85,118 +73,105 @@ impl Command for Mv { let ctrlc = engine_state.ctrlc.clone(); let path = current_dir(engine_state, stack)?; + let source = path.join(spanned_source.item.as_str()); + let destination = path.join(spanned_destination.item.as_str()); + + let mut sources = nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) + .map_or_else(|_| Vec::new(), Iterator::collect); + + if sources.is_empty() { + return Err(ShellError::GenericError( + "Invalid file or pattern".into(), + "invalid file or pattern".into(), + Some(spanned_source.span), + None, + Vec::new(), + )); + } + + // We have two possibilities. + // + // First, the destination exists. + // - If a directory, move everything into that directory, otherwise + // - if only a single source, overwrite the file, otherwise + // - error. + // + // Second, the destination doesn't exist, so we can only rename a single source. Otherwise + // it's an error. + + if (destination.exists() && !destination.is_dir() && sources.len() > 1) + || (!destination.exists() && sources.len() > 1) + { + return Err(ShellError::GenericError( + "Can only move multiple sources if destination is a directory".into(), + "destination must be a directory when multiple sources".into(), + Some(spanned_destination.span), + None, + Vec::new(), + )); + } + + let some_if_source_is_destination = sources + .iter() + .find(|f| matches!(f, Ok(f) if destination.starts_with(f))); + if destination.exists() && destination.is_dir() && sources.len() == 1 { + if let Some(Ok(filename)) = some_if_source_is_destination { + return Err(ShellError::GenericError( + format!( + "Not possible to move {:?} to itself", + filename.file_name().expect("Invalid file name") + ), + "cannot move to itself".into(), + Some(spanned_destination.span), + None, + Vec::new(), + )); + } + } + + if let Some(Ok(_filename)) = some_if_source_is_destination { + sources = sources + .into_iter() + .filter(|f| matches!(f, Ok(f) if !destination.starts_with(f))) + .collect(); + } let span = call.head; - - Ok(spanned_sources + Ok(sources .into_iter() - .flat_map(move |spanned_source| { - let path = path.clone(); - let source = path.join(spanned_source.item.as_str()); - let destination = expand_ndots(path.join(spanned_destination.item.as_str())); - - let mut sources: Vec = - nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) - .map_or_else(|_| Vec::new(), Iterator::collect); - - if sources.is_empty() { - let err = ShellError::GenericError( - "Invalid file or pattern".into(), - "invalid file or pattern".into(), - Some(spanned_source.span), - None, - Vec::new(), - ); - return Vec::from([Value::Error { error: err }]).into_iter(); + .flatten() + .filter_map(move |entry| { + let result = move_file( + Spanned { + item: entry.clone(), + span: spanned_source.span, + }, + Spanned { + item: destination.clone(), + span: spanned_destination.span, + }, + interactive, + ); + if let Err(error) = result { + Some(Value::Error { error }) + } else if verbose { + let val = if result.expect("Error value when unwrapping mv result") { + format!( + "moved {:} to {:}", + entry.to_string_lossy(), + destination.to_string_lossy() + ) + } else { + format!( + "{:} not moved to {:}", + entry.to_string_lossy(), + destination.to_string_lossy() + ) + }; + Some(Value::String { val, span }) + } else { + None } - - // We have two possibilities. - // - // First, the destination exists. - // - If a directory, move everything into that directory, otherwise - // - if only a single source, overwrite the file, otherwise - // - error. - // - // Second, the destination doesn't exist, so we can only rename a single source. Otherwise - // it's an error. - - if (destination.exists() && !destination.is_dir() && sources.len() > 1) - || (!destination.exists() && sources.len() > 1) - { - let err = ShellError::GenericError( - "Can only move multiple sources if destination is a directory".into(), - "destination must be a directory when multiple sources".into(), - Some(spanned_destination.span), - None, - Vec::new(), - ); - return Vec::from([Value::Error { error: err }]).into_iter(); - } - - let some_if_source_is_destination = sources - .iter() - .find(|f| matches!(f, Ok(f) if destination.starts_with(f))); - if destination.exists() && destination.is_dir() && sources.len() == 1 { - if let Some(Ok(filename)) = some_if_source_is_destination { - let err = ShellError::GenericError( - format!( - "Not possible to move {:?} to itself", - filename.file_name().unwrap_or(filename.as_os_str()) - ), - "cannot move to itself".into(), - Some(spanned_destination.span), - None, - Vec::new(), - ); - return Vec::from([Value::Error { error: err }]).into_iter(); - } - } - - if let Some(Ok(_filename)) = some_if_source_is_destination { - sources = sources - .into_iter() - .filter(|f| matches!(f, Ok(f) if !destination.starts_with(f))) - .collect(); - } - - sources - .into_iter() - .flatten() - .filter_map(move |entry| { - let entry = expand_ndots(entry); - let result = move_file( - Spanned { - item: entry.clone(), - span: spanned_source.span, - }, - Spanned { - item: destination.clone(), - span: spanned_destination.span, - }, - interactive, - ); - if let Err(error) = result { - Some(Value::Error { error }) - } else if verbose { - let val = match result { - Ok(true) => format!( - "moved {:} to {:}", - entry.to_string_lossy(), - destination.to_string_lossy() - ), - _ => format!( - "{:} not moved to {:}", - entry.to_string_lossy(), - destination.to_string_lossy() - ), - }; - Some(Value::String { val, span }) - } else { - None - } - }) - .collect_vec() - .into_iter() }) .into_pipeline_data(ctrlc)) } diff --git a/crates/nu-command/tests/commands/cp.rs b/crates/nu-command/tests/commands/cp.rs index aa7c9a9dc8..b12ec132d5 100644 --- a/crates/nu-command/tests/commands/cp.rs +++ b/crates/nu-command/tests/commands/cp.rs @@ -17,22 +17,6 @@ fn copies_a_file() { }); } -#[test] -fn copies_multiple_files() { - Playground::setup("cp_test_1_1", |dirs, sandbox| { - sandbox - .with_files(vec![EmptyFile("a.txt"), EmptyFile("b.txt")]) - .mkdir("dest"); - nu!( - cwd: dirs.test(), - "cp a.txt b.txt dest", - ); - - assert!(dirs.test().join("dest/a.txt").exists()); - assert!(dirs.test().join("dest/b.txt").exists()); - }); -} - #[test] fn copies_the_file_inside_directory_if_path_to_copy_is_directory() { Playground::setup("cp_test_2", |dirs, _| { diff --git a/crates/nu-command/tests/commands/move_/mv.rs b/crates/nu-command/tests/commands/move_/mv.rs index 793db1dc48..84f29ff366 100644 --- a/crates/nu-command/tests/commands/move_/mv.rs +++ b/crates/nu-command/tests/commands/move_/mv.rs @@ -22,36 +22,6 @@ fn moves_a_file() { }) } -#[test] -fn moves_multiple_files() { - Playground::setup("mv_test_1_1", |dirs, sandbox| { - sandbox - .mkdir("expected") - .with_files(vec![EmptyFile("andres.txt"), EmptyFile("yehuda.txt")]) - .within("foo") - .with_files(vec![EmptyFile("bar.txt")]); - - let original_1 = dirs.test().join("andres.txt"); - let original_2 = dirs.test().join("yehuda.txt"); - let original_3 = dirs.test().join("foo/bar.txt"); - let expected_1 = dirs.test().join("expected/andres.txt"); - let expected_2 = dirs.test().join("expected/yehuda.txt"); - let expected_3 = dirs.test().join("expected/bar.txt"); - - nu!( - cwd: dirs.test(), - "mv andres.txt yehuda.txt foo/bar.txt expected" - ); - - assert!(!original_1.exists()); - assert!(!original_2.exists()); - assert!(!original_3.exists()); - assert!(expected_1.exists()); - assert!(expected_2.exists()); - assert!(expected_3.exists()); - }) -} - #[test] fn overwrites_if_moving_to_existing_file() { Playground::setup("mv_test_2", |dirs, sandbox| {