From 6a63c3e5be0070cbe4868af15c306b4358a7946a Mon Sep 17 00:00:00 2001
From: Laurent Monin <github@norz.org>
Date: Wed, 13 Mar 2019 14:30:02 +0100
Subject: [PATCH] Improve file times preservation using os.utime() ns parameter
 (py >= 3.3)

https://docs.python.org/3/library/os.html#os.utime
Since Python 3.3, ns parameter is available
"The best way to preserve exact times is to use the st_atime_ns and st_mtime_ns
fields from the os.stat() result object with the ns parameter to utime."

Make time preservation testable.
---
 picard/file.py    | 42 ++++++++++++++++++++++++++++----
 test/test_file.py | 61 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/picard/file.py b/picard/file.py
index 094602f37..79b8c7f33 100644
--- a/picard/file.py
+++ b/picard/file.py
@@ -82,6 +82,12 @@ class File(QtCore.QObject, Item):
         "format": 2,
     }
 
+    class PreserveTimesStatError(Exception):
+        pass
+
+    class PreserveTimesUtimeError(Exception):
+        pass
+
     def __init__(self, filename):
         super().__init__()
         self.filename = filename
@@ -203,6 +209,27 @@ class File(QtCore.QObject, Item):
             priority=2,
             thread_pool=self.tagger.save_thread_pool)
 
+    def _preserve_times(self, filename, func):
+        """Save filename times before calling func, and set them again"""
+        try:
+            # https://docs.python.org/3/library/os.html#os.utime
+            # Since Python 3.3, ns parameter is available
+            # The best way to preserve exact times is to use the st_atime_ns and st_mtime_ns
+            # fields from the os.stat() result object with the ns parameter to utime.
+            st = os.stat(filename)
+            times_ns = (st.st_atime_ns, st.st_mtime_ns)
+        except OSError as why:
+            errmsg = "Couldn't read timestamps from %r: %s" % (filename, why)
+            raise self.PreserveTimesStatError(errmsg) from None
+            # if we can't read original times, don't call func and let caller handle this
+        func()
+        try:
+            os.utime(filename, ns=times_ns)
+        except OSError as why:
+            errmsg = "Couldn't preserve timestamps for %r: %s" % (filename, why)
+            raise self.PreserveTimesUtimeError(errmsg) from None
+        return times_ns
+
     def _save_and_rename(self, old_filename, metadata):
         """Save the metadata."""
         # Check that file has not been removed since thread was queued
@@ -215,13 +242,18 @@ class File(QtCore.QObject, Item):
             return None
         new_filename = old_filename
         if not config.setting["dont_write_tags"]:
-            info = os.stat(old_filename)
-            self._save(old_filename, metadata)
+            save = partial(self._save, old_filename, metadata)
             if config.setting["preserve_timestamps"]:
                 try:
-                    os.utime(old_filename, (info.st_atime, info.st_mtime))
-                except OSError:
-                    log.warning("Couldn't preserve timestamp for %r", old_filename)
+                    self._preserve_times(old_filename, save)
+                except self.PreserveTimesStatError as why:
+                    log.warning(why)
+                    # we didn't save the file yet, bail out
+                    return None
+                except self.FilePreserveTimesUtimeError as why:
+                    log.warning(why)
+            else:
+                save()
         # Rename files
         if config.setting["rename_files"] or config.setting["move_files"]:
             new_filename = self._rename(old_filename, metadata)
diff --git a/test/test_file.py b/test/test_file.py
index 28b33b07e..4d43f9c1c 100644
--- a/test/test_file.py
+++ b/test/test_file.py
@@ -1,3 +1,7 @@
+import os
+import shutil
+from tempfile import mkdtemp
+
 from test.picardtestcase import PicardTestCase
 
 from picard.file import File
@@ -26,3 +30,60 @@ class DataObjectTest(PicardTestCase):
         self.assertEqual(42, self.file.discnumber)
         self.file.metadata['discnumber'] = 'FOURTYTWO'
         self.assertEqual(0, self.file.discnumber)
+
+
+class TestPreserveTimes(PicardTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.tmp_directory = mkdtemp()
+        filepath = os.path.join(self.tmp_directory, 'a.mp3')
+        self.file = File(filepath)
+
+    def tearDown(self):
+        shutil.rmtree(self.tmp_directory)
+
+    def _create_testfile(self):
+        # create a dummy file
+        with open(self.file.filename, 'w') as f:
+            f.write('xxx')
+
+    def _modify_testfile(self):
+        # dummy file modification, append data to it
+        with open(self.file.filename, 'a') as f:
+            f.write('yyy')
+
+    def _read_testfile(self):
+        with open(self.file.filename, 'r') as f:
+            return f.read()
+
+    def test_preserve_times(self):
+        self._create_testfile()
+
+        st = os.stat(self.file.filename)
+        expect = (st.st_mtime_ns, st.st_atime_ns)
+
+        # test if times are preserved
+        result = self.file._preserve_times(self.file.filename,
+                                           self._modify_testfile)
+        self.assertEqual(result, expect)
+
+        # ensure written data can be read back
+        self.assertEqual(self._read_testfile(), 'xxxyyy')
+
+    def test_preserve_times_nofile(self):
+
+        with self.assertRaises(self.file.PreserveTimesStatError):
+            self.file._preserve_times(self.file.filename,
+                                      self._modify_testfile)
+        with self.assertRaises(FileNotFoundError):
+            self._read_testfile()
+
+    def test_preserve_times_nofile_utime(self):
+        self._create_testfile()
+
+        def save():
+            os.remove(self.file.filename)
+
+        with self.assertRaises(self.file.PreserveTimesUtimeError):
+            result = self.file._preserve_times(self.file.filename, save)