Merge pull request #1027 from samschott/preserve-file-metadata

Preserve file metadata

pyproject.toml

@@ -41,6 +41,7 @@ dependencies = [
     "survey>=4.0,<6.0",
     "typing_extensions",
     "watchdog>=2.0.1",
+    "xattr",
 ]
 
 [project.readme]

src/maestral/sync.py

@@ -77,7 +77,6 @@ from .constants import (
     EXCLUDED_DIR_NAMES,
     MIGNORE_FILE,
     FILE_CACHE,
-    IS_MACOS,
 )
 from .exceptions import (
     SyncError,
@@ -3585,14 +3584,7 @@ class SyncEngine:
             # Preserve permissions of the destination file if we are only syncing an
             # update to the file content (Dropbox ID of the file remains the same).
             old_entry = self.get_index_entry(event.dbx_path_lower)
-            preserve_permissions = bool(old_entry and event.dbx_id == old_entry.dbx_id)
-
-            if preserve_permissions:
-                # Ignore FileModifiedEvent when changing permissions.
-                # Note that two FileModifiedEvents may be emitted on macOS.
-                ignore_events.append(FileModifiedEvent(event.local_path))
-                if IS_MACOS:
-                    ignore_events.append(FileModifiedEvent(event.local_path))
+            preserve_metadata = bool(old_entry and event.dbx_id == old_entry.dbx_id)
 
             if isfile(event.local_path):
                 # Ignore FileDeletedEvent when replacing old file.
@@ -3607,7 +3599,8 @@ class SyncEngine:
                     move(
                         tmp_fname,
                         event.local_path,
-                        preserve_dest_permissions=preserve_permissions,
+                        keep_target_permissions=preserve_metadata,
+                        keep_target_xattrs=preserve_metadata,
                         raise_error=True,
                     )
 

src/maestral/utils/path.py

@@ -14,6 +14,9 @@ import platform
 from stat import S_ISDIR
 from typing import List, Optional, Tuple, Callable, Iterator, Iterable, Union
 
+# third party imports
+import xattr
+
 # local imports
 from .hashing import DropboxContentHasher
 
@@ -355,7 +358,8 @@ def move(
     src_path: str,
     dest_path: str,
     raise_error: bool = False,
-    preserve_dest_permissions: bool = False,
+    keep_target_permissions: bool = False,
+    keep_target_xattrs: bool = False,
 ) -> Optional[OSError]:
     """
     Moves a file or folder from ``src_path`` to ``dest_path``. If either the source or
@@ -369,22 +373,31 @@ def move(
         by the move. Any existing **empty** folder will be replaced if the source is
         also a folder.
     :param raise_error: Whether to raise errors or return them.
-    :param preserve_dest_permissions: Whether to apply the permissions of the source
-        path to the destination path. Permissions will not be set recursively and may
-        will be set for symlinks if this is not supported by the platform, i.e., if
-        ``os.chmod not in os.supports_follow_symlinks``.
+    :param keep_target_permissions: Whether to preserve the permissions of a file at the
+        destination, if any.
+    :param keep_target_xattrs: Whether to preserve the extended attributes of a file at
+        the destination, if any.
     :returns: Any caught exception during the move.
     """
     err: Optional[OSError] = None
-    orig_mode: Optional[int] = None
 
-    if preserve_dest_permissions:
-        # save dest permissions
+    if keep_target_permissions:
         try:
-            orig_mode = os.lstat(dest_path).st_mode & 0o777
+            dest_mode = os.lstat(dest_path).st_mode & 0o777
+            follow_symlinks = os.chmod not in os.supports_follow_symlinks
+            os.chmod(src_path, dest_mode, follow_symlinks=follow_symlinks)
         except (FileNotFoundError, NotADirectoryError):
             pass
 
+    if keep_target_xattrs:
+        try:
+            dest_attrs = xattr.xattr(dest_path)
+            for key, value in dest_attrs.iteritems():
+                xattr.setxattr(src_path, key, value)
+        except OSError:
+            # Fail gracefully if extended attributes are not supported by the system.
+            pass
+
     try:
         os.rename(src_path, dest_path)
     except FileNotFoundError:
@@ -392,15 +405,6 @@ def move(
         pass
     except OSError as exc:
         err = exc
-    else:
-        if orig_mode:
-            # Reapply dest permissions. If the dest is a symlink, only apply permissions
-            # if this is supported for symlinks by the platform.
-            try:
-                if os.chmod in os.supports_follow_symlinks:
-                    os.chmod(dest_path, orig_mode, follow_symlinks=False)
-            except OSError:
-                pass
 
     if raise_error and err:
         raise err

tests/linked/integration/test_sync.py

@@ -450,10 +450,6 @@ def test_excluded_folder_cleared_on_deletion(m: Maestral) -> None:
     assert_no_errors(m)
 
 
-@pytest.mark.skipif(
-    os.chmod not in os.supports_follow_symlinks,
-    reason="chmod does not support follow_symlinks=False",
-)
 def test_unix_permissions(m: Maestral) -> None:
     """
     Tests that a newly downloaded file is created with default permissions for our

tests/offline/utils/test_path.py

@@ -1,3 +1,7 @@
+import os
+import stat
+
+import xattr
 import pytest
 
 from maestral.utils.path import (
@@ -5,10 +9,15 @@ from maestral.utils.path import (
     get_existing_equivalent_paths,
     is_fs_case_sensitive,
     is_child,
+    move,
 )
 from maestral.utils.appdirs import get_home_dir
 
 
+def touch(path: str) -> None:
+    open(path, "w").close()
+
+
 def test_normalized_path_exists(tmp_path):
     # Assert that an existing path is found, even when a different casing is used.
 
@@ -79,3 +88,34 @@ def test_is_child():
     assert is_child("/parent/path/child/", "/parent/path")
     assert not is_child("/parent/path", "/parent/path")
     assert not is_child("/path1", "/path2")
+
+
+def test_move_preserves_permissions(tmp_path):
+    src_path = str(tmp_path / "source.txt")
+    dest_path = str(tmp_path / "dest.txt")
+
+    touch(src_path)
+    touch(dest_path)
+
+    os.chmod(dest_path, stat.S_IEXEC)
+
+    move(src_path, dest_path, keep_target_permissions=True)
+
+    assert bool(os.stat(dest_path).st_mode & stat.S_IEXEC)
+
+
+def test_move_preserves_xattrs(tmp_path):
+    src_path = str(tmp_path / "source.txt")
+    dest_path = str(tmp_path / "dest.txt")
+
+    touch(src_path)
+    touch(dest_path)
+
+    try:
+        xattr.setxattr(dest_path, "com.samschott.maestral.test", "value".encode())
+    except OSError:
+        pytest.skip("Xattr not supported by this system")
+
+    move(src_path, dest_path, keep_target_xattrs=True)
+
+    assert xattr.getxattr(dest_path, "com.samschott.maestral.test") == "value".encode()