Repository: rdiff-backup/rdiff-backup (language: python)

Issue:
[BUG] rpath.delete fails on read-only files under Windows
## Bug summary

As part of the tests under Windows, a read-only file is copied as part of a tree, Windows keeps the read-only attributes (which IMHO is a difference to Linux). Then deleting the copied tree fails on shutil.rmtree because of this read-only file.

## Version, Python, Operating System

2.1.3b3etc (latest development version)

## rdiff-backup call

It's not directly an rdiff-backup call, just using `some_rpath.delete()` as part of the test suite `cmdlinetest.py`

## What happened and what did you expect?

`some_rpath.delete()`  resp. `shutil.rmtree(some_rpath.path)` fails on the one read-only file.

## More information

I think, I'd like some_rpath.delete() to call "over the line" a new function `conn.delete_local()` as to have more flexibility on what the code does locally to remove the tree without risk to fail.


Discussion / hints:
A specific case of #795 hence I delay the fixing until 2.4 (the issue exists since ever and didn't disturb a lot of people apparently).

Fix patch:
diff --git a/src/rdiff_backup/Security.py b/src/rdiff_backup/Security.py
index ff0b1d854..701371bfd 100644
--- a/src/rdiff_backup/Security.py
+++ b/src/rdiff_backup/Security.py
@@ -72,7 +72,8 @@ class Violation(Exception):
     "os.unlink": 0,
     "os.utime": 0,
     "rpath.make_file_dict": 0,
-    "rpath.delete_dir_no_files": 0
+    "rpath.delete_dir_no_files": 0,
+    "shutil.rmtree": 0,
 }
 
 # functions to set global values


Test patch:
diff --git a/testing/commontest.py b/testing/commontest.py
index e79692de0..5e0d868fe 100644
--- a/testing/commontest.py
+++ b/testing/commontest.py
@@ -165,12 +165,15 @@ def rdiff_backup_action(source_local, dest_local,
     """
     remote_exec = CMD_SEP.join([b"cd %s", b"%s server::%s"])
 
+    is_remote = False
     if src_dir and not source_local:
         src_dir = (remote_exec % (abs_remote1_dir, RBBin, src_dir))
+        is_remote = True
     if dest_dir and not dest_local:
         dest_dir = (remote_exec % (abs_remote2_dir, RBBin, dest_dir))
+        is_remote = True
 
-    if not (source_local and dest_local):
+    if is_remote:
         generic_opts = list(generic_opts) + [b"--remote-schema", b"{h}"]
 
     cmdargs = [RBBin] + list(generic_opts) + [action] + list(specific_opts)
diff --git a/testing/fileset.py b/testing/fileset.py
index e552bb5f1..f043be7ac 100644
--- a/testing/fileset.py
+++ b/testing/fileset.py
@@ -8,7 +8,6 @@
 # Distributions of rdiff-backup should include a copy of the GPL in a
 # file called COPYING.  The GPL is also available online at
 # https://www.gnu.org/copyleft/gpl.html.
-
 """
 This library allows to create a set of files (and directories), using a
 structure of dictionaries.
@@ -328,6 +327,11 @@ def _rmtree(set_path):
     for dir_name, dirs, files in os.walk(set_path):  # topdown
         mode = os.stat(dir_name).st_mode
         os.chmod(dir_name, mode | 0o222)
+        # Windows can't remove read-only files
+        for file_name in files:
+            file = os.path.join(dir_name, file_name)
+            mode = os.stat(file).st_mode
+            os.chmod(file, mode | 0o222)
     shutil.rmtree(set_path)
 
 
diff --git a/testing/readonly_actions_test.py b/testing/readonly_actions_test.py
index cfc13bb61..6a1317395 100644
--- a/testing/readonly_actions_test.py
+++ b/testing/readonly_actions_test.py
@@ -7,6 +7,8 @@
 import commontest as comtst
 import fileset
 
+from rdiff_backup import Globals, rpath
+
 
 class ActionReadOnlyTest(unittest.TestCase):
     """
@@ -27,9 +29,10 @@ def setUp(self):
                 "dirA": {"contents": {"fileA": {"content": "afterwards"}}},
                 "fileB": {"content": "now else"}
             }}}
+        rec = {"fmode": 0o400, "dmode": 0o500}
         self.from2_path = os.path.join(self.base_dir, b"from2")
-        fileset.create_fileset(self.base_dir, self.from1_struct)
-        fileset.create_fileset(self.base_dir, self.from2_struct)
+        fileset.create_fileset(self.base_dir, self.from1_struct, recurse=rec)
+        fileset.create_fileset(self.base_dir, self.from2_struct, recurse=rec)
         fileset.remove_fileset(self.base_dir, {"bak": {"type": "dir"}})
         self.bak_path = os.path.join(self.base_dir, b"bak")
 
@@ -48,7 +51,7 @@ def setUp(self):
     def test_readonly_regress(self):
         """test the "regress" action on a read-only repository"""
 
-        # then we regress forcefully
+        # we regress forcefully
         self.assertEqual(comtst.rdiff_backup_action(
             False, None, self.bak_path, None,
             ("--api-version", "201", "--force"),
@@ -60,7 +63,7 @@ def test_readonly_regress(self):
     def test_readonly_remove(self):
         """test the "remove" action on a read-only repository"""
 
-        # then we regress forcefully
+        # we remove forcefully
         self.assertEqual(comtst.rdiff_backup_action(
             True, None, self.bak_path, None,
             ("--api-version", "201", "--force"),
@@ -69,6 +72,17 @@ def test_readonly_remove(self):
         # all tests were successful
         self.success = True
 
+    @unittest.skip("Skipped until issue #790 can be fixed as part of 2.4")
+    def test_readonly_delete(self):
+        """test the "delete" method of rpath on a read-only repository"""
+
+        from1_rp = rpath.RPath(Globals.local_connection, self.from1_path)
+        from1_rp.delete()
+        self.assertIsNone(from1_rp.lstat())
+
+        # all tests were successful
+        self.success = True
+
     def tearDown(self):
         # we clean-up only if the test was successful
         if self.success: