Repository: pkolaczk/fclones (language: rust)

Issue:
`fclones dedupe` results in updates to mtimes on directories
Running `fclones dedupe` on some directory tree results in mtimes being updated for directories containing files that were deduplicated. 

I don't know whether this should be addressed, because while mtimes may be desirable to preserve, the directories really were updated through file creation. This effect does make it less likely that I would want to use `fclones dedupe` on old directory trees with potentially informative mtimes, though.


Discussion / hints:
Given the `dedupe` action is almost a userspace-transparent operation I agree that mtimes timestamps should not be affected.

Where is the right place to collect the timestamps beforehand? If placed inside `reflink()` it would stat directories multiple times, and returning the result `io::Result<..>` is probably annoying. Though maybe a concurrent hashmap could be passed along to skip that step if possible.

Where could it be done earlier? The iterator passed to `run_script()` is lazy so the stat'ing has to be woven somewhere into it - and it must reduce the result to unique directories. Not sure if this "dual flow" of files and directories is really possible.

Then the space-efficient `fclones::Path` has to be converted to a `PathBuf` to do the `stat` - should this be kept or should the conversion be repeated later when restoring the timestamps? Also, all other operations don't need that save-restore step at all, so skipping all this should not be expensive.

Fix patch:
diff --git a/src/reflink.rs b/src/reflink.rs
index 053c22f..32ae3a0 100644
--- a/src/reflink.rs
+++ b/src/reflink.rs
@@ -14,6 +14,12 @@ use crate::Path as FcPath;
 /// The destination file is allowed to exist.
 pub fn reflink(src: &FileMetadata, dest: &FileMetadata, log: &Log) -> io::Result<()> {
     let _ = FileLock::new(&dest.path)?; // don't touch a locked file
+
+    // Remember the original metadata of the parent directory of the destination file:
+    let dest_parent = dest.path.parent();
+    let dest_parent_metadata = dest_parent.map(|p| p.to_path_buf().metadata());
+
+    // Call reflink:
     let result = {
         if cfg!(any(target_os = "linux", target_os = "android")) && !test::cfg::crosstest() {
             linux_reflink(src, dest, log).map_err(|e| {
@@ -26,9 +32,23 @@ pub fn reflink(src: &FileMetadata, dest: &FileMetadata, log: &Log) -> io::Result
             safe_reflink(src, dest, log)
         }
     };
-    if let Err(err) = restore_some_metadata(&dest.path.to_path_buf(), &dest.metadata) {
-        log.warn(format!("Failed keep metadata for {}: {:?}", dest, err))
+
+    // Restore the original metadata of the deduplicated file:
+    if let Err(e) = restore_some_metadata(&dest.path.to_path_buf(), &dest.metadata) {
+        log.warn(format!("Failed keep metadata for {}: {}", dest, e))
+    }
+
+    // Restore the original metadata of the deduplicated files's parent directory:
+    if let Some(parent) = dest_parent {
+        if let Some(metadata) = dest_parent_metadata {
+            let result = metadata
+                .and_then(|metadata| restore_some_metadata(&parent.to_path_buf(), &metadata));
+            if let Err(e) = result {
+                log.warn(format!("Failed keep metadata for {}: {}", parent, e))
+            }
+        }
     }
+
     result
 }
 


Test patch: