initial implementation of timestamp preservation #166 (#360)

Closes #166

Cargo.lock

@@ -211,6 +211,18 @@ version = "1.6.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457"
 
+[[package]]
+name = "filetime"
+version = "0.2.13"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0c122a393ea57648015bf06fbd3d372378992e86b9ff5a7a497b076a28c79efe"
+dependencies = [
+ "cfg-if 1.0.0",
+ "libc",
+ "redox_syscall",
+ "winapi",
+]
+
 [[package]]
 name = "glob"
 version = "0.3.0"
@@ -395,6 +407,7 @@ dependencies = [
  "cloudflare-zlib",
  "crc",
  "crossbeam-channel",
+ "filetime",
  "image",
  "indexmap",
  "itertools",
@@ -455,6 +468,12 @@ dependencies = [
  "num_cpus",
 ]
 
+[[package]]
+name = "redox_syscall"
+version = "0.1.57"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "41cc0f7e4d5d4544e8861606a285bb08d3e70712ccc7d2b84d7c0ccfaf4b05ce"
+
 [[package]]
 name = "rgb"
 version = "0.8.25"

Cargo.toml

@@ -42,6 +42,10 @@ log = "0.4.14"
 stderrlog = { version = "0.5.1", optional = true }
 crossbeam-channel = "0.5.0"
 
+[dependencies.filetime]
+optional = true
+version = "0.2.13"
+
 [dependencies.rayon]
 optional = true
 version = "^1.5.0"
@@ -72,7 +76,7 @@ binary = [
     "wild",
     "stderrlog",
 ]
-default = ["binary", "parallel", "libdeflater", "zopfli"]
+default = ["binary", "filetime", "parallel", "libdeflater", "zopfli"]
 parallel = ["rayon", "indexmap/rayon"]
 
 [lib]

src/lib.rs

@@ -36,7 +36,7 @@ use image::{DynamicImage, GenericImageView, ImageFormat, Pixel};
 use log::{debug, error, info, warn};
 use rayon::prelude::*;
 use std::fmt;
-use std::fs::{copy, File};
+use std::fs::{copy, File, Metadata};
 use std::io::{stdin, stdout, BufWriter, Read, Write};
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -330,9 +330,29 @@ 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) => PngData::read_file(input_path)?,
+        InFile::Path(ref input_path) => {
+            if opts.preserve_attrs {
+                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)?;
+                debug!("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)
@@ -388,20 +408,27 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<(
                     err
                 ))
             })?;
