From dd34293ce8e5a0858f2cdf44e4be5e9ac0b6ffbb Mon Sep 17 00:00:00 2001 From: tommady Date: Sun, 18 May 2025 06:00:51 +0000 Subject: [PATCH 01/13] scaffold close-issue-825 Signed-off-by: tommady --- src/commands/decompress.rs | 75 ++++++++++++++++++++++++++++++-------- src/main.rs | 2 +- src/utils/fs.rs | 34 +---------------- src/utils/mod.rs | 6 +-- src/utils/question.rs | 37 ++++++++++++++++++- 5 files changed, 101 insertions(+), 53 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 6c254ad..059bbd3 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -1,4 +1,5 @@ use std::{ + collections::HashMap, io::{self, BufReader, Read}, ops::ControlFlow, path::{Path, PathBuf}, @@ -22,7 +23,7 @@ use crate::{ logger::{info, info_accessible}, nice_directory_display, user_wants_to_continue, }, - QuestionAction, QuestionPolicy, BUFFER_CAPACITY, + FileConflitOperation, QuestionAction, QuestionPolicy, BUFFER_CAPACITY, }; trait ReadSeek: Read + io::Seek {} @@ -341,16 +342,25 @@ fn unpack( let is_valid_output_dir = !output_dir.exists() || (output_dir.is_dir() && output_dir.read_dir()?.next().is_none()); let output_dir_cleaned = if is_valid_output_dir { - output_dir.to_owned() + output_dir } else { - match utils::resolve_path_conflict(output_dir, question_policy, QuestionAction::Decompression)? { - Some(path) => path, - None => return Ok(ControlFlow::Break(())), + // TODO: will enhance later + match utils::check_conflics_and_ask_user(output_dir, question_policy, QuestionAction::Decompression)? { + FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), + FileConflitOperation::Overwrite => { + // TODO: issue 820 could try to enhance here to fix the issue + // https://github.com/ouch-org/ouch/issues/820 + utils::remove_file_or_dir(output_dir)?; + output_dir + } + FileConflitOperation::Rename => &utils::rename_for_available_filename(output_dir), + FileConflitOperation::Merge => output_dir, + FileConflitOperation::GoodToGo => output_dir, } }; if !output_dir_cleaned.exists() { - fs::create_dir(&output_dir_cleaned)?; + fs::create_dir(output_dir_cleaned)?; } let files = unpack_fn(&output_dir_cleaned)?; @@ -383,7 +393,7 @@ fn smart_unpack( let root_contains_only_one_element = fs::read_dir(temp_dir_path)?.take(2).count() == 1; - let (previous_path, mut new_path) = if root_contains_only_one_element { + let (previous_path, 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(); @@ -397,15 +407,50 @@ fn smart_unpack( (temp_dir_path.to_owned(), output_file_path.to_owned()) }; - // Before moving, need to check if a file with the same name already exists - // If it does, need to ask the user what to do - new_path = match utils::resolve_path_conflict(&new_path, question_policy, QuestionAction::Decompression)? { - Some(path) => path, - None => return Ok(ControlFlow::Break(())), - }; + // TODO: will enhance later + match utils::check_conflics_and_ask_user(&new_path, question_policy, QuestionAction::Decompression)? { + FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), + FileConflitOperation::GoodToGo => { + fs::rename(&previous_path, &new_path)?; + } + FileConflitOperation::Overwrite => { + // TODO: issue 820 could try to enhance here to fix the issue + // https://github.com/ouch-org/ouch/issues/820 + utils::remove_file_or_dir(&new_path)?; + fs::rename(&previous_path, &new_path)?; + } + FileConflitOperation::Rename => { + fs::rename(&previous_path, utils::rename_for_available_filename(&new_path))?; + } + FileConflitOperation::Merge => { + // TODO: just a simple way to verify, will enhance later + let mut seen = HashMap::new(); + for entry in fs::read_dir(&new_path)? { + let entry = entry?; + let path = entry.path(); + let name = path + .file_name() + .expect("The paths were read by the program, so it should be safe."); + + seen.insert(name.to_os_string(), path); + } + + for entry in fs::read_dir(&previous_path)? { + let entry = entry?; + let path = entry.path(); + let name = path + .file_name() + .expect("The paths were read by the program, so it should be safe."); + + if seen.contains_key(&name.to_os_string()) { + continue; + } + + fs::copy(&path, &new_path.join(name))?; + } + } + } - // Rename the temporary directory to the archive name, which is output_file_path - fs::rename(&previous_path, &new_path)?; info_accessible(format!( "Successfully moved \"{}\" to \"{}\"", nice_directory_display(&previous_path), diff --git a/src/main.rs b/src/main.rs index 3f1f7dd..7c784d5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,7 +17,7 @@ use self::{ error::{Error, Result}, utils::{ logger::{shutdown_logger_and_wait, spawn_logger_thread}, - QuestionAction, QuestionPolicy, + FileConflitOperation, QuestionAction, QuestionPolicy, }, }; diff --git a/src/utils/fs.rs b/src/utils/fs.rs index f65f152..faa50ba 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -8,47 +8,15 @@ use std::{ use fs_err as fs; -use super::{question::FileConflitOperation, user_wants_to_overwrite}; use crate::{ extension::Extension, - utils::{logger::info_accessible, EscapedPathDisplay, QuestionAction}, - QuestionPolicy, + utils::{logger::info_accessible, EscapedPathDisplay}, }; pub fn is_path_stdin(path: &Path) -> bool { path.as_os_str() == "-" } -/// 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(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 resolve_path_conflict( - path: &Path, - question_policy: QuestionPolicy, - question_action: QuestionAction, -) -> crate::Result> { - if path.exists() { - match user_wants_to_overwrite(path, question_policy, question_action)? { - 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)) - } - FileConflitOperation::Merge => Ok(Some(path.to_path_buf())), - } - } else { - Ok(Some(path.to_path_buf())) - } -} - pub fn remove_file_or_dir(path: &Path) -> crate::Result<()> { if path.is_dir() { fs::remove_dir_all(path)?; diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 444cf1f..d157c9f 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -19,11 +19,11 @@ pub use self::{ }, fs::{ 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, + rename_for_available_filename, try_infer_extension, }, question::{ - ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, FileConflitOperation, QuestionAction, - QuestionPolicy, + ask_to_create_file, check_conflics_and_ask_user, user_wants_to_continue, user_wants_to_overwrite, + FileConflitOperation, QuestionAction, QuestionPolicy, }, utf8::{get_invalid_utf8_paths, is_invalid_utf8}, }; diff --git a/src/utils/question.rs b/src/utils/question.rs index 4b4c97e..5389342 100644 --- a/src/utils/question.rs +++ b/src/utils/question.rs @@ -50,6 +50,41 @@ pub enum FileConflitOperation { Rename, /// Merge conflicting folders Merge, + /// There is no conflict, good to go + GoodToGo, +} + +/// Check if &Path exists, if it does then ask the user what to do. +/// 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 +/// If the user want to merge then the conflics file or directory will be skiped +/// +/// * `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 check_conflics_and_ask_user( + path: &Path, + question_policy: QuestionPolicy, + question_action: QuestionAction, +) -> crate::Result { + if path.exists() { + user_wants_to_overwrite(path, question_policy, question_action) + // match user_wants_to_overwrite(path, question_policy, question_action)? { + // 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)) + // } + // FileConflitOperation::Merge => Ok(Some(path.to_path_buf())), + // } + } else { + Ok(FileConflitOperation::GoodToGo) + // Ok(Some(path.to_path_buf())) + } } /// Check if QuestionPolicy flags were set, otherwise, ask user if they want to overwrite. @@ -112,7 +147,7 @@ pub fn ask_to_create_file( }; match action { - FileConflitOperation::Merge => Ok(Some(fs::File::create(path)?)), + FileConflitOperation::Merge | FileConflitOperation::GoodToGo => Ok(Some(fs::File::create(path)?)), FileConflitOperation::Overwrite => { utils::remove_file_or_dir(path)?; Ok(Some(fs::File::create(path)?)) From 0c4d4ff916b2f9159dbf7aebf9cd5a978cc3693a Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 22 May 2025 06:31:56 +0000 Subject: [PATCH 02/13] fix linter and enhance merge test Signed-off-by: tommady --- src/commands/decompress.rs | 6 +-- tests/integration.rs | 79 ++++++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 059bbd3..2dd344e 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -344,7 +344,6 @@ fn unpack( let output_dir_cleaned = if is_valid_output_dir { output_dir } else { - // TODO: will enhance later match utils::check_conflics_and_ask_user(output_dir, question_policy, QuestionAction::Decompression)? { FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), FileConflitOperation::Overwrite => { @@ -363,7 +362,7 @@ fn unpack( fs::create_dir(output_dir_cleaned)?; } - let files = unpack_fn(&output_dir_cleaned)?; + let files = unpack_fn(output_dir_cleaned)?; Ok(ControlFlow::Continue(files)) } @@ -407,7 +406,6 @@ fn smart_unpack( (temp_dir_path.to_owned(), output_file_path.to_owned()) }; - // TODO: will enhance later match utils::check_conflics_and_ask_user(&new_path, question_policy, QuestionAction::Decompression)? { FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), FileConflitOperation::GoodToGo => { @@ -446,7 +444,7 @@ fn smart_unpack( continue; } - fs::copy(&path, &new_path.join(name))?; + fs::copy(&path, new_path.join(name))?; } } } diff --git a/tests/integration.rs b/tests/integration.rs index 9f870db..d98962e 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -806,7 +806,7 @@ fn no_git_folder_after_decompression_with_gitignore_flag_active() { ); } -#[cfg(feature = "allow_piped_choice")] +// #[cfg(feature = "allow_piped_choice")] #[proptest(cases = 25)] fn unpack_multiple_sources_into_the_same_destination_with_merge( ext: DirectoryExtension, @@ -814,35 +814,63 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( ) { let temp_dir = tempdir()?; let root_path = temp_dir.path(); - let source_path = root_path - .join(format!("example_{}", merge_extensions(&ext, &extra_extensions))) - .join("sub_a") - .join("sub_b") - .join("sub_c"); + let source_path = root_path.join(format!("example_{}", merge_extensions(&ext, &extra_extensions))); fs::create_dir_all(&source_path)?; + fs::File::create(source_path.join("file1.txt"))?; + fs::File::create(source_path.join("file2.txt"))?; + fs::File::create(source_path.join("file3.txt"))?; + let sub_folder_path = source_path.join("sub_a"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file1.txt"))?; + fs::File::create(sub_folder_path.join("file2.txt"))?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + let sub_folder_path = sub_folder_path.join("sub_b"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file1.txt"))?; + fs::File::create(sub_folder_path.join("file2.txt"))?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + let sub_folder_path = sub_folder_path.join("sub_c"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file1.txt"))?; + fs::File::create(sub_folder_path.join("file2.txt"))?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + let archive = root_path.join(format!("archive.{}", merge_extensions(&ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") - .args([ - fs::File::create(source_path.join("file1.txt"))?.path(), - fs::File::create(source_path.join("file2.txt"))?.path(), - fs::File::create(source_path.join("file3.txt"))?.path(), - ]) + .arg(&source_path) .arg(&archive) .assert() .success(); fs::remove_dir_all(&source_path)?; + + let source_path = root_path.join(format!("example_{}", merge_extensions(&ext, &extra_extensions))); fs::create_dir_all(&source_path)?; + fs::File::create(source_path.join("file3.txt"))?; + fs::File::create(source_path.join("file4.txt"))?; + fs::File::create(source_path.join("file5.txt"))?; + let sub_folder_path = source_path.join("sub_a"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + fs::File::create(sub_folder_path.join("file4.txt"))?; + fs::File::create(sub_folder_path.join("file5.txt"))?; + let sub_folder_path = sub_folder_path.join("sub_b"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + fs::File::create(sub_folder_path.join("file4.txt"))?; + fs::File::create(sub_folder_path.join("file5.txt"))?; + let sub_folder_path = sub_folder_path.join("sub_c"); + fs::create_dir_all(&sub_folder_path)?; + fs::File::create(sub_folder_path.join("file3.txt"))?; + fs::File::create(sub_folder_path.join("file4.txt"))?; + fs::File::create(sub_folder_path.join("file5.txt"))?; + let archive1 = root_path.join(format!("archive1.{}", merge_extensions(&ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") - .args([ - fs::File::create(source_path.join("file3.txt"))?.path(), - fs::File::create(source_path.join("file4.txt"))?.path(), - fs::File::create(source_path.join("file5.txt"))?.path(), - ]) + .arg(&source_path) .arg(&archive1) .assert() .success(); @@ -867,7 +895,24 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( .assert() .success(); - assert_eq!(5, out_path.as_path().read_dir()?.count()); + assert_eq!(20, count_files_recursively(&out_path)); +} + +fn count_files_recursively(dir: &Path) -> usize { + let mut count = 0; + // println!("{:?}", dir); + if let Ok(entries) = fs::read_dir(dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.is_file() { + // println!("{:?}", path); + count += 1; + } else if path.is_dir() { + count += count_files_recursively(&path); + } + } + } + count } #[test] From f9cf337aa51d79c2422968e7356736caeaf00de9 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 22 May 2025 06:37:38 +0000 Subject: [PATCH 03/13] enhance test Signed-off-by: tommady --- tests/integration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration.rs b/tests/integration.rs index d98962e..ef157bd 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -806,7 +806,7 @@ fn no_git_folder_after_decompression_with_gitignore_flag_active() { ); } -// #[cfg(feature = "allow_piped_choice")] +#[cfg(feature = "allow_piped_choice")] #[proptest(cases = 25)] fn unpack_multiple_sources_into_the_same_destination_with_merge( ext: DirectoryExtension, From fbefb54b0269b9cf225ba2d39f51892c9f9c21c3 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 22 May 2025 06:41:30 +0000 Subject: [PATCH 04/13] enhance test Signed-off-by: tommady --- tests/integration.rs | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index ef157bd..9087533 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -895,24 +895,22 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( .assert() .success(); - assert_eq!(20, count_files_recursively(&out_path)); -} - -fn count_files_recursively(dir: &Path) -> usize { - let mut count = 0; - // println!("{:?}", dir); - if let Ok(entries) = fs::read_dir(dir) { - for entry in entries.flatten() { - let path = entry.path(); - if path.is_file() { - // println!("{:?}", path); - count += 1; - } else if path.is_dir() { - count += count_files_recursively(&path); + fn count_files_recursively(dir: &Path) -> usize { + let mut count = 0; + if let Ok(entries) = fs::read_dir(dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.is_file() { + count += 1; + } else if path.is_dir() { + count += count_files_recursively(&path); + } } } + count } - count + + assert_eq!(20, count_files_recursively(&out_path)); } #[test] From 83003c96ed973535478fbf655c131a50d24d296c Mon Sep 17 00:00:00 2001 From: tommady Date: Sat, 28 Jun 2025 03:20:04 +0000 Subject: [PATCH 05/13] Revert "scaffold close-issue-825" This reverts commit dd34293ce8e5a0858f2cdf44e4be5e9ac0b6ffbb. --- src/commands/decompress.rs | 73 ++++++++------------------------------ src/main.rs | 2 +- src/utils/fs.rs | 34 +++++++++++++++++- src/utils/mod.rs | 6 ++-- src/utils/question.rs | 37 +------------------ 5 files changed, 53 insertions(+), 99 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 2dd344e..102d994 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -1,5 +1,4 @@ use std::{ - collections::HashMap, io::{self, BufReader, Read}, ops::ControlFlow, path::{Path, PathBuf}, @@ -23,7 +22,7 @@ use crate::{ logger::{info, info_accessible}, nice_directory_display, user_wants_to_continue, }, - FileConflitOperation, QuestionAction, QuestionPolicy, BUFFER_CAPACITY, + QuestionAction, QuestionPolicy, BUFFER_CAPACITY, }; trait ReadSeek: Read + io::Seek {} @@ -342,24 +341,16 @@ fn unpack( let is_valid_output_dir = !output_dir.exists() || (output_dir.is_dir() && output_dir.read_dir()?.next().is_none()); let output_dir_cleaned = if is_valid_output_dir { - output_dir + output_dir.to_owned() } else { - match utils::check_conflics_and_ask_user(output_dir, question_policy, QuestionAction::Decompression)? { - FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), - FileConflitOperation::Overwrite => { - // TODO: issue 820 could try to enhance here to fix the issue - // https://github.com/ouch-org/ouch/issues/820 - utils::remove_file_or_dir(output_dir)?; - output_dir - } - FileConflitOperation::Rename => &utils::rename_for_available_filename(output_dir), - FileConflitOperation::Merge => output_dir, - FileConflitOperation::GoodToGo => output_dir, + match utils::resolve_path_conflict(output_dir, question_policy, QuestionAction::Decompression)? { + Some(path) => path, + None => return Ok(ControlFlow::Break(())), } }; if !output_dir_cleaned.exists() { - fs::create_dir(output_dir_cleaned)?; + fs::create_dir(&output_dir_cleaned)?; } let files = unpack_fn(output_dir_cleaned)?; @@ -392,7 +383,7 @@ fn smart_unpack( let root_contains_only_one_element = fs::read_dir(temp_dir_path)?.take(2).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(); @@ -406,49 +397,15 @@ fn smart_unpack( (temp_dir_path.to_owned(), output_file_path.to_owned()) }; - match utils::check_conflics_and_ask_user(&new_path, question_policy, QuestionAction::Decompression)? { - FileConflitOperation::Cancel => return Ok(ControlFlow::Break(())), - FileConflitOperation::GoodToGo => { - fs::rename(&previous_path, &new_path)?; - } - FileConflitOperation::Overwrite => { - // TODO: issue 820 could try to enhance here to fix the issue - // https://github.com/ouch-org/ouch/issues/820 - utils::remove_file_or_dir(&new_path)?; - fs::rename(&previous_path, &new_path)?; - } - FileConflitOperation::Rename => { - fs::rename(&previous_path, utils::rename_for_available_filename(&new_path))?; - } - FileConflitOperation::Merge => { - // TODO: just a simple way to verify, will enhance later - let mut seen = HashMap::new(); - for entry in fs::read_dir(&new_path)? { - let entry = entry?; - let path = entry.path(); - let name = path - .file_name() - .expect("The paths were read by the program, so it should be safe."); - - seen.insert(name.to_os_string(), path); - } - - for entry in fs::read_dir(&previous_path)? { - let entry = entry?; - let path = entry.path(); - let name = path - .file_name() - .expect("The paths were read by the program, so it should be safe."); - - if seen.contains_key(&name.to_os_string()) { - continue; - } - - fs::copy(&path, new_path.join(name))?; - } - } - } + // Before moving, need to check if a file with the same name already exists + // If it does, need to ask the user what to do + new_path = match utils::resolve_path_conflict(&new_path, question_policy, QuestionAction::Decompression)? { + 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)?; info_accessible(format!( "Successfully moved \"{}\" to \"{}\"", nice_directory_display(&previous_path), diff --git a/src/main.rs b/src/main.rs index 7c784d5..3f1f7dd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,7 +17,7 @@ use self::{ error::{Error, Result}, utils::{ logger::{shutdown_logger_and_wait, spawn_logger_thread}, - FileConflitOperation, QuestionAction, QuestionPolicy, + QuestionAction, QuestionPolicy, }, }; diff --git a/src/utils/fs.rs b/src/utils/fs.rs index faa50ba..f65f152 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -8,15 +8,47 @@ use std::{ use fs_err as fs; +use super::{question::FileConflitOperation, user_wants_to_overwrite}; use crate::{ extension::Extension, - utils::{logger::info_accessible, EscapedPathDisplay}, + utils::{logger::info_accessible, EscapedPathDisplay, QuestionAction}, + QuestionPolicy, }; pub fn is_path_stdin(path: &Path) -> bool { path.as_os_str() == "-" } +/// 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(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 resolve_path_conflict( + path: &Path, + question_policy: QuestionPolicy, + question_action: QuestionAction, +) -> crate::Result> { + if path.exists() { + match user_wants_to_overwrite(path, question_policy, question_action)? { + 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)) + } + FileConflitOperation::Merge => Ok(Some(path.to_path_buf())), + } + } else { + Ok(Some(path.to_path_buf())) + } +} + pub fn remove_file_or_dir(path: &Path) -> crate::Result<()> { if path.is_dir() { fs::remove_dir_all(path)?; diff --git a/src/utils/mod.rs b/src/utils/mod.rs index d157c9f..444cf1f 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -19,11 +19,11 @@ pub use self::{ }, fs::{ cd_into_same_dir_as, create_dir_if_non_existent, is_path_stdin, remove_file_or_dir, - rename_for_available_filename, try_infer_extension, + rename_for_available_filename, resolve_path_conflict, try_infer_extension, }, question::{ - ask_to_create_file, check_conflics_and_ask_user, user_wants_to_continue, user_wants_to_overwrite, - FileConflitOperation, QuestionAction, QuestionPolicy, + ask_to_create_file, user_wants_to_continue, user_wants_to_overwrite, FileConflitOperation, QuestionAction, + QuestionPolicy, }, utf8::{get_invalid_utf8_paths, is_invalid_utf8}, }; diff --git a/src/utils/question.rs b/src/utils/question.rs index 5389342..4b4c97e 100644 --- a/src/utils/question.rs +++ b/src/utils/question.rs @@ -50,41 +50,6 @@ pub enum FileConflitOperation { Rename, /// Merge conflicting folders Merge, - /// There is no conflict, good to go - GoodToGo, -} - -/// Check if &Path exists, if it does then ask the user what to do. -/// 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 -/// If the user want to merge then the conflics file or directory will be skiped -/// -/// * `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 check_conflics_and_ask_user( - path: &Path, - question_policy: QuestionPolicy, - question_action: QuestionAction, -) -> crate::Result { - if path.exists() { - user_wants_to_overwrite(path, question_policy, question_action) - // match user_wants_to_overwrite(path, question_policy, question_action)? { - // 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)) - // } - // FileConflitOperation::Merge => Ok(Some(path.to_path_buf())), - // } - } else { - Ok(FileConflitOperation::GoodToGo) - // Ok(Some(path.to_path_buf())) - } } /// Check if QuestionPolicy flags were set, otherwise, ask user if they want to overwrite. @@ -147,7 +112,7 @@ pub fn ask_to_create_file( }; match action { - FileConflitOperation::Merge | FileConflitOperation::GoodToGo => Ok(Some(fs::File::create(path)?)), + FileConflitOperation::Merge => Ok(Some(fs::File::create(path)?)), FileConflitOperation::Overwrite => { utils::remove_file_or_dir(path)?; Ok(Some(fs::File::create(path)?)) From 512e4e60f4e19c1776ae224d63cccbdb5c0608eb Mon Sep 17 00:00:00 2001 From: tommady Date: Sat, 28 Jun 2025 03:22:01 +0000 Subject: [PATCH 06/13] fix revert failure Signed-off-by: tommady --- src/commands/decompress.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 102d994..6c254ad 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -353,7 +353,7 @@ fn unpack( fs::create_dir(&output_dir_cleaned)?; } - let files = unpack_fn(output_dir_cleaned)?; + let files = unpack_fn(&output_dir_cleaned)?; Ok(ControlFlow::Continue(files)) } From 5ba90f59814e4709b9b3758699eb67bebed9f13d Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 09:28:36 +0000 Subject: [PATCH 07/13] undo the test unpack_multiple_sources_into_the_same_destination_with_merge and add a new test for issue 825 to reproduce the issue Signed-off-by: tommady --- tests/integration.rs | 127 +++++++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 59 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index b8ec387..4d46c31 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -812,68 +812,42 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( ) { let temp_dir = tempdir()?; let root_path = temp_dir.path(); + let source_path = root_path + .join(format!("example_{}", merge_extensions(ext, &extra_extensions))) + .join("sub_a") + .join("sub_b") + .join("sub_c"); - let source_path = root_path.join(format!("example_{}", merge_extensions(&ext, &extra_extensions))); fs::create_dir_all(&source_path)?; - fs::File::create(source_path.join("file1.txt"))?; - fs::File::create(source_path.join("file2.txt"))?; - fs::File::create(source_path.join("file3.txt"))?; - let sub_folder_path = source_path.join("sub_a"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file1.txt"))?; - fs::File::create(sub_folder_path.join("file2.txt"))?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - let sub_folder_path = sub_folder_path.join("sub_b"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file1.txt"))?; - fs::File::create(sub_folder_path.join("file2.txt"))?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - let sub_folder_path = sub_folder_path.join("sub_c"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file1.txt"))?; - fs::File::create(sub_folder_path.join("file2.txt"))?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - let archive = root_path.join(format!("archive.{}", merge_extensions(&ext, &extra_extensions))); + let archive = root_path.join(format!("archive.{}", merge_extensions(ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") - .arg(&source_path) + .args([ + fs::File::create(source_path.join("file1.txt"))?.path(), + fs::File::create(source_path.join("file2.txt"))?.path(), + fs::File::create(source_path.join("file3.txt"))?.path(), + ]) .arg(&archive) .assert() .success(); fs::remove_dir_all(&source_path)?; - - let source_path = root_path.join(format!("example_{}", merge_extensions(&ext, &extra_extensions))); fs::create_dir_all(&source_path)?; - fs::File::create(source_path.join("file3.txt"))?; - fs::File::create(source_path.join("file4.txt"))?; - fs::File::create(source_path.join("file5.txt"))?; - let sub_folder_path = source_path.join("sub_a"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - fs::File::create(sub_folder_path.join("file4.txt"))?; - fs::File::create(sub_folder_path.join("file5.txt"))?; - let sub_folder_path = sub_folder_path.join("sub_b"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - fs::File::create(sub_folder_path.join("file4.txt"))?; - fs::File::create(sub_folder_path.join("file5.txt"))?; - let sub_folder_path = sub_folder_path.join("sub_c"); - fs::create_dir_all(&sub_folder_path)?; - fs::File::create(sub_folder_path.join("file3.txt"))?; - fs::File::create(sub_folder_path.join("file4.txt"))?; - fs::File::create(sub_folder_path.join("file5.txt"))?; - let archive1 = root_path.join(format!("archive1.{}", merge_extensions(&ext, &extra_extensions))); + let archive1 = root_path.join(format!("archive1.{}", merge_extensions(ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") - .arg(&source_path) + .args([ + fs::File::create(source_path.join("file3.txt"))?.path(), + fs::File::create(source_path.join("file4.txt"))?.path(), + fs::File::create(source_path.join("file5.txt"))?.path(), + ]) .arg(&archive1) .assert() .success(); - let out_path = root_path.join(format!("out_{}", merge_extensions(&ext, &extra_extensions))); + let out_path = root_path.join(format!("out_{}", merge_extensions(ext, &extra_extensions))); fs::create_dir_all(&out_path)?; crate::utils::cargo_bin() @@ -893,22 +867,57 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( .assert() .success(); - fn count_files_recursively(dir: &Path) -> usize { - let mut count = 0; - if let Ok(entries) = fs::read_dir(dir) { - for entry in entries.flatten() { - let path = entry.path(); - if path.is_file() { - count += 1; - } else if path.is_dir() { - count += count_files_recursively(&path); - } - } - } - count - } + assert_eq!(5, out_path.as_path().read_dir()?.count()); +} - assert_eq!(20, count_files_recursively(&out_path)); +#[cfg(feature = "allow_piped_choice")] +#[test] +fn unpack_multiple_sources_into_the_same_destination_with_merge_issue_825() { + let temp_dir = tempdir().unwrap(); + let root_path = temp_dir.path(); + let source_path = root_path.join("parent"); + + fs::create_dir_all(&source_path).unwrap(); + fs::File::create(source_path.join("file1.txt")).unwrap(); + fs::File::create(source_path.join("file2.txt")).unwrap(); + fs::File::create(source_path.join("file3.txt")).unwrap(); + + crate::utils::cargo_bin() + .arg("compress") + .arg(&source_path) + .arg("a.tar") + .assert() + .success(); + + fs::remove_dir_all(&source_path).unwrap(); + fs::create_dir_all(&source_path).unwrap(); + fs::File::create(source_path.join("file3.txt")).unwrap(); + fs::File::create(source_path.join("file4.txt")).unwrap(); + fs::File::create(source_path.join("file5.txt")).unwrap(); + + crate::utils::cargo_bin() + .arg("compress") + .arg(&source_path) + .arg("b.tar") + .assert() + .success(); + + fs::remove_dir_all(&source_path).unwrap(); + + crate::utils::cargo_bin() + .arg("decompress") + .arg("a.tar") + .assert() + .success(); + + crate::utils::cargo_bin() + .arg("decompress") + .arg("b.tar") + .write_stdin("m") + .assert() + .success(); + + assert_eq!(5, source_path.read_dir().unwrap().count()); } #[test] From 776413a9f93242a8eb3085491a6e1c0a0a952bab Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 11:53:40 +0000 Subject: [PATCH 08/13] use copy dir when decompress rename failed Signed-off-by: tommady --- src/commands/decompress.rs | 20 ++++++++++++++- src/utils/fs.rs | 13 ++++++++++ src/utils/mod.rs | 2 +- tests/integration.rs | 50 -------------------------------------- 4 files changed, 33 insertions(+), 52 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index f5dc712..7197e0e 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -413,7 +413,25 @@ fn smart_unpack( }; // Rename the temporary directory to the archive name, which is output_file_path - fs::rename(&previous_path, &new_path)?; + if fs::rename(&previous_path, &new_path).is_err() { + utils::copy_dir(&previous_path, &new_path)?; + // println!("################### {:?}", e); + // if e.kind() == io::ErrorKind::CrossesDevices || e.raw_os_error() == Some(18) { + // println!("⚠️ Rename failed due to a cross-device link. Falling back to copy-and-delete."); + + // // The rename failed, so we fall back to a copy and remove. + // // This is more expensive but works across devices. + // fs::copy(&previous_path, &new_path)?; // Copy the file to the new location. + // fs::symlink_metadata(path) + + // println!("✅ Successfully moved file via copy-and-delete fallback."); + // Ok(()) + // } else { + // // If it's a different error (e.g., permissions), we should fail. + // println!("❌ Rename failed with an unrecoverable error."); + // Err(e) + // } + }; info_accessible(format!( "Successfully moved \"{}\" to \"{}\"", nice_directory_display(&previous_path), diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 0f55252..59a11cb 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -209,3 +209,16 @@ pub fn try_infer_extension(path: &Path) -> Option { None } } + +pub fn copy_dir(src: &Path, dst: &Path) -> crate::Result<()> { + for entry in fs::read_dir(src)? { + let entry = entry?; + let ty = entry.file_type()?; + if ty.is_dir() { + copy_dir(entry.path().as_path(), dst.join(entry.file_name()).as_path())?; + } else { + fs::copy(entry.path(), dst.join(entry.file_name()).as_path())?; + } + } + Ok(()) +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 444cf1f..060a7ed 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -18,7 +18,7 @@ pub use self::{ EscapedPathDisplay, }, fs::{ - cd_into_same_dir_as, create_dir_if_non_existent, is_path_stdin, remove_file_or_dir, + cd_into_same_dir_as, copy_dir, create_dir_if_non_existent, is_path_stdin, remove_file_or_dir, rename_for_available_filename, resolve_path_conflict, try_infer_extension, }, question::{ diff --git a/tests/integration.rs b/tests/integration.rs index 4d46c31..3353020 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -870,56 +870,6 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( assert_eq!(5, out_path.as_path().read_dir()?.count()); } -#[cfg(feature = "allow_piped_choice")] -#[test] -fn unpack_multiple_sources_into_the_same_destination_with_merge_issue_825() { - let temp_dir = tempdir().unwrap(); - let root_path = temp_dir.path(); - let source_path = root_path.join("parent"); - - fs::create_dir_all(&source_path).unwrap(); - fs::File::create(source_path.join("file1.txt")).unwrap(); - fs::File::create(source_path.join("file2.txt")).unwrap(); - fs::File::create(source_path.join("file3.txt")).unwrap(); - - crate::utils::cargo_bin() - .arg("compress") - .arg(&source_path) - .arg("a.tar") - .assert() - .success(); - - fs::remove_dir_all(&source_path).unwrap(); - fs::create_dir_all(&source_path).unwrap(); - fs::File::create(source_path.join("file3.txt")).unwrap(); - fs::File::create(source_path.join("file4.txt")).unwrap(); - fs::File::create(source_path.join("file5.txt")).unwrap(); - - crate::utils::cargo_bin() - .arg("compress") - .arg(&source_path) - .arg("b.tar") - .assert() - .success(); - - fs::remove_dir_all(&source_path).unwrap(); - - crate::utils::cargo_bin() - .arg("decompress") - .arg("a.tar") - .assert() - .success(); - - crate::utils::cargo_bin() - .arg("decompress") - .arg("b.tar") - .write_stdin("m") - .assert() - .success(); - - assert_eq!(5, source_path.read_dir().unwrap().count()); -} - #[test] fn reading_nested_archives_with_two_archive_extensions_adjacent() { let archive_formats = ["tar", "zip", "7z"].into_iter(); From 2fa3fa2310799f270fdf9a09762af1f39bff906d Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 11:54:36 +0000 Subject: [PATCH 09/13] remove comments Signed-off-by: tommady --- src/commands/decompress.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 7197e0e..33a046f 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -415,22 +415,6 @@ fn smart_unpack( // Rename the temporary directory to the archive name, which is output_file_path if fs::rename(&previous_path, &new_path).is_err() { utils::copy_dir(&previous_path, &new_path)?; - // println!("################### {:?}", e); - // if e.kind() == io::ErrorKind::CrossesDevices || e.raw_os_error() == Some(18) { - // println!("⚠️ Rename failed due to a cross-device link. Falling back to copy-and-delete."); - - // // The rename failed, so we fall back to a copy and remove. - // // This is more expensive but works across devices. - // fs::copy(&previous_path, &new_path)?; // Copy the file to the new location. - // fs::symlink_metadata(path) - - // println!("✅ Successfully moved file via copy-and-delete fallback."); - // Ok(()) - // } else { - // // If it's a different error (e.g., permissions), we should fail. - // println!("❌ Rename failed with an unrecoverable error."); - // Err(e) - // } }; info_accessible(format!( "Successfully moved \"{}\" to \"{}\"", From 51f9e732dc22ccc7f8600bf362233c09ea36a86d Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 11:56:35 +0000 Subject: [PATCH 10/13] undo unpack_multiple_sources_into_the_same_destination_with_merge test Signed-off-by: tommady --- tests/integration.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 3353020..7df3995 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -813,14 +813,13 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( let temp_dir = tempdir()?; let root_path = temp_dir.path(); let source_path = root_path - .join(format!("example_{}", merge_extensions(ext, &extra_extensions))) + .join(format!("example_{}", merge_extensions(&ext, &extra_extensions))) .join("sub_a") .join("sub_b") .join("sub_c"); fs::create_dir_all(&source_path)?; - - let archive = root_path.join(format!("archive.{}", merge_extensions(ext, &extra_extensions))); + let archive = root_path.join(format!("archive.{}", merge_extensions(&ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") .args([ @@ -834,8 +833,7 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( fs::remove_dir_all(&source_path)?; fs::create_dir_all(&source_path)?; - - let archive1 = root_path.join(format!("archive1.{}", merge_extensions(ext, &extra_extensions))); + let archive1 = root_path.join(format!("archive1.{}", merge_extensions(&ext, &extra_extensions))); crate::utils::cargo_bin() .arg("compress") .args([ @@ -847,7 +845,7 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( .assert() .success(); - let out_path = root_path.join(format!("out_{}", merge_extensions(ext, &extra_extensions))); + let out_path = root_path.join(format!("out_{}", merge_extensions(&ext, &extra_extensions))); fs::create_dir_all(&out_path)?; crate::utils::cargo_bin() From 1c622ff52a660fa639abc158962ca093b9c04bb1 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 11:59:33 +0000 Subject: [PATCH 11/13] add changelog Signed-off-by: tommady --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4b20ad..0e534e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Categories Used: - Fix tar extraction count when --quiet [\#824](https://github.com/ouch-org/ouch/pull/824) ([marcospb19](https://github.com/marcospb19)) - Fix 7z BadSignature error when compressing and then listing [\#819](https://github.com/ouch-org/ouch/pull/819) ([tommady](https://github.com/tommady)) +- Fix Unpacking with merge flag failed without --dir flag [\#826](https://github.com/ouch-org/ouch/pull/826) ([tommady](https://github.com/tommady)) ### Tweaks From 1f3257da70277380172c52ba17f1ca15a73b065b Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 12:26:02 +0000 Subject: [PATCH 12/13] add doc for utils copy_dir Signed-off-by: tommady --- src/utils/fs.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 59a11cb..4752e63 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -210,6 +210,7 @@ pub fn try_infer_extension(path: &Path) -> Option { } } +/// Copy the src directory into the dst directory recursively pub fn copy_dir(src: &Path, dst: &Path) -> crate::Result<()> { for entry in fs::read_dir(src)? { let entry = entry?; From a85c12a12fbc7a44252a78f76d02e75216c257c9 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 16 Jul 2025 12:32:00 +0000 Subject: [PATCH 13/13] add src and dst detection for utils copy_dir Signed-off-by: tommady --- src/utils/fs.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 4752e63..f5a23fc 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -212,6 +212,12 @@ pub fn try_infer_extension(path: &Path) -> Option { /// Copy the src directory into the dst directory recursively pub fn copy_dir(src: &Path, dst: &Path) -> crate::Result<()> { + if !src.exists() || !dst.exists() { + return Err(crate::Error::NotFound { + error_title: "source or destination directory does not exist".to_string(), + }); + } + for entry in fs::read_dir(src)? { let entry = entry?; let ty = entry.file_type()?;