Skip to content

Commit c458437

Browse files
committed
fix: disambiguate archive entries when multiple inputs share the same basename
When compressing multiple directories that share the same basename (e.g., /a/parent1/Scripts and /a/parent2/Scripts), ouch previously stripped paths to just the basename using file_name(). This caused: - Zip: 'Invalid zip archive - Duplicate filename: Scripts/' - Tar: both directories merge into one, data loss on decompress Fix: detect when input paths have duplicate basenames and, in that case, compute the common ancestor directory of all inputs. Archive entries are then stored relative to this common ancestor, including enough parent path components to disambiguate (e.g., parent1/Scripts vs parent2/Scripts). When basenames don't collide, behavior is unchanged (backward compatible). Applied consistently to all three archive builders: tar, zip, and 7z. Fixes #978
1 parent da7b247 commit c458437

5 files changed

Lines changed: 211 additions & 13 deletions

File tree

src/archive/sevenz.rs

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ use crate::{
1818
info,
1919
list::{FileInArchive, ListFileType},
2020
utils::{
21-
BytesFmt, FileVisibilityPolicy, PathFmt, cd_into_same_dir_as, ensure_parent_dir_exists, is_same_file_as_output,
21+
BytesFmt, FileVisibilityPolicy, PathFmt, cd_into_same_dir_as, common_ancestor, ensure_parent_dir_exists,
22+
has_duplicate_basenames, is_same_file_as_output,
2223
},
2324
warning,
2425
};
@@ -134,14 +135,35 @@ where
134135
let mut writer = sevenz_rust2::ArchiveWriter::new(writer)?;
135136
let output_handle = Handle::from_path(output_path);
136137

