Merge pull request #4013 from palaster/rm-correct-prompts

rm: rm3 now passes

Cargo.lock

@@ -2818,6 +2818,7 @@ name = "uu_rm"
 version = "0.0.16"
 dependencies = [
  "clap 4.0.18",
+ "libc",
  "remove_dir_all 0.7.0",
  "uucore",
  "walkdir",

src/uu/rm/Cargo.toml

@@ -20,6 +20,9 @@ walkdir = "2.2"
 remove_dir_all = "0.7.0"
 uucore = { version=">=0.0.16", package="uucore", path="../../uucore", features=["fs"] }
 
+[target.'cfg(unix)'.dependencies]
+libc = "0.2.137"
+
 [target.'cfg(windows)'.dependencies]
 windows-sys = { version = "0.42.0", default-features = false, features = ["Win32_Storage_FileSystem"] }
 

src/uu/rm/src/rm.rs

@@ -10,13 +10,11 @@
 #[macro_use]
 extern crate uucore;
 
-use clap::{crate_version, Arg, ArgAction, Command};
+use clap::{crate_version, parser::ValueSource, Arg, ArgAction, Command};
 use remove_dir_all::remove_dir_all;
 use std::collections::VecDeque;
-use std::fs;
+use std::fs::{self, File, Metadata};
-use std::fs::File;
+use std::io::{stderr, stdin, BufRead, ErrorKind, Write};
-use std::io::ErrorKind;
-use std::io::{stderr, stdin, BufRead, Write};
 use std::ops::BitOr;
 use std::path::{Path, PathBuf};
 use uucore::display::Quotable;
@@ -87,17 +85,30 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         .map(|v| v.map(ToString::to_string).collect())
         .unwrap_or_default();
 
-    let force = matches.get_flag(OPT_FORCE);
+    let force_flag = matches.get_flag(OPT_FORCE);
 
-    if files.is_empty() && !force {
+    // If -f(--force) is before any -i (or variants) we want prompts else no prompts
+    let force_prompt_never: bool = force_flag && {
+        let force_index = matches.index_of(OPT_FORCE).unwrap_or(0);
+        ![OPT_PROMPT, OPT_PROMPT_MORE, OPT_INTERACTIVE]
+            .iter()
+            .any(|flag| {
+                matches.value_source(flag) == Some(ValueSource::CommandLine)
+                    && matches.index_of(flag).unwrap_or(0) > force_index
+            })
+    };
+
+    if files.is_empty() && !force_flag {
         // Still check by hand and not use clap
         // Because "rm -f" is a thing
         return Err(UUsageError::new(1, "missing operand"));
     } else {
         let options = Options {
-            force,
+            force: force_flag,
             interactive: {
-                if matches.get_flag(OPT_PROMPT) {
+                if force_prompt_never {
+                    InteractiveMode::Never
+                } else if matches.get_flag(OPT_PROMPT) {
                     InteractiveMode::Always
                 } else if matches.get_flag(OPT_PROMPT_MORE) {
                     InteractiveMode::Once
@@ -159,12 +170,17 @@ pub fn uu_app() -> Command {
             Arg::new(OPT_PROMPT)
                 .short('i')
                 .help("prompt before every removal")
+                .overrides_with_all(&[OPT_PROMPT_MORE, OPT_INTERACTIVE])
+                .action(ArgAction::SetTrue),
+        )
+        .arg(
+            Arg::new(OPT_PROMPT_MORE)
+                .short('I')
+                .help("prompt once before removing more than three files, or when removing recursively. \
+                Less intrusive than -i, while still giving some protection against most mistakes")
+                .overrides_with_all(&[OPT_PROMPT, OPT_INTERACTIVE])
                 .action(ArgAction::SetTrue),
         )
-        .arg(Arg::new(OPT_PROMPT_MORE).short('I').help(
-            "prompt once before removing more than three files, or when removing recursively. \
-            Less intrusive than -i, while still giving some protection against most mistakes",
-        ).action(ArgAction::SetTrue))
         .arg(
             Arg::new(OPT_INTERACTIVE)
                 .long(OPT_INTERACTIVE)
@@ -172,7 +188,8 @@ pub fn uu_app() -> Command {
                     "prompt according to WHEN: never, once (-I), or always (-i). Without WHEN, \
                     prompts always",
                 )
-                .value_name("WHEN"),
+                .value_name("WHEN")
+                .overrides_with_all(&[OPT_PROMPT, OPT_PROMPT_MORE]),
         )
         .arg(
             Arg::new(OPT_ONE_FILE_SYSTEM)
@@ -361,12 +378,7 @@ fn handle_dir(path: &Path, options: &Options) -> bool {
 }
 
 fn remove_dir(path: &Path, options: &Options) -> bool {
-    let response = if options.interactive == InteractiveMode::Always {
+    if prompt_file(path, options, true) {
-        prompt_file(path, true)
-    } else {
-        true
-    };
-    if response {
         if let Ok(mut read_dir) = fs::read_dir(path) {
             if options.dir || options.recursive {
                 if read_dir.next().is_none() {
@@ -411,12 +423,7 @@ fn remove_dir(path: &Path, options: &Options) -> bool {
 }
 
 fn remove_file(path: &Path, options: &Options) -> bool {
-    let response = if options.interactive == InteractiveMode::Always {
+    if prompt_file(path, options, false) {
-        prompt_file(path, false)
-    } else {
-        true
-    };
-    if response && prompt_write_protected(path, false, options) {
         match fs::remove_file(path) {
             Ok(_) => {
                 if options.verbose {
@@ -438,46 +445,138 @@ fn remove_file(path: &Path, options: &Options) -> bool {
     false
 }
 
-fn prompt_write_protected(path: &Path, is_dir: bool, options: &Options) -> bool {
+fn prompt_file(path: &Path, options: &Options, is_dir: bool) -> bool {
+    // If interactive is Never we never want to send prompts
     if options.interactive == InteractiveMode::Never {
         return true;
     }
-    match File::open(path) {
+    // If interactive is Always we want to check if the file is symlink to prompt the right message
-        Ok(_) => true,
+    if options.interactive == InteractiveMode::Always {
-        Err(err) => {
+        if let Ok(metadata) = fs::symlink_metadata(path) {
-            if err.kind() == ErrorKind::PermissionDenied {
+            if metadata.is_symlink() {
-                if is_dir {
+                return prompt(&(format!("remove symbolic link {}? ", path.quote())));
-                    prompt(&(format!("rm: remove write-protected directory {}? ", path.quote())))
+            }
-                } else {
+        }
-                    if fs::metadata(path).unwrap().len() == 0 {
+    }
-                        return prompt(
+    if is_dir {
-                            &(format!(
+        // We can't use metadata.permissions.readonly for directories because it only works on files
-                                "rm: remove write-protected regular empty file {}? ",
+        // So we have to handle wether a directory is writable on not manually
-                                path.quote()
+        if let Ok(metadata) = fs::metadata(path) {
-                            )),
+            handle_writable_directory(path, options, &metadata)
-                        );
+        } else {
+            true
+        }
+    } else {
+        // File::open(path) doesn't open the file in write mode so we need to use file options to open it in also write mode to check if it can written too
+        match File::options().read(true).write(true).open(path) {
+            Ok(file) => {
+                if let Ok(metadata) = file.metadata() {
+                    if metadata.permissions().readonly() {
+                        if metadata.len() == 0 {
+                            prompt(
+                                &(format!(
+                                    "remove write-protected regular empty file {}? ",
+                                    path.quote()
+                                )),
+                            )
+                        } else {
+                            prompt(
+                                &(format!(
+                                    "remove write-protected regular file {}? ",
+                                    path.quote()
+                                )),
+                            )
+                        }
+                    } else if options.interactive == InteractiveMode::Always {
+                        if metadata.len() == 0 {
+                            prompt(&(format!("remove regular empty file {}? ", path.quote())))
+                        } else {
+                            prompt(&(format!("remove file {}? ", path.quote())))
+                        }
+                    } else {
+                        true
                     }
-                    prompt(&(format!("rm: remove write-protected regular file {}? ", path.quote())))
+                } else {
+                    true
+                }
+            }
+            Err(err) => {
+                if err.kind() == ErrorKind::PermissionDenied {
+                    if let Ok(metadata) = fs::metadata(path) {
+                        if metadata.len() == 0 {
+                            prompt(
+                                &(format!(
+                                    "remove write-protected regular empty file {}? ",
+                                    path.quote()
+                                )),
+                            )
+                        } else {
+                            prompt(
+                                &(format!(
+                                    "remove write-protected regular file {}? ",
+                                    path.quote()
+                                )),
+                            )
+                        }
+                    } else {
+                        prompt(&(format!("remove write-protected regular file {}? ", path.quote())))
+                    }
+                } else {
+                    true
                 }
-            } else {
-                true
             }
         }
     }
 }
 
-fn prompt_descend(path: &Path) -> bool {
+// For directories finding if they are writable or not is a hassle. In Unix we can use the built-in rust crate to to check mode bits. But other os don't have something similar afaik
-    prompt(&(format!("rm: descend into directory {}? ", path.quote())))
+// Most cases are covered by keep eye out for edge cases
+#[cfg(unix)]
+fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata) -> bool {
+    use std::os::unix::fs::PermissionsExt;
+    let mode = metadata.permissions().mode();
+    // Check if directory has user write permissions
+    // Why is S_IWUSR showing up as a u16 on macos?
+    let user_writable = (mode & (libc::S_IWUSR as u32)) != 0;
+    if !user_writable {
+        prompt(&(format!("remove write-protected directory {}? ", path.quote())))
+    } else if options.interactive == InteractiveMode::Always {
+        prompt(&(format!("remove directory {}? ", path.quote())))
+    } else {
+        true
+    }
 }
 
-fn prompt_file(path: &Path, is_dir: bool) -> bool {
+// For windows we can use windows metadata trait and file attributes to see if a directory is readonly
-    if is_dir {
+#[cfg(windows)]
-        prompt(&(format!("rm: remove directory {}? ", path.quote())))
+fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata) -> bool {
+    use std::os::windows::prelude::MetadataExt;
+    use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_READONLY;
+    let not_user_writable = (metadata.file_attributes() & FILE_ATTRIBUTE_READONLY) != 0;
+    if not_user_writable {
+        prompt(&(format!("remove write-protected directory {}? ", path.quote())))
+    } else if options.interactive == InteractiveMode::Always {
+        prompt(&(format!("remove directory {}? ", path.quote())))
     } else {
-        prompt(&(format!("rm: remove file {}? ", path.quote())))
+        true
     }
 }
 
+// I have this here for completeness but it will always return "remove directory {}" because metadata.permissions().readonly() only works for file not directories
+#[cfg(not(windows))]
+#[cfg(not(unix))]
+fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata) -> bool {
+    if options.interactive == InteractiveMode::Always {
+        prompt(&(format!("remove directory {}? ", path.quote())))
+    } else {
+        true
+    }
+}
+
+fn prompt_descend(path: &Path) -> bool {
+    prompt(&(format!("descend into directory {}? ", path.quote())))
+}
+
 fn normalize(path: &Path) -> PathBuf {
     // copied from https://github.com/rust-lang/cargo/blob/2e4cfc2b7d43328b207879228a2ca7d427d188bb/src/cargo/util/paths.rs#L65-L90
     // both projects are MIT https://github.com/rust-lang/cargo/blob/master/LICENSE-MIT
@@ -487,7 +586,7 @@ fn normalize(path: &Path) -> PathBuf {
 }
 
 fn prompt(msg: &str) -> bool {
-    let _ = stderr().write_all(msg.as_bytes());
+    let _ = stderr().write_all(format!("{}: {}", uucore::util_name(), msg).as_bytes());
     let _ = stderr().flush();
 
     let mut buf = Vec::new();
@@ -501,15 +600,13 @@ fn prompt(msg: &str) -> bool {
 }
 
 #[cfg(not(windows))]
-fn is_symlink_dir(_metadata: &fs::Metadata) -> bool {
+fn is_symlink_dir(_metadata: &Metadata) -> bool {
     false
 }
 
 #[cfg(windows)]
-use std::os::windows::prelude::MetadataExt;
+fn is_symlink_dir(metadata: &Metadata) -> bool {
-
+    use std::os::windows::prelude::MetadataExt;
-#[cfg(windows)]
-fn is_symlink_dir(metadata: &fs::Metadata) -> bool {
     use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_DIRECTORY;
 
     metadata.file_type().is_symlink()

tests/by-util/test_rm.rs

@@ -400,6 +400,151 @@ fn test_rm_descend_directory() {
     assert!(!at.file_exists(file_2));
 }
 
+#[cfg(feature = "chmod")]
+#[test]
+fn test_rm_prompts() {
+    use std::io::Write;
+    use std::process::Child;
+
+    // Needed for talking with stdin on platforms where CRLF or LF matters
+    const END_OF_LINE: &str = if cfg!(windows) { "\r\n" } else { "\n" };
+
+    let mut answers = vec![
+        "rm: descend into directory 'a'?",
+        "rm: remove write-protected regular empty file 'a/empty-no-write'?",
+        "rm: remove symbolic link 'a/slink'?",
+        "rm: remove symbolic link 'a/slink-dot'?",
+        "rm: remove write-protected regular file 'a/f-no-write'?",
+        "rm: remove regular empty file 'a/empty'?",
+        "rm: remove directory 'a/b'?",
+        "rm: remove write-protected directory 'a/b-no-write'?",
+        "rm: remove directory 'a'?",
+    ];
+
+    answers.sort();
+
+    let yes = format!("y{}", END_OF_LINE);
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("a/");
+
+    let file_1 = "a/empty";
+    let file_2 = "a/empty-no-write";
+    let file_3 = "a/f-no-write";
+
+    at.touch(file_1);
+    at.touch(file_2);
+    at.make_file(file_3)
+        .write_all(b"not-empty")
+        .expect("Couldn't write to a/f-no-write");
+
+    at.symlink_dir("a/empty-f", "a/slink");
+    at.symlink_dir(".", "a/slink-dot");
+
+    let dir_1 = "a/b/";
+    let dir_2 = "a/b-no-write/";
+
+    at.mkdir(dir_1);
+    at.mkdir(dir_2);
+
+    scene
+        .ccmd("chmod")
+        .arg("u-w")
+        .arg(file_3)
+        .arg(dir_2)
+        .arg(file_2)
+        .succeeds();
+
+    let mut child: Child = scene.ucmd().arg("-ri").arg("a").run_no_wait();
+
+    let mut child_stdin = child.stdin.take().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+
+    let output = child.wait_with_output().unwrap();
+
+    let mut trimmed_output = Vec::new();
+    for string in String::from_utf8(output.stderr)
+        .expect("Couldn't convert output.stderr to string")
+        .split("rm: ")
+    {
+        if !string.is_empty() {
+            let trimmed_string = format!("rm: {}", string).trim().to_string();
+            trimmed_output.push(trimmed_string);
+        }
+    }
+
+    trimmed_output.sort();
+
+    assert!(trimmed_output.len() == answers.len());
+
+    for (i, checking_string) in trimmed_output.iter().enumerate() {
+        assert!(checking_string == answers[i]);
+    }
+
+    assert!(!at.dir_exists("a"));
+}
+
+#[test]
+fn test_rm_force_prompts_order() {
+    use std::io::Write;
+    use std::process::Child;
+
+    // Needed for talking with stdin on platforms where CRLF or LF matters
+    const END_OF_LINE: &str = if cfg!(windows) { "\r\n" } else { "\n" };
+
+    let yes = format!("y{}", END_OF_LINE);
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let empty_file = "empty";
+
+    at.touch(empty_file);
+
+    // This should cause rm to prompt to remove regular empty file
+    let mut child: Child = scene.ucmd().arg("-fi").arg(empty_file).run_no_wait();
+
+    let mut child_stdin = child.stdin.take().unwrap();
+    child_stdin.write_all(yes.as_bytes()).unwrap();
+    child_stdin.flush().unwrap();
+
+    let output = child.wait_with_output().unwrap();
+    let string_output =
+        String::from_utf8(output.stderr).expect("Couldn't convert output.stderr to string");
+    assert!(string_output.trim() == "rm: remove regular empty file 'empty'?");
+    assert!(!at.file_exists(empty_file));
+
+    at.touch(empty_file);
+
+    // This should not cause rm to prompt to remove regular empty file
+    scene
+        .ucmd()
+        .arg("-if")
+        .arg(empty_file)
+        .succeeds()
+        .no_stderr();
+    assert!(!at.file_exists(empty_file));
+}
+
 #[test]
 #[ignore = "issue #3722"]
 fn test_rm_directory_rights_rm1() {