-            if opts.preserve_attrs {
+            if let Some(metadata_input) = &opt_metadata_preserved {
-                if let Some(input_path) = input.path() {
+                copy_permissions(&metadata_input, &out_file)?;
-                    copy_permissions(input_path, &out_file);
-                }
             }
 
             let mut buffer = BufWriter::new(out_file);
-            buffer.write_all(&optimized_output).map_err(|e| {
+            buffer
-                PngError::new(&format!(
+                .write_all(&optimized_output)
-                    "Unable to write to {}: {}",
+                // flush BufWriter so IO errors don't get swallowed silently on close() by drop!
-                    output_path.display(),
+                .and_then(|()| buffer.flush())
-                    e
+                .map_err(|e| {
-                ))
+                    PngError::new(&format!(
-            })?;
+                        "Unable to write to {}: {}",
+                        output_path.display(),
+                        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)?;
+            }
             info!("Output: {}", output_path.display());
         }
     }
@@ -942,33 +969,98 @@ fn perform_backup(input_path: &Path) -> PngResult<()> {
 }
 
 #[cfg(not(unix))]
-fn copy_permissions(input_path: &Path, out_file: &File) {
+fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> {
-    if let Ok(f) = File::open(input_path) {
+    let readonly_input = metadata_input.permissions().readonly();
-        if let Ok(metadata) = f.metadata() {
+
-            if let Ok(out_meta) = out_file.metadata() {
+    out_file
-                let readonly = metadata.permissions().readonly();
+        .metadata()
-                out_meta.permissions().set_readonly(readonly);
+        .map_err(|err_io| {
-                return;
+            PngError::new(&format!(
-            }
+                "unable to read filesystem metadata of output file: {}",
-        }
+                err_io
-    };
+            ))
-    warn!("Failed to set permissions on output file");
+        })
+        .and_then(|out_meta| {
+            out_meta.permissions().set_readonly(readonly_input);
+            out_file
+                .metadata()
+                .map_err(|err_io| {
+                    PngError::new(&format!(
+                        "unable to re-read filesystem metadata of output file: {}",
+                        err_io
+                    ))
+                })
+                .and_then(|out_meta_reread| {
+                    if out_meta_reread.permissions().readonly() != readonly_input {
+                        Err(PngError::new(&format!(
+                            "failed to set readonly, expected: {}, found: {}",
+                            readonly_input,
+                            out_meta_reread.permissions().readonly()
+                        )))
+                    } else {
+                        Ok(())
+                    }
+                })
+        })
 }
 
 #[cfg(unix)]
-fn copy_permissions(input_path: &Path, out_file: &File) {
+fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> {
     use std::os::unix::fs::PermissionsExt;
 
-    if let Ok(f) = File::open(input_path) {
+    let permissions = metadata_input.permissions().mode();
-        if let Ok(metadata) = f.metadata() {
+
-            if let Ok(out_meta) = out_file.metadata() {
+    out_file
-                let permissions = metadata.permissions().mode();
+        .metadata()
-                out_meta.permissions().set_mode(permissions);
+        .map_err(|err_io| {
-                return;
+            PngError::new(&format!(
-            }
+                "unable to read filesystem metadata of output file: {}",
-        }
+                err_io
-    };
+            ))
-    warn!("Failed to set permissions on output file");
+        })
+        .and_then(|out_meta| {
+            out_meta.permissions().set_mode(permissions);
+            out_file
+                .metadata()
+                .map_err(|err_io| {
+                    PngError::new(&format!(
+                        "unable to re-read filesystem metadata of output file: {}",
+                        err_io
+                    ))
+                })
+                .and_then(|out_meta_reread| {
+                    if out_meta_reread.permissions().mode() != permissions {
+                        Err(PngError::new(&format!(
+                            "failed to set permissions, expected: {:04o}, found: {:04o}",
+                            permissions,
+                            out_meta_reread.permissions().mode()
+                        )))
+                    } else {
+                        Ok(())
+                    }
+                })
+        })
+}
+
+#[cfg(not(feature = "filetime"))]
+fn copy_times(_: &Metadata, _: &Path) -> PngResult<()> {
+    Ok(())
+}
+
+#[cfg(feature = "filetime")]
+fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> {
+    let atime = filetime::FileTime::from_last_access_time(input_path_meta);
+    let mtime = filetime::FileTime::from_last_modification_time(input_path_meta);
+    debug!(
+        "attempting to set file times: atime: {:?}, mtime: {:?}",
+        atime, mtime
+    );
+    filetime::set_file_times(out_path, atime, mtime).map_err(|err_io| {
+        PngError::new(&format!(
+            "unable to set file times on {:?}: {}",
+            out_path, err_io
+        ))
+    })
 }
 
 /// Compares images pixel by pixel for equivalent content

tests/flags.rs

@@ -1,7 +1,11 @@
 use indexmap::IndexSet;
 use oxipng::internal_tests::*;
 use oxipng::{InFile, OutFile};
+#[cfg(feature = "filetime")]
+use std::cell::RefCell;
 use std::fs::remove_file;
+#[cfg(feature = "filetime")]
+use std::ops::Deref;
 use std::path::Path;
 use std::path::PathBuf;
 
@@ -20,7 +24,8 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) {
     )
 }
 
-fn test_it_converts(
+/// Add callback to allow checks before the output file is deleted again
+fn test_it_converts_callbacks<CBPRE, CBPOST>(
     input: PathBuf,
     output: &OutFile,
     opts: &oxipng::Options,
@@ -28,19 +33,28 @@ fn test_it_converts(
     bit_depth_in: BitDepth,
     color_type_out: ColorType,
     bit_depth_out: BitDepth,
-) {
+    mut callback_pre: CBPRE,
+    mut callback_post: CBPOST,
+) where
+    CBPOST: FnMut(&Path) -> (),
+    CBPRE: FnMut(&Path) -> (),
+{
     let png = PngData::new(&input, opts.fix_errors).unwrap();
 
     assert_eq!(png.raw.ihdr.color_type, color_type_in);
     assert_eq!(png.raw.ihdr.bit_depth, bit_depth_in);
 
-    match oxipng::optimize(&InFile::Path(input), output, opts) {
+    callback_pre(&input);
+
+    match oxipng::optimize(&InFile::Path(input), &output, &opts) {
         Ok(_) => (),
         Err(x) => panic!("{}", x),
     };
     let output = output.path().unwrap();
     assert!(output.exists());
 
+    callback_post(&output);
+
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
@@ -55,6 +69,29 @@ fn test_it_converts(
     remove_file(output).ok();
 }
 
+/// Shim for new callback functionality
+fn test_it_converts(
+    input: PathBuf,
+    output: &OutFile,
+    opts: &oxipng::Options,
+    color_type_in: ColorType,
+    bit_depth_in: BitDepth,
+    color_type_out: ColorType,
+    bit_depth_out: BitDepth,
+) {
+    test_it_converts_callbacks(
+        input,
+        output,
+        opts,
+        color_type_in,
+        bit_depth_in,
+        color_type_out,
+        bit_depth_out,
+        |_| {},
+        |_| {},
+    )
+}
+
 #[test]
 fn verbose_mode() {
     use crossbeam_channel::{unbounded, Sender};
@@ -446,10 +483,53 @@ fn interlaced_0_to_1_other_filter_mode() {
 #[test]
 fn preserve_attrs() {
     let input = PathBuf::from("tests/files/preserve_attrs.png");
+
+    #[cfg(feature = "filetime")]
+    let atime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0));
+    #[cfg(feature = "filetime")]
+    let mtime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0));
+
     let (output, mut opts) = get_opts(&input);
     opts.preserve_attrs = true;
 
-    test_it_converts(
+    #[cfg(feature = "filetime")]
+    let callback_pre = |path_in: &Path| {
+        let meta_input = path_in
+            .metadata()
+            .expect("unable to get file metadata for output file");
+
+        atime_canon.replace(filetime::FileTime::from_last_access_time(&meta_input));
+        mtime_canon.replace(filetime::FileTime::from_last_modification_time(&meta_input));
+    };
+    #[cfg(not(feature = "filetime"))]
+    let callback_pre = |_: &Path| {};
+
+    #[cfg(feature = "filetime")]
+    let callback_post = |path_out: &Path| {
+        let meta_output = path_out
+            .metadata()
+            .expect("unable to get file metadata for output file");
+
+        let cellref_atime_canon = atime_canon.borrow();
+        let cellref_mtime_canon = mtime_canon.borrow();
+        let ref_atime_canon: &filetime::FileTime = cellref_atime_canon.deref();
+        let ref_mtime_canon: &filetime::FileTime = cellref_mtime_canon.deref();
+
+        assert_eq!(
+            ref_atime_canon,
+            &filetime::FileTime::from_last_access_time(&meta_output),
+            "expected access time to be identical to that of input",
+        );
+        assert_eq!(
+            ref_mtime_canon,
+            &filetime::FileTime::from_last_modification_time(&meta_output),
+            "expected modification time to be identical to that of input",
+        );
+    };
+    #[cfg(not(feature = "filetime"))]
+    let callback_post = |_: &Path| {};
+
+    test_it_converts_callbacks(
         input,
         &output,
         &opts,
@@ -457,6 +537,8 @@ fn preserve_attrs() {
         BitDepth::Eight,
         ColorType::RGB,
         BitDepth::Eight,
+        callback_pre,
+        callback_post,
     );
 
     // TODO: Actually check permissions