138+
// When multiple inputs share the same basename, archive entry names would collide.
139+
// In that case, use the common ancestor of all paths and store entries relative to it,
140+
// including enough parent path components to disambiguate.
141+
let use_common_ancestor = files.len() > 1 && has_duplicate_basenames(files);
142+
let common_ancestor = if use_common_ancestor {
143+
Some(common_ancestor(files))
144+
} else {
145+
None
146+
};
147+
137148
for filename in files {
138-
let previous_location = cd_into_same_dir_as(filename)?;
149+
let previous_location = if let Some(ref ancestor) = common_ancestor {
150+
let previous = env::current_dir()?;
151+
env::set_current_dir(ancestor)?;
152+
previous
153+
} else {
154+
cd_into_same_dir_as(filename)?
155+
};
139156

140157
// Unwrap safety:
141158
// paths should be canonicalized by now, and the root directory rejected.
142-
let filename = filename.file_name().unwrap();
159+
// When using common ancestor, include enough parent path to avoid collisions.
160+
let walker_root = if let Some(ref ancestor) = common_ancestor {
161+
filename.strip_prefix(ancestor).unwrap().to_path_buf()
162+
} else {
163+
PathBuf::from(filename.file_name().unwrap())
164+
};
143165

144-
for entry in file_visibility_policy.build_walker(filename) {
166+
for entry in file_visibility_policy.build_walker(&walker_root) {
145167
let entry = entry?;
146168
let path = entry.path();
147169

src/archive/tar.rs

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,14 +144,35 @@ where
144144
let output_handle = Handle::from_path(output_path);
145145
let mut seen_inode: HashMap<(u64, u64), PathBuf> = HashMap::new();
146146

147+
// When multiple inputs share the same basename, archive entry names would collide.
148+
// In that case, use the common ancestor of all paths and store entries relative to it,
149+
// including enough parent path components to disambiguate.
150+
let use_common_ancestor = explicit_paths.len() > 1 && utils::has_duplicate_basenames(explicit_paths);
151+
let common_ancestor = if use_common_ancestor {
152+
Some(utils::common_ancestor(explicit_paths))
153+
} else {
154+
None
155+
};
156+
147157
for explicit_path in explicit_paths {
148-
let previous_location = utils::cd_into_same_dir_as(explicit_path)?;
158+
let previous_location = if let Some(ref ancestor) = common_ancestor {
159+
let previous = env::current_dir()?;
160+
env::set_current_dir(ancestor)?;
161+
previous
162+
} else {
163+
utils::cd_into_same_dir_as(explicit_path)?
164+
};
149165

150166
// Unwrap expectation:
151167
// paths should be canonicalized by now, and the root directory rejected.
152-
let filename = explicit_path.file_name().unwrap();
168+
// When using common ancestor, include enough parent path to avoid collisions.
169+
let walker_root = if let Some(ref ancestor) = common_ancestor {
170+
explicit_path.strip_prefix(ancestor).unwrap().as_os_str()
171+
} else {
172+
explicit_path.file_name().unwrap()
173+
};
153174

154-
let iter = file_visibility_policy.workaround_build_walker_or_broken_link_path(explicit_path, filename);
175+
let iter = file_visibility_policy.workaround_build_walker_or_broken_link_path(explicit_path, walker_root);
155176

156177
for entry in iter {
157178
let path = entry?;

src/archive/zip.rs

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ use crate::{
2121
info, info_accessible,
2222
list::{FileInArchive, ListFileType},
2323
utils::{
24-
BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, cd_into_same_dir_as, create_symlink,
25-
ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, pretty_format_list_of_paths,
26-
read_file_type, strip_cur_dir,
24+
BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, cd_into_same_dir_as, common_ancestor,
25+
create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, has_duplicate_basenames,
26+
is_same_file_as_output, pretty_format_list_of_paths, read_file_type, strip_cur_dir,
2727
},
2828
warning,
2929
};
@@ -180,14 +180,35 @@ where
180180
return Err(error.into());
181181
}
182182

183+
// When multiple inputs share the same basename, archive entry names would collide.
184+
// In that case, use the common ancestor of all paths and store entries relative to it,
185+
// including enough parent path components to disambiguate.
186+
let use_common_ancestor = input_filenames.len() > 1 && has_duplicate_basenames(input_filenames);
187+
let common_ancestor = if use_common_ancestor {
188+
Some(common_ancestor(input_filenames))
189+
} else {
190+
None
191+
};
192+
183193
for explicit_path in input_filenames {
184-
let previous_location = cd_into_same_dir_as(explicit_path)?;
194+
let previous_location = if let Some(ref ancestor) = common_ancestor {
195+
let previous = env::current_dir()?;
196+
env::set_current_dir(ancestor)?;
197+
previous
198+
} else {
199+
cd_into_same_dir_as(explicit_path)?
200+
};
185201

186202
// Unwrap safety:
187203
// paths should be canonicalized by now, and the root directory rejected.
188-
let filename = explicit_path.file_name().unwrap();
204+
// When using common ancestor, include enough parent path to avoid collisions.
205+
let walker_root = if let Some(ref ancestor) = common_ancestor {
206+
explicit_path.strip_prefix(ancestor).unwrap().as_os_str()
207+
} else {
208+
explicit_path.file_name().unwrap()
209+
};
189210

190-
let iter = file_visibility_policy.workaround_build_walker_or_broken_link_path(explicit_path, filename);
211+
let iter = file_visibility_policy.workaround_build_walker_or_broken_link_path(explicit_path, walker_root);
191212

192213
for entry in iter {
193214
let path = entry?;

src/utils/fs.rs

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,56 @@ pub fn cd_into_same_dir_as(filename: &Path) -> Result<PathBuf> {
138138
Ok(previous_location)
139139
}
140140

141+
/// Check if any of the given paths share the same basename (file_name).
142+
///
143+
/// Returns `true` if there are duplicate basenames among the paths.
144+
/// Used to detect when archive entry names would collide.
145+
pub fn has_duplicate_basenames(paths: &[PathBuf]) -> bool {
146+
use std::{collections::HashSet, ffi::OsStr};
147+
148+
let basenames: Vec<&OsStr> = paths.iter().filter_map(|p| p.file_name()).collect();
149+
150+
let mut seen = HashSet::new();
151+
for name in &basenames {
152+
if !seen.insert(name) {
153+
return true;
154+
}
155+
}
156+
false
157+
}
158+
159+
/// Compute the longest common ancestor directory shared by all given paths.
160+
///
161+
/// All paths should be absolute (canonicalized). Returns the longest path
162+
/// prefix that is a common ancestor of all input paths.
163+
///
164+
/// For example:
165+
/// - `/a/b/Scripts` and `/c/d/Scripts` → `/`
166+
/// - `/home/user/a/Scripts` and `/home/user/b/Scripts` → `/home/user`
167+
pub fn common_ancestor(paths: &[PathBuf]) -> PathBuf {
168+
use std::path::Component;
169+
170+
if paths.is_empty() {
171+
return PathBuf::new();
172+
}
173+
174+
let components: Vec<Vec<Component>> = paths.iter().map(|p| p.components().collect()).collect();
175+
176+
let min_len = components.iter().map(|c| c.len()).min().unwrap_or(0);
177+
178+
let mut common = PathBuf::new();
179+
for i in 0..min_len {
180+
let component = components[0][i];
181+
if components.iter().all(|c| c[i] == component) {
182+
common.push(component.as_os_str());
183+
} else {
184+
break;
185+
}
186+
}
187+
188+
common
189+
}
190+
141191
/// Check if a path refers to the same file as the output handle.
142192
pub fn is_same_file_as_output(path: &Path, output_handle: &Handle) -> bool {
143193
if matches!(Handle::from_path(path), Ok(x) if &x == output_handle) {

tests/integration.rs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1569,3 +1569,87 @@ fn decompress_concatenated_lz4_frames() {
15691569
encoder.finish().unwrap()
15701570
});
15711571
}
1572+
1573+
/// Test that compressing multiple directories with the same basename works correctly.
1574+
/// This is the fix for issue #978: when two input directories share the same name
1575+
/// (e.g., `/a/parent1/Scripts` and `/a/parent2/Scripts`), ouch should include enough
1576+
/// parent path to disambiguate them in the archive, rather than collapsing both into
1577+
/// a single "Scripts" entry.
1578+
#[test]
1579+
fn compress_dirs_with_same_basename_tar() {
1580+
let (_tempdir, dir) = testdir().unwrap();
1581+
1582+
// Create two directories with the same basename but different parents
1583+
let dir_a = dir.join("parent1").join("Scripts");
1584+
let dir_b = dir.join("parent2").join("Scripts");
1585+
fs::create_dir_all(&dir_a).unwrap();
1586+
fs::create_dir_all(&dir_b).unwrap();
1587+
1588+
// Create distinct files in each
1589+
fs::write(dir_a.join("a.txt"), "content from A").unwrap();
1590+
fs::write(dir_b.join("b.txt"), "content from B").unwrap();
1591+
1592+
let archive = dir.join("archive.tar");
1593+
ouch!("-A", "c", &dir_a, &dir_b, &archive);
1594+
1595+
let after = dir.join("after");
1596+
ouch!("-A", "d", &archive, "-d", &after);
1597+
1598+
// Both directories should be present with their parent disambiguators
1599+
assert!(
1600+
after.join("parent1").join("Scripts").join("a.txt").exists(),
1601+
"parent1/Scripts/a.txt missing from archive"
1602+
);
1603+
assert!(
1604+
after.join("parent2").join("Scripts").join("b.txt").exists(),
1605+
"parent2/Scripts/b.txt missing from archive"
1606+
);
1607+
1608+
// Verify file contents
1609+
assert_eq!(
1610+
"content from A",
1611+
fs::read_to_string(after.join("parent1").join("Scripts").join("a.txt")).unwrap()
1612+
);
1613+
assert_eq!(
1614+
"content from B",
1615+
fs::read_to_string(after.join("parent2").join("Scripts").join("b.txt")).unwrap()
1616+
);
1617+
}
1618+
1619+
/// Same as above but for zip format
1620+
#[test]
1621+
fn compress_dirs_with_same_basename_zip() {
1622+
let (_tempdir, dir) = testdir().unwrap();
1623+
1624+
let dir_a = dir.join("parent1").join("Scripts");
1625+
let dir_b = dir.join("parent2").join("Scripts");
1626+
fs::create_dir_all(&dir_a).unwrap();
1627+
fs::create_dir_all(&dir_b).unwrap();
1628+
1629+
fs::write(dir_a.join("a.txt"), "content from A").unwrap();
1630+
fs::write(dir_b.join("b.txt"), "content from B").unwrap();
1631+
1632+
let archive = dir.join("archive.zip");
1633+
ouch!("-A", "c", &dir_a, &dir_b, &archive);
1634+
1635+
let after = dir.join("after");
1636+
ouch!("-A", "d", &archive, "-d", &after);
1637+
1638+
assert!(
1639+
after.join("parent1").join("Scripts").join("a.txt").exists(),
1640+
"parent1/Scripts/a.txt missing from zip archive"
1641+
);
1642+
assert!(
1643+
after.join("parent2").join("Scripts").join("b.txt").exists(),
1644+
"parent2/Scripts/b.txt missing from zip archive"
1645+
);
1646+
1647+
assert_eq!(
1648+
"content from A",
1649+
fs::read_to_string(after.join("parent1").join("Scripts").join("a.txt")).unwrap()
1650+
);
1651+
assert_eq!(
1652+
"content from B",
1653+
fs::read_to_string(after.join("parent2").join("Scripts").join("b.txt")).unwrap()
1654+
);
1655+
}

0 commit comments

Comments
 (0)