Revert cp and mv back to last-known-good state (#6169)

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(
+            .required("source", SyntaxShape::GlobPattern, "the place to copy from")
-                "source(s)",
-                SyntaxShape::String,
-                "the place(s) 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<PipelineData, ShellError> {
-        let mut src_vec: Vec<Spanned<String>> = call.rest(engine_state, stack, 0)?;
+        let src: Spanned<String> = call.req(engine_state, stack, 0)?;
-        // read dst as final argument
+        let dst: Spanned<String> = call.req(engine_state, stack, 1)?;
-        let dst: Spanned<String> = src_vec.pop().expect("Final argument is destination");
-
         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<PathBuf> = vec![];
+        let sources: Vec<_> = match nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) {
-        let mut path_to_span: HashMap<PathBuf, Span> = HashMap::new();
+            Ok(files) => files.collect(),
-
+            Err(e) => {
-        for src in &src_vec {
+                return Err(ShellError::GenericError(
-            let source = current_dir_path.join(src.item.as_str());
+                    e.to_string(),
-            let glob_results: Vec<GlobResult> =
+                    "invalid pattern".to_string(),
-                match nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS) {
+                    Some(src.span),
-                    Ok(files) => files.collect(),
+                    None,
-                    Err(e) => {
+                    Vec::new(),
-                        return Err(ShellError::GenericError(
+                ))
-                            e.to_string(),
-                            "invalid pattern".to_string(),
-                            Some(src.span),
-                            None,
-                            Vec::new(),
-                        ))
-                    }
-                };
-
-            let mut new_sources: Vec<PathBuf> = 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(),
-                        ))
-                    }
-                }
             }
-
+        };
-            sources.append(&mut new_sources);
-        }
 
         if sources.is_empty() {
             return Err(ShellError::GenericError(
                 "No matches found".into(),
                 "no matches found".into(),
-                Some(merge_spans(
+                Some(src.span),
-                    &src_vec.into_iter().map(|src| src.span).collect_vec(),
-                )),
                 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 any_source_is_dir && !recursive {
-            if !recursive {
+            return Err(ShellError::GenericError(
-                return Err(ShellError::GenericError(
+                "Directories must be copied using \"--recursive\"".into(),
-                    "Directories must be copied using \"--recursive\"".into(),
+                "resolves to a directory (not copied)".into(),
-                    "resolves to a directory (not copied)".into(),
+                Some(src.span),
-                    Some(*path_to_span.get(dir_source).unwrap_or_else(|| {
+                None,
-                        panic!("Key {:?} should exist", dir_source.as_os_str())
+                Vec::new(),
-                    })),
+            ));
-                    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(),
+                                source, destination
-                                destination
                             );
 
                             return Err(ShellError::GenericError(

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(
+            .required(
-                "source(s)",
+                "source",
-                SyntaxShape::String,
+                SyntaxShape::GlobPattern,
-                "the location(s) to move files/directories from",
+                "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<nu_protocol::PipelineData, nu_protocol::ShellError> {
         // TODO: handle invalid directory or insufficient permissions when moving
-        let mut spanned_sources: Vec<Spanned<String>> = call.rest(engine_state, stack, 0)?;
+        let spanned_source: Spanned<String> = call.req(engine_state, stack, 0)?;
-        // read destination as final argument
+        let spanned_destination: Spanned<String> = call.req(engine_state, stack, 1)?;
-        let spanned_destination: Spanned<String> =
-            call.req(engine_state, stack, spanned_sources.len() - 1)?;
-        // don't read destination argument
-        spanned_sources.pop();
         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(sources
-        Ok(spanned_sources
             .into_iter()
-            .flat_map(move |spanned_source| {
+            .flatten()
-                let path = path.clone();
+            .filter_map(move |entry| {
-                let source = path.join(spanned_source.item.as_str());
+                let result = move_file(
-                let destination = expand_ndots(path.join(spanned_destination.item.as_str()));
+                    Spanned {
-
+                        item: entry.clone(),
-                let mut sources: Vec<GlobResult> =
+                        span: spanned_source.span,
-                    nu_glob::glob_with(&source.to_string_lossy(), GLOB_PARAMS)
+                    },
-                        .map_or_else(|_| Vec::new(), Iterator::collect);
+                    Spanned {
-
+                        item: destination.clone(),
-                if sources.is_empty() {
+                        span: spanned_destination.span,
-                    let err = ShellError::GenericError(
+                    },
-                        "Invalid file or pattern".into(),
+                    interactive,
-                        "invalid file or pattern".into(),
+                );
-                        Some(spanned_source.span),
+                if let Err(error) = result {
-                        None,
+                    Some(Value::Error { error })
-                        Vec::new(),
+                } else if verbose {
-                    );
+                    let val = if result.expect("Error value when unwrapping mv result") {
-                    return Vec::from([Value::Error { error: err }]).into_iter();
+                        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))
     }

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, _| {

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| {