Trash: Clean up discarded symlinks properly

Author: jorio

gitfourchette/pycompat.py

@@ -55,13 +55,27 @@ def _pathWalk(self: _Path, top_down=True, on_error=None, follow_symlinks=False):
 # Python 3.10, 3.11 compatibility
 # (`follow_symlinks` argument in Path.exists() is new in Python 3.12)
 if _sys.version_info < (3, 12):
-    def _pathExists(self: _Path, follow_symlinks=True):
-        return _pathExistsVanilla(self) if follow_symlinks else _os.path.lexists(self)
+    def _pathExists(self: _Path, follow_symlinks=True) -> bool:
+        if follow_symlinks:
+            return _pathExistsVanilla(self)
+        return _os.path.lexists(self)
 
     _pathExistsVanilla = _Path.exists
     _Path.exists = _pathExists
 
 
+# Python 3.10, 3.11, 3.12 compatibility
+# (`follow_symlinks` argument in Path.is_file() is new in Python 3.13)
+if _sys.version_info < (3, 13):
+    def _pathIsFile(self: _Path, follow_symlinks=True) -> bool:
+        if follow_symlinks:
+            return _pathIsFileVanilla(self)
+        return not self.is_symlink() and _pathIsFileVanilla(self)
+
+    _pathIsFileVanilla = _Path.is_file
+    _Path.is_file = _pathIsFile
+
+
 # Python 3.10, 3.11, 3.12 compatibility (glob.translate is new Python 3.13)
 # Adapted from cpython/Lib/glob.py
 if not hasattr(_glob, "translate"):

gitfourchette/trash.py

@@ -78,7 +78,9 @@ def exists(self) -> bool:
         return self.trashDir.is_dir()
 
     def pathIsTrashManaged(self, p: Path) -> bool:
-        return p.is_relative_to(self.trashDir) and (p.is_file() or p.is_symlink())
+        if not p.is_relative_to(self.trashDir):
+            return False
+        return p.is_file(follow_symlinks=False) or p.is_symlink()
 
     def refreshFiles(self):
         self.trashFiles.clear()
@@ -89,7 +91,7 @@ def refreshFiles(self):
     def makeRoom(self, maxFiles: int):
         while len(self.trashFiles) > maxFiles:
             f = self.trashFiles.pop()
-            if f.is_file():
+            if self.pathIsTrashManaged(f):
                 logger.debug(f"Deleting trash file {f}")
                 f.unlink()
 

test/test_trash.py

@@ -15,10 +15,17 @@ def _fillTrashWithJunk(n):
     trash = Trash.instance()
     trash.refreshFiles()
     trash.clear()
-    os.makedirs(trash.trashDir, exist_ok=True)
+    trash.trashDir.mkdir(parents=True, exist_ok=True)
+
     for i in range(n):
-        with open(F"{trash.trashDir}/19991231T235900-test{i}.txt", "w") as junk:
-            junk.write(F"test{i}")
+        junkPath = Path(trash.trashDir, f"19991231T235900-test{i}.txt")
+
+        # Alternate between bogus text files and bogus symlinks
+        if i % 2 == 0:
+            junkPath.write_text(f"test{i}")
+        else:
+            junkPath.symlink_to(trash.trashDir)
+
     trash.refreshFiles()
 
 
@@ -109,20 +116,26 @@ def testTrashFull(tempDir, mainWindow):
     # Trash should have been purged to make room for new patch
     trashInstance = Trash.instance()
     assert len(trashInstance.trashFiles) == settings.prefs.maxTrashFiles
+    assert len(list(trashInstance.trashDir.iterdir())) == settings.prefs.maxTrashFiles
     assert "a1.txt" in trashInstance.trashFiles[0].name
 
 
 def testClearTrash(mainWindow):
-    assert Trash.instance().count() == 0
+    trashInstance = Trash.instance()
+    assert trashInstance.count() == 0
 
     mainWindow.clearRescueFolder()
     acceptQMessageBox(mainWindow, "no discarded (patches|changes) to delete")
 
     _fillTrashWithJunk(40)
-    assert Trash.instance().count() == 40
+    assert trashInstance.count() == 40
     mainWindow.clearRescueFolder()
     acceptQMessageBox(mainWindow, "delete.+40.+discarded (patches|changes)")
-    assert Trash.instance().count() == 0
+    assert trashInstance.count() == 0
+
+    trashInstance.refreshFiles()
+    assert trashInstance.count() == 0
+    assert not list(trashInstance.trashDir.iterdir())
 
 
 def testOpenTrashFolder(mainWindow):