From 1d74044536dc5872a810e619784e5e01d7f1a47a Mon Sep 17 00:00:00 2001 From: moschroe Date: Wed, 9 Dec 2020 23:59:25 +0100 Subject: [PATCH] initial implementation of attribute preservation #166 --- Cargo.lock | 19 +++++++++ Cargo.toml | 1 + src/lib.rs | 102 ++++++++++++++++++++++++++++++++++--------------- tests/flags.rs | 80 +++++++++++++++++++++++++++++++++++--- 4 files changed, 166 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 78507913..cdcb1b03 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -224,6 +224,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" @@ -408,6 +420,7 @@ dependencies = [ "cloudflare-zlib", "crc", "crossbeam-channel", + "filetime", "image", "indexmap", "itertools", @@ -468,6 +481,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" diff --git a/Cargo.toml b/Cargo.toml index dac29f1a..d8663c08 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ libdeflater = { version = "0.7.1", optional = true } log = "0.4.11" stderrlog = { version = "0.5.0", optional = true } crossbeam-channel = "0.5.0" +filetime = "0.2.13" [dependencies.rayon] optional = true diff --git a/src/lib.rs b/src/lib.rs index 081d1a60..c8e4a778 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,7 +35,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}; @@ -324,9 +324,30 @@ 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| { + // TODO: Fail if input and output file are the same and metadata cannot be preserved? + warn!( + "Unable to read metadata from input file {:?}: {}", + input_path, err + ); + err + }) + .ok(); + 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) @@ -382,20 +403,27 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( err )) })?; - if opts.preserve_attrs { - if let Some(input_path) = input.path() { - copy_permissions(input_path, &out_file); - } + if let Some(metadata_input) = &opt_metadata_preserved { + copy_permissions(&metadata_input, &out_file); } let mut buffer = BufWriter::new(out_file); - buffer.write_all(&optimized_output).map_err(|e| { - PngError::new(&format!( - "Unable to write to {}: {}", - output_path.display(), - e - )) - })?; + buffer + .write_all(&optimized_output) + // flush BufWriter so IO errors don't get swallowed silently on close() by drop! + .and_then(|()| buffer.flush()) + .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()); } } @@ -932,35 +960,47 @@ fn perform_backup(input_path: &Path) -> PngResult<()> { } #[cfg(not(unix))] -fn copy_permissions(input_path: &Path, out_file: &File) { - if let Ok(f) = File::open(input_path) { - if let Ok(metadata) = f.metadata() { - if let Ok(out_meta) = out_file.metadata() { - let readonly = metadata.permissions().readonly(); - out_meta.permissions().set_readonly(readonly); - return; - } - } - }; +fn copy_permissions(metadata_input: &Metadata, out_file: &File) { + if let Ok(out_meta) = out_file.metadata() { + let readonly = metadata_input.permissions().readonly(); + out_meta.permissions().set_readonly(readonly); + return; + } warn!("Failed to set permissions on output file"); } #[cfg(unix)] -fn copy_permissions(input_path: &Path, out_file: &File) { +fn copy_permissions(metadata_input: &Metadata, out_file: &File) { use std::os::unix::fs::PermissionsExt; - if let Ok(f) = File::open(input_path) { - if let Ok(metadata) = f.metadata() { - if let Ok(out_meta) = out_file.metadata() { - let permissions = metadata.permissions().mode(); - out_meta.permissions().set_mode(permissions); - return; + let permissions = metadata_input.permissions().mode(); + if let Ok(out_meta) = out_file.metadata() { + out_meta.permissions().set_mode(permissions); + if let Ok(out_meta_reread) = out_file.metadata() { + if out_meta_reread.permissions().mode() != permissions { + warn!("Failed to set input file permissions on output file"); } + } else { + warn!("Failed to read newly-set permissions on output file"); } - }; + return; + } warn!("Failed to set permissions on output file"); } +fn copy_times(input_path_meta: &Metadata, out_path: &Path) { + 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 + ); + if let Err(err) = filetime::set_file_times(out_path, atime, mtime) { + warn!("Failed to set input file access/modification time on output file"); + debug!("Error: {:?}", err); + } +} + /// Compares images pixel by pixel for equivalent content fn images_equal(old_png: &DynamicImage, new_png: &DynamicImage) -> bool { let a = old_png.pixels().filter(|x| { diff --git a/tests/flags.rs b/tests/flags.rs index 69d79f6a..ae13a196 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -1,7 +1,9 @@ use indexmap::IndexSet; use oxipng::internal_tests::*; use oxipng::{InFile, OutFile}; +use std::cell::RefCell; use std::fs::remove_file; +use std::ops::Deref; use std::path::Path; use std::path::PathBuf; @@ -18,7 +20,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( input: PathBuf, output: &OutFile, opts: &oxipng::Options, @@ -26,12 +29,19 @@ 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); + callback_pre(&input); + match oxipng::optimize(&InFile::Path(input), &output, &opts) { Ok(_) => (), Err(x) => panic!("{}", x), @@ -39,6 +49,8 @@ fn test_it_converts( 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) => { @@ -53,6 +65,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}; @@ -444,10 +479,45 @@ fn interlaced_0_to_1_other_filter_mode() { #[test] fn preserve_attrs() { let input = PathBuf::from("tests/files/preserve_attrs.png"); + + let atime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0)); + 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( + 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)); + }; + + 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", + ); + }; + + test_it_converts_callbacks( input, &output, &opts, @@ -455,9 +525,9 @@ fn preserve_attrs() { BitDepth::Eight, ColorType::RGB, BitDepth::Eight, + callback_pre, + callback_post, ); - - // TODO: Actually check permissions } #[test]