Merge pull request #278 from ouch-org/refactor-last-modified-times-when-unpacking-archives

Check for errors when setting the last modified time

CHANGELOG.md

@@ -71,6 +71,7 @@ Categories Used:
 - Respect file permissions when compressing zip files [\#271](https://github.com/ouch-org/ouch/pull/271) ([figsoda](https://github.com/figsoda))
 - Apply clippy lints [\#273](https://github.com/ouch-org/ouch/pull/273) ([figsoda](https://github.com/figsoda))
 - Warn user if file extension is passed as file name [\#277](https://github.com/ouch-org/ouch/pull/277) ([marcospb19](https://github.com/marcospb19))
+- Check for errors when setting the last modified time [\#278](https://github.com/ouch-org/ouch/pull/278) ([marcospb19](https://github.com/marcospb19))
 
 ### Tweaks
 
@@ -93,6 +94,7 @@ Categories Used:
 - Update dependencies [\#257](https://github.com/ouch-org/ouch/pull/257) ([Artturin](https://github.com/Artturin))
 - Add pull request template [\#263](https://github.com/ouch-org/ouch/pull/263) ([figsoda](https://github.com/figsoda))
 - Clean up the description for the `-d/--dir` argument to `decompress` [\#264](https://github.com/ouch-org/ouch/pull/264) ([hivehand](https://github.com/hivehand))
+- Show subcommand aliases on --help [\#275](https://github.com/ouch-org/ouch/pull/275) ([marcospb19](https://github.com/marcospb19))
 - Update dependencies [\#276](https://github.com/ouch-org/ouch/pull/276) ([figsoda](https://github.com/figsoda))
 
 ### New Contributors

Cargo.lock

@@ -393,6 +393,12 @@ dependencies = [
  "either",
 ]
 
+[[package]]
+name = "itoa"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4217ad341ebadf8d8e724e264f13e593e0648f5b3e94b3896a5df283be015ecc"
+
 [[package]]
 name = "jobserver"
 version = "0.1.25"
@@ -521,6 +527,7 @@ dependencies = [
  "clap",
  "clap_complete",
  "clap_mangen",
+ "filetime",
  "flate2",
  "fs-err",
  "ignore",
@@ -913,10 +920,18 @@ version = "0.3.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d634a985c4d4238ec39cacaed2e7ae552fbd3c476b552c1deac3021b7d7eaf0c"
 dependencies = [
+ "itoa",
  "libc",
  "num_threads",
+ "time-macros",
 ]
 
+[[package]]
+name = "time-macros"
+version = "0.2.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "42657b1a6f4d817cda8e7a0ace261fe0cc946cf3a80314390b22cc61ae080792"
+
 [[package]]
 name = "unicode-ident"
 version = "1.0.5"
@@ -1075,6 +1090,7 @@ dependencies = [
  "crc32fast",
  "crossbeam-utils",
  "flate2",
+ "time",
 ]
 
 [[package]]

Cargo.toml

@@ -25,11 +25,12 @@ once_cell = "1.15.0"
 snap = "1.0.5"
 tar = "0.4.38"
 xz2 = "0.1.7"
-zip = { version = "0.6.2", default-features = false }
+zip = { version = "0.6.2", default-features = false, features = ["time"] }
 zstd = { version = "0.11.2", default-features = false }
 tempfile = "3.3.0"
 ignore = "0.4.18"
 indicatif = "0.17.1"
+filetime = "0.2.17"
 
 [target.'cfg(unix)'.dependencies]
 time = { version = "0.3.15", default-features = false }

src/archive/tar.rs

@@ -9,7 +9,6 @@ use std::{
 };
 
 use fs_err as fs;
-use tar;
 
 use crate::{
     error::FinalError,
@@ -40,7 +39,12 @@ pub fn unpack_archive(
         // spoken text for users using screen readers, braille displays
         // and so on
 
-        info!(@display_handle, inaccessible, "{:?} extracted. ({})", utils::strip_cur_dir(&output_folder.join(file.path()?)), Bytes::new(file.size()));
+        info!(
+            @display_handle,
+            inaccessible,
+            "{:?} extracted. ({})",
+            utils::strip_cur_dir(&output_folder.join(file.path()?)), Bytes::new(file.size())
+        );
 
         files_unpacked.push(file_path);
     }

src/archive/zip.rs

@@ -10,6 +10,7 @@ use std::{
     thread,
 };
 
+use filetime::{set_file_mtime, FileTime};
 use fs_err as fs;
 use zip::{self, read::ZipFile, ZipArchive};
 
@@ -67,18 +68,22 @@ where
                 let file_path = strip_cur_dir(file_path.as_path());
 
                 // same reason is in _is_dir: long, often not needed text
-                info!(@display_handle, inaccessible, "{:?} extracted. ({})", file_path.display(), Bytes::new(file.size()));
+                info!(
+                    @display_handle,
+                    inaccessible,
+                    "{:?} extracted. ({})",
+                    file_path.display(), Bytes::new(file.size())
+                );
 
                 let mut output_file = fs::File::create(file_path)?;
                 io::copy(&mut file, &mut output_file)?;
 
-                #[cfg(unix)]
+                set_last_modified_time(&file, file_path)?;
-                set_last_modified_time(&output_file, &file)?;
             }
         }
 
         #[cfg(unix)]
-        __unix_set_permissions(&file_path, &file)?;
+        unix_set_permissions(&file_path, &file)?;
 
         unpacked_files.push(file_path);
     }
@@ -228,55 +233,23 @@ fn display_zip_comment_if_exists(file: &ZipFile) {
     }
 }
 
-#[cfg(unix)]
+fn set_last_modified_time(zip_file: &ZipFile, path: &Path) -> crate::Result<()> {
-/// Attempts to convert a [`zip::DateTime`] to a [`libc::timespec`].
+    let modification_time_in_seconds = zip_file
-fn convert_zip_date_time(date_time: zip::DateTime) -> Option<libc::timespec> {
+        .last_modified()
-    use time::{Date, Month, PrimitiveDateTime, Time};
+        .to_time()
+        .expect("Zip archive contains a file with broken 'last modified time'")
+        .unix_timestamp();
 
-    // Safety: time::Month is repr(u8) and goes from 1 to 12
+    // Zip does not support nanoseconds, so we can assume zero here
-    let month: Month = unsafe { std::mem::transmute(date_time.month()) };
+    let modification_time = FileTime::from_unix_time(modification_time_in_seconds, 0);
 
-    let date = Date::from_calendar_date(date_time.year() as _, month, date_time.day()).ok()?;
+    set_file_mtime(path, modification_time)?;
-
-    let time = Time::from_hms(date_time.hour(), date_time.minute(), date_time.second()).ok()?;
-
-    let date_time = PrimitiveDateTime::new(date, time);
-    let timestamp = date_time.assume_utc().unix_timestamp();
-
-    Some(libc::timespec {
-        tv_sec: timestamp,
-        tv_nsec: 0,
-    })
-}
-
-#[cfg(unix)]
-fn set_last_modified_time(file: &fs::File, zip_file: &ZipFile) -> crate::Result<()> {
-    use std::os::unix::prelude::AsRawFd;
-
-    use libc::UTIME_NOW;
-
-    let now = libc::timespec {
-        tv_sec: 0,
-        tv_nsec: UTIME_NOW,
-    };
-
-    let last_modified = zip_file.last_modified();
-    let last_modified = convert_zip_date_time(last_modified).unwrap_or(now);
-
-    // The first value is the last accessed time, which we'll set as being right now.
-    // The second value is the last modified time, which we'll copy over from the zip archive
-    let times = [now, last_modified];
-
-    let output_fd = file.as_raw_fd();
-
-    // TODO: check for -1
-    unsafe { libc::futimens(output_fd, &times as *const _) };
 
     Ok(())
 }
 
 #[cfg(unix)]
-fn __unix_set_permissions(file_path: &Path, file: &ZipFile) -> crate::Result<()> {
+fn unix_set_permissions(file_path: &Path, file: &ZipFile) -> crate::Result<()> {
     use std::fs::Permissions;
 
     if let Some(mode) = file.unix_mode() {

src/utils/question.rs

@@ -60,8 +60,6 @@ pub fn create_or_ask_overwrite(path: &Path, question_policy: QuestionPolicy) ->
         Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
             if user_wants_to_overwrite(path, question_policy)? {
                 if path.is_dir() {
-                    // We can't just use `fs::File::create(&path)` because it would return io::ErrorKind::IsADirectory
-                    // ToDo: Maybe we should emphasise that `path` is a directory and everything inside it will be gone?
                     fs::remove_dir_all(path)?;
                 }
                 Ok(Some(fs::File::create(path)?))