From 31dd9eb9234d42205e6984b6dea568085c6fd792 Mon Sep 17 00:00:00 2001 From: Talison Fabio <54823205+talis-fb@users.noreply.github.com> Date: Sun, 30 Mar 2025 19:19:21 -0300 Subject: [PATCH] feat: Add rename option in overwrite menu (#779) * feat: Add generic Choice prompt implementation * feat: use ChoicePrompt in user_wants_to_continue method * feat: add "rename" option in ask_to_create_file method * feat: check accessible mode for choises prompt * feat: rename file in "ask_to_create_file" rename action * feat: create "resolve_path" for smart_unpack to deal with rename * feat: use resolve_path instead clear_path in smart_unpack * fix: remove unused clear_path function * chore: cargo fmt * Add docs * refactor: rename "resolve_path" method * chore: fix ChoicePrompt doc * doc: improve doc of resolve_path_conflict * fix: out of bound when type answer bigger than some choice * doc: improve rename_path docs * chore: cargo fmt * chore: revert user_wants_to_continue * fix: update error message when find EOF in choise prompt response * revert: update message error in ChoicePrompt instead Confirmation * test: add overwrite and cancel tests * test: Add rename test with "allow_piped_choice" new feature * cargo fmt * test: create test for autoincrement new renamed files --- Cargo.toml | 1 + src/commands/decompress.rs | 10 ++- src/utils/fs.rs | 67 ++++++++++++--- src/utils/mod.rs | 9 +- src/utils/question.rs | 169 ++++++++++++++++++++++++++++++++++--- tests/integration.rs | 169 +++++++++++++++++++++++++++++++++++++ 6 files changed, 393 insertions(+), 32 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4192a77..a9f3ba8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,6 +73,7 @@ test-strategy = "0.4.0" default = ["use_zlib", "use_zstd_thin", "unrar"] use_zlib = ["flate2/zlib", "gzp/deflate_zlib", "zip/deflate-zlib"] use_zstd_thin = ["zstd/thin"] +allow_piped_choice = [] # For generating binaries for releases [profile.release] diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 6d2efee..b14dc3c 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -309,7 +309,7 @@ fn smart_unpack( let root_contains_only_one_element = fs::read_dir(temp_dir_path)?.count() == 1; - let (previous_path, new_path) = if root_contains_only_one_element { + let (previous_path, mut new_path) = if root_contains_only_one_element { // Only one file in the root directory, so we can just move it to the output directory let file = fs::read_dir(temp_dir_path)?.next().expect("item exists")?; let file_path = file.path(); @@ -324,9 +324,11 @@ fn smart_unpack( }; // Before moving, need to check if a file with the same name already exists - if !utils::clear_path(&new_path, question_policy)? { - return Ok(ControlFlow::Break(())); - } + // If it does, need to ask the user what to do + new_path = match utils::resolve_path_conflict(&new_path, question_policy)? { + Some(path) => path, + None => return Ok(ControlFlow::Break(())), + }; // Rename the temporary directory to the archive name, which is output_file_path fs::rename(&previous_path, &new_path)?; diff --git a/src/utils/fs.rs b/src/utils/fs.rs index d33411a..c6ffb8a 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -8,7 +8,7 @@ use std::{ use fs_err as fs; -use super::user_wants_to_overwrite; +use super::{question::FileConflitOperation, user_wants_to_overwrite}; use crate::{ extension::Extension, utils::{logger::info_accessible, EscapedPathDisplay}, @@ -19,19 +19,29 @@ pub fn is_path_stdin(path: &Path) -> bool { path.as_os_str() == "-" } -/// Remove `path` asking the user to overwrite if necessary. +/// Check if &Path exists, if it does then ask the user if they want to overwrite or rename it. +/// If the user want to overwrite then the file or directory will be removed and returned the same input path +/// If the user want to rename then nothing will be removed and a new path will be returned with a new name /// -/// * `Ok(true)` means the path is clear, -/// * `Ok(false)` means the user doesn't want to overwrite +/// * `Ok(None)` means the user wants to cancel the operation +/// * `Ok(Some(path))` returns a valid PathBuf without any another file or directory with the same name /// * `Err(_)` is an error -pub fn clear_path(path: &Path, question_policy: QuestionPolicy) -> crate::Result { - if path.exists() && !user_wants_to_overwrite(path, question_policy)? { - return Ok(false); +pub fn resolve_path_conflict(path: &Path, question_policy: QuestionPolicy) -> crate::Result> { + if path.exists() { + match user_wants_to_overwrite(path, question_policy)? { + FileConflitOperation::Cancel => Ok(None), + FileConflitOperation::Overwrite => { + remove_file_or_dir(path)?; + Ok(Some(path.to_path_buf())) + } + FileConflitOperation::Rename => { + let renamed_path = rename_for_available_filename(path); + Ok(Some(renamed_path)) + } + } + } else { + Ok(Some(path.to_path_buf())) } - - remove_file_or_dir(path)?; - - Ok(true) } pub fn remove_file_or_dir(path: &Path) -> crate::Result<()> { @@ -43,6 +53,41 @@ pub fn remove_file_or_dir(path: &Path) -> crate::Result<()> { Ok(()) } +/// Create a new path renaming the "filename" from &Path for a available name in the same directory +pub fn rename_for_available_filename(path: &Path) -> PathBuf { + let mut renamed_path = rename_or_increment_filename(path); + while renamed_path.exists() { + renamed_path = rename_or_increment_filename(&renamed_path); + } + renamed_path +} + +/// Create a new path renaming the "filename" from &Path to `filename_1` +/// if its name already ends with `_` and some number, then it increments the number +/// Example: +/// - `file.txt` -> `file_1.txt` +/// - `file_1.txt` -> `file_2.txt` +pub fn rename_or_increment_filename(path: &Path) -> PathBuf { + let parent = path.parent().unwrap_or_else(|| Path::new("")); + let filename = path.file_stem().and_then(|s| s.to_str()).unwrap_or(""); + let extension = path.extension().and_then(|s| s.to_str()).unwrap_or(""); + + let new_filename = match filename.rsplit_once('_') { + Some((base, number_str)) if number_str.chars().all(char::is_numeric) => { + let number = number_str.parse::().unwrap_or(0); + format!("{}_{}", base, number + 1) + } + _ => format!("{}_1", filename), + }; + + let mut new_path = parent.join(new_filename); + if !extension.is_empty() { + new_path.set_extension(extension); + } + + new_path +} + /// Creates a directory at the path, if there is nothing there. pub fn create_dir_if_non_existent(path: &Path) -> crate::Result<()> { if !path.exists() { diff --git a/src/utils/mod.rs b/src/utils/mod.rs index c153689..444cf1f 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -18,10 +18,13 @@ pub use self::{ EscapedPathDisplay, }, fs::{ - cd_into_same_dir_as, clear_path, create_dir_if_non_existent, is_path_stdin, remove_file_or_dir, - try_infer_extension, + cd_into_same_dir_as, create_dir_if_non_existent, is_path_stdin, remove_file_or_dir, + rename_for_available_filename, resolve_path_conflict, try_infer_extension, + }, + question::{ + ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, FileConflitOperation, QuestionAction, + QuestionPolicy, }, - question::{ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, QuestionAction, QuestionPolicy}, utf8::{get_invalid_utf8_paths, is_invalid_utf8}, }; diff --git a/src/utils/question.rs b/src/utils/question.rs index ee36e74..27eb7bb 100644 --- a/src/utils/question.rs +++ b/src/utils/question.rs @@ -37,31 +37,69 @@ pub enum QuestionAction { Decompression, } +#[derive(Default)] +/// Determines which action to do when there is a file conflict +pub enum FileConflitOperation { + #[default] + /// Cancel the operation + Cancel, + /// Overwrite the existing file with the new one + Overwrite, + /// Rename the file + /// It'll be put "_1" at the end of the filename or "_2","_3","_4".. if already exists + Rename, +} + /// Check if QuestionPolicy flags were set, otherwise, ask user if they want to overwrite. -pub fn user_wants_to_overwrite(path: &Path, question_policy: QuestionPolicy) -> crate::Result { +pub fn user_wants_to_overwrite(path: &Path, question_policy: QuestionPolicy) -> crate::Result { + use FileConflitOperation as Op; + match question_policy { - QuestionPolicy::AlwaysYes => Ok(true), - QuestionPolicy::AlwaysNo => Ok(false), - QuestionPolicy::Ask => { - let path = path_to_str(strip_cur_dir(path)); - let path = Some(&*path); - let placeholder = Some("FILE"); - Confirmation::new("Do you want to overwrite 'FILE'?", placeholder).ask(path) - } + QuestionPolicy::AlwaysYes => Ok(Op::Overwrite), + QuestionPolicy::AlwaysNo => Ok(Op::Cancel), + QuestionPolicy::Ask => ask_file_conflict_operation(path), } } +/// Ask the user if they want to overwrite or rename the &Path +pub fn ask_file_conflict_operation(path: &Path) -> Result { + use FileConflitOperation as Op; + + let path = path_to_str(strip_cur_dir(path)); + + ChoicePrompt::new( + format!("Do you want to overwrite {path}?"), + [ + ("yes", Op::Overwrite, *colors::GREEN), + ("no", Op::Cancel, *colors::RED), + ("rename", Op::Rename, *colors::BLUE), + ], + ) + .ask() +} + /// Create the file if it doesn't exist and if it does then ask to overwrite it. /// If the user doesn't want to overwrite then we return [`Ok(None)`] pub fn ask_to_create_file(path: &Path, question_policy: QuestionPolicy) -> Result> { match fs::OpenOptions::new().write(true).create_new(true).open(path) { Ok(w) => Ok(Some(w)), Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { - if user_wants_to_overwrite(path, question_policy)? { - utils::remove_file_or_dir(path)?; - Ok(Some(fs::File::create(path)?)) - } else { - Ok(None) + let action = match question_policy { + QuestionPolicy::AlwaysYes => FileConflitOperation::Overwrite, + QuestionPolicy::AlwaysNo => FileConflitOperation::Cancel, + QuestionPolicy::Ask => ask_file_conflict_operation(path)?, + }; + + match action { + FileConflitOperation::Overwrite => { + utils::remove_file_or_dir(path)?; + Ok(Some(fs::File::create(path)?)) + } + FileConflitOperation::Cancel => Ok(None), + FileConflitOperation::Rename => { + let renamed_file_path = utils::rename_for_available_filename(path); + Ok(Some(fs::File::create(renamed_file_path)?)) + } } } Err(e) => Err(Error::from(e)), @@ -90,6 +128,108 @@ pub fn user_wants_to_continue( } } +/// Choise dialog for end user with [option1/option2/...] question. +/// Each option is a [Choice] entity, holding a value "T" returned when that option is selected +pub struct ChoicePrompt<'a, T: Default> { + /// The message to be displayed before the options + /// e.g.: "Do you want to overwrite 'FILE'?" + pub prompt: String, + + pub choises: Vec>, +} + +/// A single choice showed as a option to user in a [ChoicePrompt] +/// It holds a label and a color to display to user and a real value to be returned +pub struct Choice<'a, T: Default> { + label: &'a str, + value: T, + color: &'a str, +} + +impl<'a, T: Default> ChoicePrompt<'a, T> { + /// Creates a new Confirmation. + pub fn new(prompt: impl Into, choises: impl IntoIterator) -> Self { + Self { + prompt: prompt.into(), + choises: choises + .into_iter() + .map(|(label, value, color)| Choice { label, value, color }) + .collect(), + } + } + + /// Creates user message and receives a input to be compared with choises "label" + /// and returning the real value of the choise selected + pub fn ask(mut self) -> crate::Result { + let message = self.prompt; + + #[cfg(not(feature = "allow_piped_choice"))] + if !stdin().is_terminal() { + eprintln!("{}", message); + eprintln!("Pass --yes to proceed"); + return Ok(T::default()); + } + + let _locks = lock_and_flush_output_stdio()?; + let mut stdin_lock = stdin().lock(); + + // Ask the same question to end while no valid answers are given + loop { + let choice_prompt = if is_running_in_accessible_mode() { + self.choises + .iter() + .map(|choise| format!("{}{}{}", choise.color, choise.label, *colors::RESET)) + .collect::>() + .join("/") + } else { + let choises = self + .choises + .iter() + .map(|choise| { + format!( + "{}{}{}", + choise.color, + choise + .label + .chars() + .nth(0) + .expect("dev error, should be reported, we checked this won't happen"), + *colors::RESET + ) + }) + .collect::>() + .join("/"); + + format!("[{}]", choises) + }; + + eprintln!("{} {}", message, choice_prompt); + + let mut answer = String::new(); + let bytes_read = stdin_lock.read_line(&mut answer)?; + + if bytes_read == 0 { + let error = FinalError::with_title("Unexpected EOF when asking question.") + .detail("When asking the user:") + .detail(format!(" \"{message}\"")) + .detail("Expected one of the options as answer, but found EOF instead.") + .hint("If using Ouch in scripting, consider using `--yes` and `--no`."); + + return Err(error.into()); + } + + answer.make_ascii_lowercase(); + let answer = answer.trim(); + + let chosen_index = self.choises.iter().position(|choise| choise.label.starts_with(answer)); + + if let Some(i) = chosen_index { + return Ok(self.choises.remove(i).value); + } + } + } +} + /// Confirmation dialog for end user with [Y/n] question. /// /// If the placeholder is found in the prompt text, it will be replaced to form the final message. @@ -120,6 +260,7 @@ impl<'a> Confirmation<'a> { (Some(placeholder), Some(subs)) => Cow::Owned(self.prompt.replace(placeholder, subs)), }; + #[cfg(not(feature = "allow_piped_choice"))] if !stdin().is_terminal() { eprintln!("{}", message); eprintln!("Pass --yes to proceed"); diff --git a/tests/integration.rs b/tests/integration.rs index 7241cd1..0ff7e1d 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -194,6 +194,175 @@ fn multiple_files( assert_same_directory(before, after, !matches!(ext, DirectoryExtension::Zip)); } +#[proptest(cases = 25)] +fn multiple_files_with_conflict_and_choice_to_overwrite( + ext: DirectoryExtension, + #[any(size_range(0..1).lift())] extra_extensions: Vec, + #[strategy(0u8..3)] depth: u8, +) { + let dir = tempdir().unwrap(); + let dir = dir.path(); + + let before = &dir.join("before"); + let before_dir = &before.join("dir"); + fs::create_dir_all(before_dir).unwrap(); + create_random_files(before_dir, depth, &mut SmallRng::from_entropy()); + + let after = &dir.join("after"); + let after_dir = &after.join("dir"); + fs::create_dir_all(after_dir).unwrap(); + create_random_files(after_dir, depth, &mut SmallRng::from_entropy()); + + let archive = &dir.join(format!("archive.{}", merge_extensions(&ext, extra_extensions))); + ouch!("-A", "c", before_dir, archive); + + crate::utils::cargo_bin() + .arg("decompress") + .arg(archive) + .arg("-d") + .arg(after) + .arg("--yes") + .assert() + .success(); + + assert_same_directory(before, after, false); +} + +#[proptest(cases = 25)] +fn multiple_files_with_conflict_and_choice_to_not_overwrite( + ext: DirectoryExtension, + #[any(size_range(0..1).lift())] extra_extensions: Vec, + #[strategy(0u8..3)] depth: u8, +) { + let dir = tempdir().unwrap(); + let dir = dir.path(); + + let before = &dir.join("before"); + let before_dir = &before.join("dir"); + fs::create_dir_all(before_dir).unwrap(); + create_random_files(before_dir, depth, &mut SmallRng::from_entropy()); + + let after = &dir.join("after"); + let after_dir = &after.join("dir"); + fs::create_dir_all(after_dir).unwrap(); + + let after_backup = &dir.join("after_backup"); + let after_backup_dir = &after_backup.join("dir"); + fs::create_dir_all(after_backup_dir).unwrap(); + + // Create a file with the same name as one of the files in the after directory + fs::write(after_dir.join("something.txt"), "Some content").unwrap(); + fs::copy(after_dir.join("something.txt"), after_backup_dir.join("something.txt")).unwrap(); + + let archive = &dir.join(format!("archive.{}", merge_extensions(&ext, extra_extensions))); + ouch!("-A", "c", before_dir, archive); + + crate::utils::cargo_bin() + .arg("decompress") + .arg(archive) + .arg("-d") + .arg(after) + .arg("--no") + .assert() + .success(); + + assert_same_directory(after, after_backup, false); +} + +#[cfg(feature = "allow_piped_choice")] +#[proptest(cases = 25)] +fn multiple_files_with_conflict_and_choice_to_rename( + ext: DirectoryExtension, + #[any(size_range(0..1).lift())] extra_extensions: Vec, + #[strategy(0u8..3)] depth: u8, +) { + let dir = tempdir().unwrap(); + let dir = dir.path(); + + let before = &dir.join("before"); + let before_dir = &before.join("dir"); + fs::create_dir_all(before_dir).unwrap(); + create_random_files(before_dir, depth, &mut SmallRng::from_entropy()); + + let after = &dir.join("after"); + let after_dir = &after.join("dir"); + fs::create_dir_all(after_dir).unwrap(); + create_random_files(after_dir, depth, &mut SmallRng::from_entropy()); + + let archive = &dir.join(format!("archive.{}", merge_extensions(&ext, extra_extensions))); + ouch!("-A", "c", before_dir, archive); + + let after_renamed_dir = &after.join("dir_1"); + assert_eq!(false, after_renamed_dir.exists()); + + crate::utils::cargo_bin() + .arg("decompress") + .arg(archive) + .arg("-d") + .arg(after) + .write_stdin("r") + .assert() + .success(); + + assert_same_directory(before_dir, after_renamed_dir, false); +} + +#[cfg(feature = "allow_piped_choice")] +#[proptest(cases = 25)] +fn multiple_files_with_conflict_and_choice_to_rename_with_already_a_renamed( + ext: DirectoryExtension, + #[any(size_range(0..1).lift())] extra_extensions: Vec, + #[strategy(0u8..3)] depth: u8, +) { + let dir = tempdir().unwrap(); + let dir = dir.path(); + + let before = &dir.join("before"); + let before_dir = &before.join("dir"); + fs::create_dir_all(before_dir).unwrap(); + create_random_files(before_dir, depth, &mut SmallRng::from_entropy()); + + let after = &dir.join("after"); + let after_dir = &after.join("dir"); + fs::create_dir_all(after_dir).unwrap(); + create_random_files(after_dir, depth, &mut SmallRng::from_entropy()); + + let archive = &dir.join(format!("archive.{}", merge_extensions(&ext, extra_extensions))); + ouch!("-A", "c", before_dir, archive); + + let already_renamed_dir = &after.join("dir_1"); + fs::create_dir_all(already_renamed_dir).unwrap(); + create_random_files(already_renamed_dir, depth, &mut SmallRng::from_entropy()); + + let after_real_renamed_dir = &after.join("dir_2"); + assert_eq!(false, after_real_renamed_dir.exists()); + + crate::utils::cargo_bin() + .arg("decompress") + .arg(archive) + .arg("-d") + .arg(after) + .write_stdin("r") + .assert() + .success(); + + assert_same_directory(before_dir, after_real_renamed_dir, false); + + let after_another_real_renamed_dir = &after.join("dir_3"); + assert_eq!(false, after_another_real_renamed_dir.exists()); + + crate::utils::cargo_bin() + .arg("decompress") + .arg(archive) + .arg("-d") + .arg(after) + .write_stdin("r") + .assert() + .success(); + + assert_same_directory(before_dir, after_another_real_renamed_dir, false); +} + #[cfg(feature = "unrar")] #[test] fn unpack_rar() -> Result<(), Box> {