Skip to content

Commit 0a62d22

Browse files
more review comments
1 parent c67caee commit 0a62d22

File tree

2 files changed

+11
-9
lines changed

2 files changed

+11
-9
lines changed

src/read.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1018,9 +1018,6 @@ impl<R: Read + Seek> ZipArchive<R> {
10181018

10191019
fn make_writable_dir_all<T: AsRef<Path>>(outpath: T) -> Result<(), ZipError> {
10201020
create_dir_all(outpath.as_ref())?;
1021-
/* TODO: do we want to automatically make the directory writable? Wouldn't we prefer to
1022-
* respect the write permissions of the extraction dir? Pipelined extraction does not
1023-
* mutate permissions like this. */
10241021
#[cfg(unix)]
10251022
{
10261023
// Dirs must be writable until all normal files are extracted

src/read/pipelining.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,14 @@ pub mod path_splitting {
1313
pub enum PathSplitError {
1414
/// entry path format error: {0:?}
1515
PathFormat(String),
16-
/// file and directory paths overlapped: {0:?}
17-
FileDirOverlap(String),
16+
/// duplicate path used for file and directory: {0:?}
17+
DuplicateFileDirPath(String),
1818
}
1919

2020
fn split_by_separator(entry_path: &str) -> Result<impl Iterator<Item = &str>, PathSplitError> {
21+
/* FIXME: The ZIP spec states (APPNOTE 4.4.17) that file paths are in Unix format, and Unix
22+
* filesystems treat a backslash as a normal character. Thus they should be allowed on Unix
23+
* and replaced with \u{fffd} on Windows. */
2124
if entry_path.contains('\\') {
2225
if entry_path.contains('/') {
2326
return Err(PathSplitError::PathFormat(format!(
@@ -151,7 +154,7 @@ pub mod path_splitting {
151154
.or_insert_with(|| Box::new(FSEntry::Dir(DirEntry::default())));
152155
cur_dir = match next_subdir.as_mut() {
153156
FSEntry::File(_) => {
154-
return Err(PathSplitError::FileDirOverlap(format!(
157+
return Err(PathSplitError::DuplicateFileDirPath(format!(
155158
"a file was already registered at the same path as the dir entry {:?}",
156159
entry_path
157160
)));
@@ -164,7 +167,7 @@ pub mod path_splitting {
164167
/* We can't handle duplicate file paths, as that might mess up our
165168
* parallelization strategy. */
166169
if cur_dir.children.contains_key(filename) {
167-
return Err(PathSplitError::FileDirOverlap(format!(
170+
return Err(PathSplitError::DuplicateFileDirPath(format!(
168171
"another file or directory was already registered at the same path as the file entry {:?}",
169172
entry_path
170173
)));
@@ -178,7 +181,7 @@ pub mod path_splitting {
178181
* path, as it's not clear how to merge the possibility of two separate file
179182
* permissions. */
180183
if cur_dir.properties.replace(data).is_some() {
181-
return Err(PathSplitError::FileDirOverlap(format!(
184+
return Err(PathSplitError::DuplicateFileDirPath(format!(
182185
"another directory was already registered at the path {:?}",
183186
entry_path
184187
)));
@@ -191,7 +194,7 @@ pub mod path_splitting {
191194
properties,
192195
children,
193196
} = base_dir;
194-
assert!(properties.is_none(), "setting metadata on the top-level extraction dir is not allowed and should have been filtered out");
197+
debug_assert!(properties.is_none(), "setting metadata on the top-level extraction dir is not allowed and should have been filtered out");
195198
Ok(children)
196199
}
197200

@@ -491,6 +494,8 @@ pub mod handle_creation {
491494
properties,
492495
children,
493496
}) => {
497+
/* FIXME: use something like make_writable_dir_all() and then add to
498+
* perms_todo! */
494499
match fs::create_dir(&path) {
495500
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => (),
496501
Err(e) => return Err(e.into()),

0 commit comments

Comments
 (0)