Bugfix/1827 invalid cross device link on localFS commit (#1829)

Co-authored-by: Martin Durant <[email protected]>

Author: siemdejong

fsspec/implementations/local.py

@@ -430,12 +430,22 @@ def __getstate__(self):
     def commit(self):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
-        shutil.move(self.temp, self.path)
         try:
-            mask = 0o666
-            os.chmod(self.path, mask & ~get_umask(mask))
-        except RuntimeError:
-            pass
+            shutil.move(self.temp, self.path)
+        except PermissionError as e:
+            # shutil.move raises PermissionError if os.rename
+            # and the default copy2 fallback with shutil.copystats fail.
+            # The file should be there nonetheless, but without copied permissions.
+            # If it doesn't exist, there was no permission to create the file.
+            if not os.path.exists(self.path):
+                raise e
+        else:
+            # If PermissionError is not raised, permissions can be set.
+            try:
+                mask = 0o666
+                os.chmod(self.path, mask & ~get_umask(mask))
+            except RuntimeError:
+                pass
 
     def discard(self):
         if self.autocommit:

fsspec/implementations/tests/test_local.py

@@ -1,4 +1,5 @@
 import bz2
+import errno
 import gzip
 import os
 import os.path
@@ -562,6 +563,25 @@ def test_multiple_filesystems_use_umask_cache(tmpdir):
     assert get_umask.cache_info().hits == 1
 
 
+def test_transaction_cross_device_but_mock_temp_dir_on_wrong_device(tmpdir):
+    # If the temporary file for a transaction is not on the correct device,
+    # os.rename in shutil.move will raise EXDEV and lookup('chmod') will raise
+    # a PermissionError.
+    fs = LocalFileSystem()
+    with (
+        patch(
+            "os.rename",
+            side_effect=OSError(errno.EXDEV, "Invalid cross-device link"),
+        ),
+        patch(
+            "os.chmod",
+            side_effect=PermissionError("Operation not permitted"),
+        ),
+    ):
+        with fs.transaction, fs.open(tmpdir + "/afile", "wb") as f:
+            f.write(b"data")
+
+
 def test_make_path_posix():
     cwd = os.getcwd()
     if WIN: