Rework preserving attributes (#759)

Cargo.lock

@@ -245,18 +245,6 @@ dependencies = [
  "simd-adler32",
 ]
 
-[[package]]
-name = "filetime"
-version = "0.2.26"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bc0505cd1b6fa6580283f6bdf70a73fcf4aba1184038c90902b92b3dd0df63ed"
-dependencies = [
- "cfg-if",
- "libc",
- "libredox",
- "windows-sys 0.60.2",
-]
-
 [[package]]
 name = "find-msvc-tools"
 version = "0.1.2"
@@ -346,17 +334,6 @@ dependencies = [
  "libdeflate-sys",
 ]
 
-[[package]]
-name = "libredox"
-version = "0.1.10"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "416f7e718bdb06000964960ffa43b4335ad4012ae8b99060261aa4a8088d5ccb"
-dependencies = [
- "bitflags",
- "libc",
- "redox_syscall",
-]
-
 [[package]]
 name = "linux-raw-sys"
 version = "0.11.0"
@@ -412,7 +389,6 @@ dependencies = [
  "clap",
  "crossbeam-channel",
  "env_logger",
- "filetime",
  "glob",
  "image",
  "indexmap",
@@ -485,15 +461,6 @@ dependencies = [
  "crossbeam-utils",
 ]
 
-[[package]]
-name = "redox_syscall"
-version = "0.5.17"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5407465600fb0548f1442edf71dd20683c6ed326200ace4b1ef0763521bb3b77"
-dependencies = [
- "bitflags",
-]
-
 [[package]]
 name = "rgb"
 version = "0.8.52"

Cargo.toml

@@ -56,10 +56,6 @@ version = "0.11.8"
 optional = true
 version = "0.5.15"
 
-[dependencies.filetime]
-optional = true
-version = "0.2.26"
-
 [dependencies.rayon]
 optional = true
 version = "1.11.0"
@@ -85,12 +81,11 @@ version = "0.25.9"
 
 [features]
 binary = ["dep:clap", "dep:glob", "dep:env_logger", "dep:parse-size"]
-default = ["binary", "parallel", "zopfli", "filetime"]
+default = ["binary", "parallel", "zopfli"]
 parallel = ["dep:rayon", "indexmap/rayon", "dep:crossbeam-channel"]
 freestanding = ["libdeflater/freestanding"]
 sanity-checks = ["dep:image"]
 zopfli = ["dep:zopfli"]
-filetime = ["dep:filetime"]
 system-libdeflate = ["libdeflater/dynamic"]
 
 [lib]

src/lib.rs

@@ -7,9 +7,9 @@ extern crate rayon;
 mod rayon;
 
 use std::{
-    fs::{File, Metadata},
+    fs::File,
     io::{BufWriter, Read, Write, stdin, stdout},
-    path::{Path, PathBuf},
+    path::PathBuf,
     sync::{
         Arc,
         atomic::{AtomicBool, Ordering},
@@ -171,34 +171,9 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<(
 
     let deadline = Arc::new(Deadline::new(opts.timeout));
 
-    // grab metadata before even opening input file to preserve atime
-    let opt_metadata_preserved;
     let in_data = match *input {
-        InFile::Path(ref input_path) => {
+        InFile::Path(ref input_path) => PngData::read_file(input_path)?,
-            if matches!(
-                output,
-                OutFile::Path {
-                    preserve_attrs: true,
-                    ..
-                }
-            ) {
-                opt_metadata_preserved = input_path
-                    .metadata()
-                    .map_err(|err| {
-                        // Fail if metadata cannot be preserved
-                        PngError::new(&format!(
-                            "Unable to read metadata from input file {input_path:?}: {err}"
-                        ))
-                    })
-                    .map(Some)?;
-                trace!("preserving metadata: {opt_metadata_preserved:?}");
-            } else {
-                opt_metadata_preserved = None;
-            }
-            PngData::read_file(input_path)?
-        }
         InFile::StdIn => {
-            opt_metadata_preserved = None;
             let mut data = Vec::new();
             stdin()
                 .read_to_end(&mut data)
@@ -253,17 +228,32 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<(
                 .write_all(&optimized_output)
                 .map_err(|e| PngError::WriteFailed("stdout".into(), e))?;
         }
-        (OutFile::Path { path, .. }, _) => {
+        (
+            OutFile::Path {
+                path,
+                preserve_attrs,
+            },
+            _,
+        ) => {
+            let input_metadata = if *preserve_attrs {
+                input.path().and_then(|in_path| {
+                    let meta = in_path.metadata();
+                    if let Err(e) = &meta {
+                        warn!("Unable to read metadata from {in_path:?}: {e}");
+                    }
+                    meta.ok()
+                })
+            } else {
+                None
+            };
+
             let output_path = path
                 .as_ref()
                 .map_or_else(|| input.path().unwrap(), PathBuf::as_path);
             let out_file = File::create(output_path)
                 .map_err(|err| PngError::WriteFailed(output_path.display().to_string(), err))?;
-            if let Some(metadata_input) = &opt_metadata_preserved {
-                copy_permissions(metadata_input, &out_file)?;
-            }
 
-            let mut buffer = BufWriter::new(out_file);
+            let mut buffer = BufWriter::new(&out_file);
             buffer
                 .write_all(&optimized_output)
                 // flush BufWriter so IO errors don't get swallowed silently on close() by drop!
@@ -271,8 +261,18 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<(
                 .map_err(|e| PngError::WriteFailed(output_path.display().to_string(), e))?;
             // force drop and thereby closing of file handle before modifying any timestamp
             std::mem::drop(buffer);
-            if let Some(metadata_input) = &opt_metadata_preserved {
+
-                copy_times(metadata_input, output_path)?;
+            if let Some(metadata_input) = &input_metadata {
+                let set_time = metadata_input
+                    .modified()
+                    .and_then(|m| out_file.set_modified(m));
+                if let Err(e) = set_time {
+                    warn!("Unable to set modification time on {output_path:?}: {e}");
+                }
+                let set_perm = out_file.set_permissions(metadata_input.permissions());
+                if let Err(e) = set_perm {
+                    warn!("Unable to set permissions on {output_path:?}: {e}");
+                }
             }
             info!("{}: {}", savings, output_path.display());
         }
@@ -642,29 +642,3 @@ fn recompress_frames(
 const fn is_fully_optimized(original_size: usize, optimized_size: usize, opts: &Options) -> bool {
     original_size <= optimized_size && !opts.force
 }
-
-fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> {
-    out_file
-        .set_permissions(metadata_input.permissions())
-        .map_err(|err_io| {
-            PngError::new(&format!(
-                "Unable to set permissions for output file: {err_io}"
-            ))
-        })
-}
-
-#[cfg(not(feature = "filetime"))]
-const fn copy_times(_: &Metadata, _: &Path) -> PngResult<()> {
-    Ok(())
-}
-
-#[cfg(feature = "filetime")]
-fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> {
-    let mtime = filetime::FileTime::from_last_modification_time(input_path_meta);
-    trace!("attempting to set file modification time: {mtime:?}");
-    filetime::set_file_mtime(out_path, mtime).map_err(|err_io| {
-        PngError::new(&format!(
-            "Unable to set file times on {out_path:?}: {err_io}"
-        ))
-    })
-}

tests/flags.rs

@@ -430,12 +430,9 @@ fn interlaced_0_to_1_other_filter_mode() {
 fn preserve_attrs() {
     let input = PathBuf::from("tests/files/preserve_attrs.png");
 
-    #[cfg(feature = "filetime")]
     let meta_input = input
         .metadata()
-        .expect("unable to get file metadata for output file");
+        .expect("unable to get metadata for input file");
-    #[cfg(feature = "filetime")]
-    let mtime_canon = filetime::FileTime::from_last_modification_time(&meta_input);
 
     let (mut output, opts) = get_opts(&input);
     if let OutFile::Path { preserve_attrs, .. } = &mut output {
@@ -449,17 +446,6 @@ fn preserve_attrs() {
     let output = output.path().unwrap();
     assert!(output.exists());
 
-    #[cfg(feature = "filetime")]
-    let meta_output = output
-        .metadata()
-        .expect("unable to get file metadata for output file");
-    #[cfg(feature = "filetime")]
-    assert_eq!(
-        &mtime_canon,
-        &filetime::FileTime::from_last_modification_time(&meta_output),
-        "expected modification time to be identical to that of input",
-    );
-
     match PngData::new(output, &opts) {
         Ok(x) => x,
         Err(x) => {
@@ -468,9 +454,21 @@ fn preserve_attrs() {
         }
     };
 
-    remove_file(output).ok();
+    let meta_output = output
+        .metadata()
+        .expect("unable to get metadata for output file");
+    assert_eq!(
+        meta_input.modified().ok(),
+        meta_output.modified().ok(),
+        "expected modification time to be identical to that of input",
+    );
+    assert_eq!(
+        meta_input.permissions(),
+        meta_output.permissions(),
+        "expected permissions to be identical to that of input",
+    );
 
-    // TODO: Actually check permissions
+    remove_file(output).ok();
 }
 
 #[test]