From be4559b969ff9afc655e3870b5af3e56f670fda9 Mon Sep 17 00:00:00 2001 From: valoq Date: Wed, 20 May 2026 14:06:46 +0200 Subject: [PATCH 1/3] fix new extract logic --- src/commands/decompress.rs | 87 ++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 32 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index a2ba2eba2..02bd56440 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -278,8 +278,6 @@ fn unpack_archive( /// Expects `wrapper` to be a just-decompressed archive output directory, if /// `wrapper` contains exactly one entry with the same name (e.g. extracting /// `archive.zip` produced `archive/archive/...`), then flatten it to `archive/...`. -/// -/// Returns the resulting path the user should see. If reads fail, fn deduplicate_basename_wrapper(wrapper: &Path) -> Result<()> { let Some(wrapper_name) = wrapper.file_name() else { return Ok(()); @@ -303,33 +301,34 @@ fn deduplicate_basename_wrapper(wrapper: &Path) -> Result<()> { return Ok(()); } - // The wrapper duplicates the inner entry's name. Promote the inner entry by: - // 1. moving it into a sibling temporary directory - // 2. removing the now-empty wrapper - // 3. moving it back to the wrapper's path - // Each step leaves the filesystem in a consistent state, and a failure midway - // leaves the user with valid extracted content (just nested one level). - let inner_path = only_file_in_dir.path(); - let Some(parent) = wrapper.parent() else { + // Only collapse when the inner duplicate is itself a directory. + if !only_file_in_dir.file_type()?.is_dir() { return Ok(()); - }; - - // Create the sibling - let sibling_path = parent.join("ouch-temporary"); - fs::create_dir(&sibling_path)?; - - // Move child inside the sibling - let path_inside_sibling = sibling_path.join(wrapper_name); - fs::rename(&inner_path, &path_inside_sibling)?; - - // Delete old parent - fs::remove_dir(wrapper)?; + } - // Move child to its dead parent place - fs::rename(&path_inside_sibling, wrapper)?; + // Promote inner entries one level up, all writes stay inside `wrapper`. + let inner_dir = only_file_in_dir.path(); + + let mut staged: Option = None; + for entry in fs::read_dir(&inner_dir)? { + let entry = entry?; + let name = entry.file_name(); + if name == wrapper_name { + // Stage under a random name to avoid colliding with the still-existing inner dir. + let tmp = tempfile::Builder::new().prefix(".ouch-").tempfile_in(wrapper)?.into_temp_path(); + fs::remove_file(&tmp)?; + fs::rename(entry.path(), &tmp)?; + staged = Some(tmp); + } else { + fs::rename(entry.path(), wrapper.join(name))?; + } + } - // Delete the temporary sibling - fs::remove_dir(sibling_path)?; + fs::remove_dir(&inner_dir)?; + if let Some(tmp) = staged { + fs::rename(&tmp, wrapper.join(wrapper_name))?; + let _ = tmp.keep(); + } Ok(()) } @@ -365,7 +364,8 @@ mod tests { } /// The main case: wrapper contains exactly one entry whose name equals the wrapper's - /// name. The inner entry should be promoted up one level. + /// name. The inner entry should be promoted up one level. The flatten must not + /// create or touch anything in the wrapper's parent directory. #[test] fn deduplicate_flattens_when_inner_dir_matches_wrapper_name() { let dir = tempdir().unwrap(); @@ -377,6 +377,9 @@ mod tests { deduplicate_basename_wrapper(&wrapper).unwrap(); assert_eq!(list_tree(&wrapper), vec!["a.txt", "b.txt"]); + // Parent must be untouched: only `wrapper` itself should be visible there. + let parent_entries: Vec<_> = std_fs::read_dir(dir.path()).unwrap().map(|e| e.unwrap().file_name()).collect(); + assert_eq!(parent_entries, vec![std::ffi::OsString::from("archive")]); } /// Wrapper contains a single entry, but its name differs from the wrapper's name. @@ -418,18 +421,38 @@ mod tests { assert_eq!(list_tree(&wrapper), Vec::::new()); } - /// Edge case: the single inner entry is a *file* (not a directory) whose name - /// equals the wrapper's name. The wrapper directory should be replaced with that file. + /// Wrapper contains a single entry, but it's a file (not a directory). + /// No flatten should happen — the wrapper survives and the file stays inside. #[test] - fn deduplicate_promotes_single_inner_file_with_matching_name() { + fn deduplicate_keeps_wrapper_when_inner_is_file() { let dir = tempdir().unwrap(); let wrapper = dir.path().join("archive"); std_fs::create_dir(&wrapper).unwrap(); std_fs::write(wrapper.join("archive"), "data").unwrap(); deduplicate_basename_wrapper(&wrapper).unwrap(); - assert!(wrapper.is_file(), "wrapper should now be a file, not a directory"); - assert_eq!(std_fs::read(&wrapper).unwrap(), b"data"); + assert!(wrapper.is_dir(), "wrapper must remain a directory"); + assert_eq!(list_tree(&wrapper), vec!["archive".to_string()]); + assert_eq!(std_fs::read(wrapper.join("archive")).unwrap(), b"data"); + } + + /// Inner dir's children include one that shares the wrapper's name. The + /// in-place flatten must stage it under a random name to avoid the collision + /// and end with `wrapper/archive` containing the collision's contents. + #[test] + fn deduplicate_handles_inner_entry_named_like_wrapper() { + let dir = tempdir().unwrap(); + let wrapper = dir.path().join("archive"); + let inner = wrapper.join("archive"); + let collision = inner.join("archive"); + std_fs::create_dir_all(&collision).unwrap(); + std_fs::write(collision.join("inside"), "x").unwrap(); + std_fs::write(inner.join("other.txt"), "o").unwrap(); + + deduplicate_basename_wrapper(&wrapper).unwrap(); + assert_eq!(list_tree(&wrapper), vec!["archive/", "archive/inside", "other.txt"]); + assert_eq!(std_fs::read(wrapper.join("archive").join("inside")).unwrap(), b"x"); + assert_eq!(std_fs::read(wrapper.join("other.txt")).unwrap(), b"o"); } /// The flatten only collapses *one* level: nested same-name directories produced From cad0129400816a10a8cf2917c87f4d933cd9c33d Mon Sep 17 00:00:00 2001 From: valoq Date: Wed, 20 May 2026 14:11:20 +0200 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17d48c872..4e341452b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ Categories Used: ### New Features -- Unpack into new folder by default (https://github.com/ouch-org/ouch/pull/962). +- Unpack into new folder by default (https://github.com/ouch-org/ouch/pull/962) and (https://github.com/ouch-org/ouch/pull/996). - Add `--here` flag to unpack into current directory (https://github.com/ouch-org/ouch/pull/962). - List: show symlink targets (for tar and zip) (https://github.com/ouch-org/ouch/pull/934) - Add aliases for ebooks (`.epub`) (https://github.com/ouch-org/ouch/pull/981) From 854249f0e1bf5b5881dcad97f104c80dd658d2cc Mon Sep 17 00:00:00 2001 From: valoq Date: Wed, 20 May 2026 15:11:44 +0200 Subject: [PATCH 3/3] fix patch --- src/commands/decompress.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 02bd56440..274ec01a7 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -315,7 +315,10 @@ fn deduplicate_basename_wrapper(wrapper: &Path) -> Result<()> { let name = entry.file_name(); if name == wrapper_name { // Stage under a random name to avoid colliding with the still-existing inner dir. - let tmp = tempfile::Builder::new().prefix(".ouch-").tempfile_in(wrapper)?.into_temp_path(); + let tmp = tempfile::Builder::new() + .prefix(".ouch-") + .tempfile_in(wrapper)? + .into_temp_path(); fs::remove_file(&tmp)?; fs::rename(entry.path(), &tmp)?; staged = Some(tmp); @@ -378,7 +381,10 @@ mod tests { deduplicate_basename_wrapper(&wrapper).unwrap(); assert_eq!(list_tree(&wrapper), vec!["a.txt", "b.txt"]); // Parent must be untouched: only `wrapper` itself should be visible there. - let parent_entries: Vec<_> = std_fs::read_dir(dir.path()).unwrap().map(|e| e.unwrap().file_name()).collect(); + let parent_entries: Vec<_> = std_fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); assert_eq!(parent_entries, vec![std::ffi::OsString::from("archive")]); }