refactor files_by_unix_mode

Author: cosmicexplorer

src/read.rs

@@ -20,6 +20,7 @@ use crate::zipcrypto::{ZipCryptoReader, ZipCryptoReaderValid, ZipCryptoValidator
 use crate::ZIP64_BYTES_THR;
 use indexmap::IndexMap;
 use std::borrow::Cow;
+use std::collections::BTreeMap;
 use std::ffi::OsStr;
 use std::fs::create_dir_all;
 use std::io::{self, copy, prelude::*, sink, SeekFrom};
@@ -536,6 +537,33 @@ impl<'a> TryFrom<&'a CentralDirectoryEndInfo> for CentralDirectoryInfo {
     }
 }
 
+/// Store all entries which specify a numeric "mode" which is familiar to POSIX operating systems.
+#[cfg(unix)]
+#[derive(Default, Debug)]
+struct FilesByUnixMode {
+    map: BTreeMap<PathBuf, u32>,
+}
+
+#[cfg(unix)]
+impl FilesByUnixMode {
+    pub fn add_mode(&mut self, path: PathBuf, mode: u32) {
+        // We don't print a warning or consider it remotely out of the ordinary to receive two
+        // separate modes for the same path: just take the later one.
+        let _ = self.map.insert(path, mode);
+    }
+
+    // Child nodes will be sorted later lexicographically, so reversing the order puts them first.
+    pub fn all_perms_with_children_first(
+        self,
+    ) -> impl IntoIterator<Item = (PathBuf, std::fs::Permissions)> {
+        use std::os::unix::fs::PermissionsExt;
+        self.map
+            .into_iter()
+            .rev()
+            .map(|(p, m)| (p, std::fs::Permissions::from_mode(m)))
+    }
+}
+
 impl<R> ZipArchive<R> {
     pub(crate) fn from_finalized_writer(
         files: IndexMap<Box<str>, ZipFileData>,
@@ -885,33 +913,30 @@ impl<R: Read + Seek> ZipArchive<R> {
             .transpose()?;
 
         #[cfg(unix)]
-        let mut files_by_unix_mode = Vec::new();
+        let mut files_by_unix_mode = FilesByUnixMode::default();
 
         for i in 0..self.len() {
             let mut file = self.by_index(i)?;
 
             let mut outpath = directory.clone();
+            /* TODO: the control flow of this method call and subsequent expectations about the
+             *       values in this loop is extremely difficult to follow. It also appears to
+             *       perform a nested loop upon extracting every single file entry? Why does it
+             *       accept two arguments that point to the same directory path, one mutable? */
             file.safe_prepare_path(directory.as_ref(), &mut outpath, root_dir.as_ref())?;
 
             let mut symlink_target: Option<Vec<u8>> = None;
-            cfg_if! {
-                if #[cfg(any(unix, windows))] {
-                    if file.is_symlink() {
-                        let mut target = Vec::with_capacity(file.size() as usize);
-                        file.read_to_end(&mut target)?;
-                        symlink_target = Some(target);
-                    }
-                } else {
-                    if file.is_symlink() {
-                        todo!("no support for symlinks outside of unix or windows platforms");
-                    }
-                }
+            #[cfg(any(unix, windows))]
+            if file.is_symlink() {
+                let mut target = Vec::with_capacity(file.size() as usize);
+                file.read_to_end(&mut target)?;
+                symlink_target = Some(target);
             }
+
             if symlink_target.is_none() && file.is_dir() {
                 crate::read::make_writable_dir_all(&outpath)?;
                 continue;
             }
-
             if let Some(target) = symlink_target {
                 drop(file);
                 make_symlink(&outpath, &target, &self.shared.files)?;
@@ -920,36 +945,28 @@ impl<R: Read + Seek> ZipArchive<R> {
             let mut outfile = fs::File::create(&outpath)?;
 
             io::copy(&mut file, &mut outfile)?;
+
+            // Check for real permissions, which we'll set in a second pass.
             #[cfg(unix)]
-            {
-                // Check for real permissions, which we'll set in a second pass
-                if let Some(mode) = file.unix_mode() {
-                    files_by_unix_mode.push((outpath.clone(), mode));
-                }
+            if let Some(mode) = file.unix_mode() {
+                files_by_unix_mode.add_mode(outpath, mode);
             }
+
+            // Set original timestamp.
             #[cfg(feature = "chrono")]
-            {
-                // Set original timestamp.
-                if let Some(last_modified) = file.last_modified() {
-                    if let Some(t) = datetime_to_systemtime(&last_modified) {
-                        outfile.set_modified(t)?;
-                    }
+            if let Some(last_modified) = file.last_modified() {
+                if let Some(t) = datetime_to_systemtime(&last_modified) {
+                    outfile.set_modified(t)?;
                 }
             }
         }
-        #[cfg(unix)]
-        {
-            use std::cmp::Reverse;
-            use std::os::unix::fs::PermissionsExt;
 
-            if files_by_unix_mode.len() > 1 {
-                // Ensure we update children's permissions before making a parent unwritable
-                files_by_unix_mode.sort_by_key(|(path, _)| Reverse(path.clone()));
-            }
-            for (path, mode) in files_by_unix_mode.into_iter() {
-                fs::set_permissions(&path, fs::Permissions::from_mode(mode))?;
-            }
+        // Ensure we update children's permissions before making a parent unwritable.
+        #[cfg(unix)]
+        for (path, perms) in files_by_unix_mode.all_perms_with_children_first() {
+            std::fs::set_permissions(path, perms)?;
         }
+
         Ok(())
     }