From 8bf15fe676300b69fb28b2e52a391fcb13aec414 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 22 Oct 2025 00:07:15 +0200 Subject: [PATCH] rm: add the --progress option like with cp & mv --- Cargo.lock | 1 + docs/src/extensions.md | 4 + src/uu/rm/Cargo.toml | 1 + src/uu/rm/locales/en-US.ftl | 4 + src/uu/rm/locales/fr-FR.ftl | 4 + src/uu/rm/src/platform/linux.rs | 29 ++++++- src/uu/rm/src/rm.rs | 145 ++++++++++++++++++++++++++++---- tests/by-util/test_rm.rs | 68 +++++++++++++++ 8 files changed, 234 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4b150520e..b3926443e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3879,6 +3879,7 @@ dependencies = [ "clap", "codspeed-divan-compat", "fluent", + "indicatif", "libc", "tempfile", "thiserror 2.0.17", diff --git a/docs/src/extensions.md b/docs/src/extensions.md index cb8a9cf6e..6a479ad65 100644 --- a/docs/src/extensions.md +++ b/docs/src/extensions.md @@ -43,6 +43,10 @@ packages. `mv` can display a progress bar when the `-g`/`--progress` flag is set. +## `rm` + +`rm` can display a progress bar when the `-g`/`--progress` flag is set. + ## `hashsum` This utility does not exist in GNU coreutils. `hashsum` is a utility that diff --git a/src/uu/rm/Cargo.toml b/src/uu/rm/Cargo.toml index d4b9db954..ccf1bf93e 100644 --- a/src/uu/rm/Cargo.toml +++ b/src/uu/rm/Cargo.toml @@ -22,6 +22,7 @@ thiserror = { workspace = true } clap = { workspace = true } uucore = { workspace = true, features = ["fs", "parser", "safe-traversal"] } fluent = { workspace = true } +indicatif = { workspace = true } [target.'cfg(unix)'.dependencies] libc = { workspace = true } diff --git a/src/uu/rm/locales/en-US.ftl b/src/uu/rm/locales/en-US.ftl index de12feae4..a84f746f2 100644 --- a/src/uu/rm/locales/en-US.ftl +++ b/src/uu/rm/locales/en-US.ftl @@ -28,6 +28,10 @@ rm-help-preserve-root = do not remove '/' (default) rm-help-recursive = remove directories and their contents recursively rm-help-dir = remove empty directories rm-help-verbose = explain what is being done +rm-help-progress = display a progress bar. Note: this feature is not supported by GNU coreutils. + +# Progress messages +rm-progress-removing = Removing # Error messages rm-error-missing-operand = missing operand diff --git a/src/uu/rm/locales/fr-FR.ftl b/src/uu/rm/locales/fr-FR.ftl index 2bff4a66f..a3da4ba0b 100644 --- a/src/uu/rm/locales/fr-FR.ftl +++ b/src/uu/rm/locales/fr-FR.ftl @@ -28,6 +28,10 @@ rm-help-preserve-root = ne pas supprimer '/' (par défaut) rm-help-recursive = supprimer les répertoires et leur contenu récursivement rm-help-dir = supprimer les répertoires vides rm-help-verbose = expliquer ce qui est fait +rm-help-progress = afficher une barre de progression. Note : cette fonctionnalité n'est pas supportée par GNU coreutils. + +# Messages de progression +rm-progress-removing = Suppression # Messages d'erreur rm-error-missing-operand = opérande manquant diff --git a/src/uu/rm/src/platform/linux.rs b/src/uu/rm/src/platform/linux.rs index 265229cab..6c7d32395 100644 --- a/src/uu/rm/src/platform/linux.rs +++ b/src/uu/rm/src/platform/linux.rs @@ -7,6 +7,7 @@ // spell-checker:ignore fstatat unlinkat +use indicatif::ProgressBar; use std::ffi::OsStr; use std::fs; use std::path::Path; @@ -28,7 +29,11 @@ pub fn is_readable(path: &Path) -> bool { } /// Remove a single file using safe traversal -pub fn safe_remove_file(path: &Path, options: &Options) -> Option { +pub fn safe_remove_file( + path: &Path, + options: &Options, + progress_bar: Option<&ProgressBar>, +) -> Option { let parent = path.parent()?; let file_name = path.file_name()?; @@ -36,6 +41,10 @@ pub fn safe_remove_file(path: &Path, options: &Options) -> Option { match dir_fd.unlink_at(file_name, false) { Ok(_) => { + // Update progress bar for file removal + if let Some(pb) = progress_bar { + pb.inc(1); + } verbose_removed_file(path, options); Some(false) } @@ -51,7 +60,11 @@ pub fn safe_remove_file(path: &Path, options: &Options) -> Option { } /// Remove an empty directory using safe traversal -pub fn safe_remove_empty_dir(path: &Path, options: &Options) -> Option { +pub fn safe_remove_empty_dir( + path: &Path, + options: &Options, + progress_bar: Option<&ProgressBar>, +) -> Option { let parent = path.parent()?; let dir_name = path.file_name()?; @@ -59,6 +72,10 @@ pub fn safe_remove_empty_dir(path: &Path, options: &Options) -> Option { match dir_fd.unlink_at(dir_name, true) { Ok(_) => { + // Update progress bar for directory removal + if let Some(pb) = progress_bar { + pb.inc(1); + } verbose_removed_directory(path, options); Some(false) } @@ -172,12 +189,16 @@ pub fn remove_dir_with_special_cases(path: &Path, options: &Options, error_occur } } -pub fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool { +pub fn safe_remove_dir_recursive( + path: &Path, + options: &Options, + progress_bar: Option<&ProgressBar>, +) -> bool { // Base case 1: this is a file or a symbolic link. // Use lstat to avoid race condition between check and use match fs::symlink_metadata(path) { Ok(metadata) if !metadata.is_dir() => { - return remove_file(path, options); + return remove_file(path, options, progress_bar); } Ok(_) => {} Err(e) => { diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 3309ab006..a20a57d7f 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -7,9 +7,10 @@ use clap::builder::{PossibleValue, ValueParser}; use clap::{Arg, ArgAction, Command, parser::ValueSource}; +use indicatif::{ProgressBar, ProgressStyle}; use std::ffi::{OsStr, OsString}; use std::fs::{self, Metadata}; -use std::io::{IsTerminal, stdin}; +use std::io::{self, IsTerminal, stdin}; use std::ops::BitOr; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; @@ -70,7 +71,7 @@ fn verbose_removed_directory(path: &Path, options: &Options) { /// Helper function to show error with context and return error status fn show_removal_error(error: std::io::Error, path: &Path) -> bool { - if error.kind() == std::io::ErrorKind::PermissionDenied { + if error.kind() == io::ErrorKind::PermissionDenied { show_error!("cannot remove {}: Permission denied", path.quote()); } else { let e = @@ -158,6 +159,8 @@ pub struct Options { pub dir: bool, /// `-v`, `--verbose` pub verbose: bool, + /// `-g`, `--progress` + pub progress: bool, #[doc(hidden)] /// `---presume-input-tty` /// Always use `None`; GNU flag for testing use only @@ -174,6 +177,7 @@ impl Default for Options { recursive: false, dir: false, verbose: false, + progress: false, __presume_input_tty: None, } } @@ -189,6 +193,7 @@ static OPT_PROMPT_ALWAYS: &str = "prompt-always"; static OPT_PROMPT_ONCE: &str = "prompt-once"; static OPT_RECURSIVE: &str = "recursive"; static OPT_VERBOSE: &str = "verbose"; +static OPT_PROGRESS: &str = "progress"; static PRESUME_INPUT_TTY: &str = "-presume-input-tty"; static ARG_FILES: &str = "files"; @@ -241,6 +246,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { recursive: matches.get_flag(OPT_RECURSIVE), dir: matches.get_flag(OPT_DIR), verbose: matches.get_flag(OPT_VERBOSE), + progress: matches.get_flag(OPT_PROGRESS), __presume_input_tty: if matches.get_flag(PRESUME_INPUT_TTY) { Some(true) } else { @@ -359,6 +365,13 @@ pub fn uu_app() -> Command { .help(translate!("rm-help-verbose")) .action(ArgAction::SetTrue), ) + .arg( + Arg::new(OPT_PROGRESS) + .short('g') + .long(OPT_PROGRESS) + .help(translate!("rm-help-progress")) + .action(ArgAction::SetTrue), + ) // From the GNU source code: // This is solely for testing. // Do not document. @@ -383,6 +396,70 @@ pub fn uu_app() -> Command { ) } +/// Creates a progress bar for rm operations if conditions are met. +/// Returns Some(ProgressBar) if `total_files` > 0, None otherwise. +fn create_progress_bar(files: &[&OsStr], recursive: bool) -> Option { + let total_files = count_files(files, recursive); + if total_files == 0 { + return None; + } + + Some( + ProgressBar::new(total_files) + .with_style( + ProgressStyle::with_template( + "{msg}: [{elapsed_precise}] {wide_bar} {pos:>7}/{len:7} files", + ) + .unwrap(), + ) + .with_message(translate!("rm-progress-removing")), + ) +} + +/// Count the total number of files and directories to be deleted. +/// This function recursively counts all files and directories that will be processed. +/// Files are not deduplicated when appearing in multiple sources. If `recursive` is set to `false`, the +/// directories in `paths` will be ignored. +fn count_files(paths: &[&OsStr], recursive: bool) -> u64 { + let mut total = 0; + for p in paths { + let path = Path::new(p); + if let Ok(md) = fs::symlink_metadata(path) { + if md.is_dir() && !is_symlink_dir(&md) { + if recursive { + total += count_files_in_directory(path); + } + } else { + total += 1; + } + } + // If we can't access the file, skip it for counting + // This matches the behavior where -f suppresses errors for missing files + } + total +} + +/// A helper for `count_files` specialized for directories. +fn count_files_in_directory(p: &Path) -> u64 { + let entries_count = fs::read_dir(p) + .map(|entries| { + entries + .filter_map(Result::ok) + .map(|entry| { + let file_type = entry.file_type(); + match file_type { + Ok(ft) if ft.is_dir() => count_files_in_directory(&entry.path()), + Ok(_) => 1, + Err(_) => 0, + } + }) + .sum() + }) + .unwrap_or(0); + + 1 + entries_count +} + // TODO: implement one-file-system (this may get partially implemented in walkdir) /// Remove (or unlink) the given files /// @@ -393,17 +470,27 @@ pub fn uu_app() -> Command { pub fn remove(files: &[&OsStr], options: &Options) -> bool { let mut had_err = false; + // Check if any files actually exist before creating progress bar + let mut progress_bar: Option = None; + let mut any_files_processed = false; + for filename in files { let file = Path::new(filename); had_err = match file.symlink_metadata() { Ok(metadata) => { + // Create progress bar on first successful file metadata read + if options.progress && progress_bar.is_none() { + progress_bar = create_progress_bar(files, options.recursive); + } + + any_files_processed = true; if metadata.is_dir() { - handle_dir(file, options) + handle_dir(file, options, progress_bar.as_ref()) } else if is_symlink_dir(&metadata) { - remove_dir(file, options) + remove_dir(file, options, progress_bar.as_ref()) } else { - remove_file(file, options) + remove_file(file, options, progress_bar.as_ref()) } } @@ -426,6 +513,13 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { .bitor(had_err); } + // Only finish progress bar if it was created and files were processed + if let Some(pb) = progress_bar { + if any_files_processed { + pb.finish(); + } + } + had_err } @@ -487,7 +581,11 @@ fn is_writable(_path: &Path) -> bool { /// directory, remove all of its entries recursively and then remove the /// directory itself. In case of an error, print the error message to /// `stderr` and return `true`. If there were no errors, return `false`. -fn remove_dir_recursive(path: &Path, options: &Options) -> bool { +fn remove_dir_recursive( + path: &Path, + options: &Options, + progress_bar: Option<&ProgressBar>, +) -> bool { // Base case 1: this is a file or a symbolic link. // // The symbolic link case is important because it could be a link to @@ -495,7 +593,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { // avoids an infinite recursion in the case of a link to the current // directory, like `ln -s . link`. if !path.is_dir() || path.is_symlink() { - return remove_file(path, options); + return remove_file(path, options, progress_bar); } // Base case 2: this is a non-empty directory, but the user @@ -510,7 +608,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { // Use secure traversal on Linux for all recursive directory removals #[cfg(target_os = "linux")] { - safe_remove_dir_recursive(path, options) + safe_remove_dir_recursive(path, options, progress_bar) } // Fallback for non-Linux or use fs::remove_dir_all for very long paths @@ -534,7 +632,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { // Recursive case: this is a directory. let mut error = false; match fs::read_dir(path) { - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + Err(e) if e.kind() == io::ErrorKind::PermissionDenied => { // This is not considered an error. } Err(_) => error = true, @@ -543,7 +641,8 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { match entry { Err(_) => error = true, Ok(entry) => { - let child_error = remove_dir_recursive(&entry.path(), options); + let child_error = + remove_dir_recursive(&entry.path(), options, progress_bar); error = error || child_error; } } @@ -585,7 +684,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { } } -fn handle_dir(path: &Path, options: &Options) -> bool { +fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { let mut had_err = false; let path = clean_trailing_slashes(path); @@ -599,9 +698,9 @@ fn handle_dir(path: &Path, options: &Options) -> bool { let is_root = path.has_root() && path.parent().is_none(); if options.recursive && (!is_root || !options.preserve_root) { - had_err = remove_dir_recursive(path, options); + had_err = remove_dir_recursive(path, options, progress_bar); } else if options.dir && (!is_root || !options.preserve_root) { - had_err = remove_dir(path, options).bitor(had_err); + had_err = remove_dir(path, options, progress_bar).bitor(had_err); } else if options.recursive { show_error!("{}", RmError::DangerousRecursiveOperation); show_error!("{}", RmError::UseNoPreserveRoot); @@ -620,7 +719,7 @@ fn handle_dir(path: &Path, options: &Options) -> bool { /// Remove the given directory, asking the user for permission if necessary. /// /// Returns true if it has encountered an error. -fn remove_dir(path: &Path, options: &Options) -> bool { +fn remove_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { // Ask the user for permission. if !prompt_dir(path, options) { return false; @@ -638,21 +737,31 @@ fn remove_dir(path: &Path, options: &Options) -> bool { // Use safe traversal on Linux for empty directory removal #[cfg(target_os = "linux")] { - if let Some(result) = safe_remove_empty_dir(path, options) { + if let Some(result) = safe_remove_empty_dir(path, options, progress_bar) { return result; } } + // Update progress bar for directory removal + if let Some(pb) = progress_bar { + pb.inc(1); + } + // Fallback method for non-Linux or when safe traversal is unavailable remove_dir_with_feedback(path, options) } -fn remove_file(path: &Path, options: &Options) -> bool { +fn remove_file(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { if prompt_file(path, options) { + // Update progress bar before removing the file + if let Some(pb) = progress_bar { + pb.inc(1); + } + // Use safe traversal on Linux for individual file removal #[cfg(target_os = "linux")] { - if let Some(result) = safe_remove_file(path, options) { + if let Some(result) = safe_remove_file(path, options, progress_bar) { return result; } } @@ -663,7 +772,7 @@ fn remove_file(path: &Path, options: &Options) -> bool { verbose_removed_file(path, options); } Err(e) => { - if e.kind() == std::io::ErrorKind::PermissionDenied { + if e.kind() == io::ErrorKind::PermissionDenied { // GNU compatibility (rm/fail-eacces.sh) show_error!( "{}", diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 9f8803865..38230f2ad 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1149,3 +1149,71 @@ fn test_rm_directory_not_writable() { assert!(!at.dir_exists("b/c")); // Should be removed assert!(!at.dir_exists("b/d")); // Should be removed } + +#[test] +fn test_progress_flag_short() { + let (at, mut ucmd) = at_and_ucmd!(); + let file = "test_rm_progress_file"; + + at.touch(file); + + // Test that -g flag is accepted + ucmd.arg("-g").arg(file).succeeds(); + + assert!(!at.file_exists(file)); +} + +#[test] +fn test_progress_flag_long() { + let (at, mut ucmd) = at_and_ucmd!(); + let file = "test_rm_progress_file"; + + at.touch(file); + + // Test that --progress flag is accepted + ucmd.arg("--progress").arg(file).succeeds(); + + assert!(!at.file_exists(file)); +} + +#[test] +fn test_progress_with_recursive() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("test_dir"); + at.touch("test_dir/file1"); + at.touch("test_dir/file2"); + at.mkdir("test_dir/subdir"); + at.touch("test_dir/subdir/file3"); + + // Test progress with recursive removal + ucmd.arg("-rg").arg("test_dir").succeeds(); + + assert!(!at.dir_exists("test_dir")); +} + +#[test] +fn test_progress_with_verbose() { + let (at, mut ucmd) = at_and_ucmd!(); + let file = "test_rm_progress_verbose_file"; + + at.touch(file); + + // Test that progress and verbose work together + ucmd.arg("-gv").arg(file).succeeds().stdout_contains(file); + + assert!(!at.file_exists(file)); +} + +#[test] +fn test_progress_no_output_on_error() { + let nonexistent_file = "this_file_does_not_exist"; + + // Test that progress bar is not shown when file doesn't exist + new_ucmd!() + .arg("--progress") + .arg(nonexistent_file) + .fails() + .stderr_contains("cannot remove") + .stderr_contains("No such file or directory"); +}