Add support for removing multiple files at once (#1526)

crates/nu-cli/src/commands/rm.rs

@@ -10,7 +10,7 @@ pub struct Remove;
 
 #[derive(Deserialize)]
 pub struct RemoveArgs {
-    pub target: Tagged<PathBuf>,
+    pub rest: Vec<Tagged<PathBuf>>,
     pub recursive: Tagged<bool>,
     pub trash: Tagged<bool>,
 }
@@ -22,17 +22,17 @@ impl PerItemCommand for Remove {
 
     fn signature(&self) -> Signature {
         Signature::build("rm")
-            .required("path", SyntaxShape::Pattern, "the file path to remove")
             .switch(
                 "trash",
                 "use the platform's recycle bin instead of permanently deleting",
                 Some('t'),
             )
             .switch("recursive", "delete subdirectories recursively", Some('r'))
+            .rest(SyntaxShape::Pattern, "the file path(s) to remove")
     }
 
     fn usage(&self) -> &str {
-        "Remove a file"
+        "Remove file(s)"
     }
 
     fn run(

crates/nu-cli/src/shell/filesystem_shell.rs

@@ -14,6 +14,7 @@ use nu_parser::ExpandContext;
 use nu_protocol::{Primitive, ReturnSuccess, UntaggedValue};
 use rustyline::completion::FilenameCompleter;
 use rustyline::hint::{Hinter, HistoryHinter};
+use std::collections::HashMap;
 use std::path::{Component, Path, PathBuf};
 use std::sync::atomic::Ordering;
 use trash as SendToTrash;
@@ -991,7 +992,7 @@ impl Shell for FilesystemShell {
     fn rm(
         &self,
         RemoveArgs {
-            target,
+            rest: targets,
             recursive,
             trash,
         }: RemoveArgs,
@@ -1000,125 +1001,141 @@ impl Shell for FilesystemShell {
     ) -> Result<OutputStream, ShellError> {
         let name_tag = name;
 
-        if target.item.to_str() == Some(".") || target.item.to_str() == Some("..") {
+        if targets.is_empty() {
             return Err(ShellError::labeled_error(
-                "Remove aborted. \".\" or \"..\" may not be removed.",
+                "rm requires target paths",
-                "\".\" or \"..\" may not be removed",
+                "needs parameter",
-                target.tag,
+                name_tag,
             ));
         }
 
-        let mut path = PathBuf::from(path);
+        let mut all_targets: HashMap<PathBuf, Tag> = HashMap::new();
+        for target in targets {
+            if target.item.to_str() == Some(".") || target.item.to_str() == Some("..") {
+                return Err(ShellError::labeled_error(
+                    "Remove aborted. \".\" or \"..\" may not be removed.",
+                    "\".\" or \"..\" may not be removed",
+                    target.tag,
+                ));
+            }
 
-        path.push(&target.item);
+            let mut path = PathBuf::from(path);
-
+            path.push(&target.item);
-        match glob::glob(&path.to_string_lossy()) {
+            match glob::glob(&path.to_string_lossy()) {
-            Ok(files) => {
+                Ok(files) => {
-                let files: Vec<_> = files.collect();
+                    for file in files {
-                if files.is_empty() {
+                        match file {
-                    Err(ShellError::labeled_error(
+                            Ok(ref f) => {
-                        "Remove aborted. Not a valid path",
+                                all_targets
-                        "not a valid path",
+                                    .entry(f.clone())
-                        target.tag,
+                                    .or_insert_with(|| target.tag.clone());
+                            }
+                            Err(e) => {
+                                let msg = format!("Could not remove {:}", path.to_string_lossy());
+                                return Err(ShellError::labeled_error(
+                                    msg,
+                                    e.to_string(),
+                                    &target.tag,
+                                ));
+                            }
+                        }
+                    }
+                }
+                Err(e) => {
+                    return Err(ShellError::labeled_error(
+                        format!("Remove aborted. {:}", e.to_string()),
+                        e.to_string(),
+                        &name_tag,
                     ))
-                } else {
+                }
-                    let stream = async_stream! {
+            };
-                        for file in files.iter() {
+        }
-                            match file {
-                                Ok(f) => {
-                                    let is_empty =  match f.read_dir() {
-                                            Ok(mut p) => p.next().is_none(),
-                                            Err(_) => false
-                                    };
 
-                                    let valid_target =
+        if all_targets.is_empty() {
-                                        f.exists() && (!f.is_dir() || (is_empty || recursive.item));
+            Err(ShellError::labeled_error(
-                                    if valid_target {
+                "Remove aborted. No valid paths",
-                                        if trash.item {
+                "no valid paths",
-                                            match SendToTrash::remove(f) {
+                name_tag,
-                                                Err(e) => {
+            ))
-                                                    let msg = format!(
+        } else {
-                                                        "Could not delete {:}",
+            let stream = async_stream! {
-                                                        f.to_string_lossy()
+                for (f, tag) in all_targets.iter() {
-                                                    );
+                    let is_empty = match f.read_dir() {
-                                                    let label = format!("{:?}", e);
+                        Ok(mut p) => p.next().is_none(),
-                                                    yield Err(ShellError::labeled_error(
+                        Err(_) => false
-                                                        msg,
+                    };
-                                                        label,
+
-                                                        &target.tag,
+                    let valid_target =
-                                                    ))
+                        f.exists() && (!f.is_dir() || (is_empty || recursive.item));
-                                                },
+                    if valid_target {
-                                                Ok(()) => {
+                        if trash.item {
-                                                    let val = format!("deleted {:}", f.to_string_lossy()).into();
+                            match SendToTrash::remove(f) {
-                                                    yield Ok(ReturnSuccess::Value(val))
-                                                },
-                                            }
-                                        } else {
-                                            let success = if f.is_dir() {
-                                                std::fs::remove_dir_all(f)
-                                            } else {
-                                                std::fs::remove_file(f)
-                                            };
-                                            match success {
-                                                Err(e) => {
-                                                    let msg = format!(
-                                                        "Could not delete {:}",
-                                                        f.to_string_lossy()
-                                                    );
-                                                    yield Err(ShellError::labeled_error(
-                                                        msg,
-                                                        e.to_string(),
-                                                        &target.tag,
-                                                    ))
-                                                },
-                                                Ok(()) => {
-                                                    let val = format!("deleted {:}", f.to_string_lossy()).into();
-                                                    yield Ok(ReturnSuccess::Value(
-                                                        val,
-                                                    ))
-                                                },
-                                            }
-                                        }
-                                    } else {
-                                        if f.is_dir() {
-                                            let msg = format!(
-                                                "Cannot remove {:}. try --recursive",
-                                                f.to_string_lossy()
-                                            );
-                                            yield Err(ShellError::labeled_error(
-                                                msg,
-                                                "cannot remove non-empty directory",
-                                                &target.tag,
-                                            ))
-                                        } else {
-                                            let msg = format!("Invalid file: {:}", f.to_string_lossy());
-                                            yield Err(ShellError::labeled_error(
-                                                msg,
-                                                "invalid file",
-                                                &target.tag,
-                                            ))
-                                        }
-                                    }
-                                }
                                 Err(e) => {
-                                    let msg = format!("Could not remove {:}", path.to_string_lossy());
+                                    let msg = format!(
+                                        "Could not delete {:}",
+                                        f.to_string_lossy()
+                                    );
+                                    let label = format!("{:?}", e);
+                                    yield Err(ShellError::labeled_error(
+                                        msg,
+                                        label,
+                                        tag,
+                                    ))
+                                },
+                                Ok(()) => {
+                                    let val = format!("deleted {:}", f.to_string_lossy()).into();
+                                    yield Ok(ReturnSuccess::Value(val))
+                                },
+                            }
+                        } else {
+                            let success = if f.is_dir() {
+                                std::fs::remove_dir_all(f)
+                            } else {
+                                std::fs::remove_file(f)
+                            };
+                            match success {
+                                Err(e) => {
+                                    let msg = format!(
+                                        "Could not delete {:}",
+                                        f.to_string_lossy()
+                                    );
                                     yield Err(ShellError::labeled_error(
                                         msg,
                                         e.to_string(),
-                                        &target.tag,
+                                        tag,
+                                    ))
+                                },
+                                Ok(()) => {
+                                    let val = format!("deleted {:}", f.to_string_lossy()).into();
+                                    yield Ok(ReturnSuccess::Value(
+                                        val,
                                     ))
                                 },
                             }
-                            }
+                        }
-                    };
+                    } else {
-                    Ok(stream.to_output_stream())
+                        if f.is_dir() {
+                            let msg = format!(
+                                "Cannot remove {:}. try --recursive",
+                                f.to_string_lossy()
+                            );
+                            yield Err(ShellError::labeled_error(
+                                msg,
+                                "cannot remove non-empty directory",
+                                tag,
+                            ))
+                        } else {
+                            let msg = format!("Invalid file: {:}", f.to_string_lossy());
+                            yield Err(ShellError::labeled_error(
+                                msg,
+                                "invalid file",
+                                tag,
+                            ))
+                        }
+                    }
                 }
-            }
+            };
-            Err(e) => Err(ShellError::labeled_error(
+            Ok(stream.to_output_stream())
-                format!("Remove aborted. {:}", e.to_string()),
-                e.to_string(),
-                &name_tag,
-            )),
         }
     }
 

crates/nu-cli/tests/commands/rm.rs

@@ -159,3 +159,86 @@ fn errors_if_attempting_to_delete_two_dot_as_argument() {
         assert!(actual.contains("may not be removed"));
     })
 }
+
+#[test]
+fn removes_multiple_directories() {
+    Playground::setup("rm_test_9", |dirs, sandbox| {
+        sandbox
+            .within("src")
+            .with_files(vec![EmptyFile("a.rs"), EmptyFile("b.rs")])
+            .within("src/cli")
+            .with_files(vec![EmptyFile("c.rs"), EmptyFile("d.rs")])
+            .within("test")
+            .with_files(vec![EmptyFile("a_test.rs"), EmptyFile("b_test.rs")]);
+
+        nu!(
+            cwd: dirs.test(),
+            "rm src test --recursive"
+        );
+
+        assert_eq!(
+            Playground::glob_vec(&format!("{}/*", dirs.test().display())),
+            Vec::<std::path::PathBuf>::new()
+        );
+    })
+}
+
+#[test]
+fn removes_multiple_files() {
+    Playground::setup("rm_test_10", |dirs, sandbox| {
+        sandbox.with_files(vec![
+            EmptyFile("yehuda.txt"),
+            EmptyFile("jonathan.txt"),
+            EmptyFile("andres.txt"),
+        ]);
+
+        nu!(
+            cwd: dirs.test(),
+            "rm yehuda.txt jonathan.txt andres.txt"
+        );
+
+        assert_eq!(
+            Playground::glob_vec(&format!("{}/*", dirs.test().display())),
+            Vec::<std::path::PathBuf>::new()
+        );
+    })
+}
+
+#[test]
+fn removes_multiple_files_with_asterisks() {
+    Playground::setup("rm_test_11", |dirs, sandbox| {
+        sandbox.with_files(vec![
+            EmptyFile("yehuda.txt"),
+            EmptyFile("jonathan.txt"),
+            EmptyFile("andres.toml"),
+        ]);
+
+        nu!(
+            cwd: dirs.test(),
+            "rm *.txt *.toml"
+        );
+
+        assert_eq!(
+            Playground::glob_vec(&format!("{}/*", dirs.test().display())),
+            Vec::<std::path::PathBuf>::new()
+        );
+    })
+}
+
+#[test]
+fn allows_doubly_specified_file() {
+    Playground::setup("rm_test_12", |dirs, sandbox| {
+        sandbox.with_files(vec![EmptyFile("yehuda.txt"), EmptyFile("jonathan.toml")]);
+
+        let actual = nu!(
+            cwd: dirs.test(),
+            "rm *.txt yehuda* *.toml"
+        );
+
+        assert_eq!(
+            Playground::glob_vec(&format!("{}/*", dirs.test().display())),
+            Vec::<std::path::PathBuf>::new()
+        );
+        assert!(!actual.contains("error"))
+    })
+}