From c2e9a78ddfb244f44e6c67d091f1a1d73d790be4 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Tue, 6 Jun 2023 11:33:06 +0530 Subject: [PATCH] Windows: Make moving files in the calibre library folder more robust Now even folders are opened and locked at the start of the operation not just files. Also folders have their attributes reset to normal in case they are marked readonly, not just files. --- src/calibre/utils/copy_files.py | 32 +++++++++++++++++++++++----- src/calibre/utils/copy_files_test.py | 8 +++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/calibre/utils/copy_files.py b/src/calibre/utils/copy_files.py index a0aa55fcf7..33d6a997ef 100644 --- a/src/calibre/utils/copy_files.py +++ b/src/calibre/utils/copy_files.py @@ -8,7 +8,7 @@ import stat import time from collections import defaultdict from contextlib import suppress -from typing import Callable, Dict, Set, Tuple, Union +from typing import Callable, Dict, Set, Tuple, Union, List from calibre.constants import filesystem_encoding, iswindows from calibre.utils.filenames import make_long_path_useable, samefile, windows_hardlink @@ -28,6 +28,9 @@ class UnixFileCopier: def register(self, path: str, dest: str) -> None: self.copy_map[path] = dest + def register_folder(self, path: str) -> None: + pass + def __enter__(self) -> None: pass @@ -64,6 +67,8 @@ class WindowsFileCopier: self.path_to_fileid_map : Dict[str, WindowsFileId] = {} self.fileid_to_paths_map: Dict[WindowsFileId, Set[str]] = defaultdict(set) self.path_to_handle_map: Dict[str, 'winutil.Handle'] = {} + self.folder_to_handle_map: Dict[str, 'winutil.Handle'] = {} + self.folders: List[str] = [] self.copy_map: Dict[str, str] = {} def register(self, path: str, dest: str) -> None: @@ -73,11 +78,18 @@ class WindowsFileCopier: self.path_to_fileid_map[path] = winutil.get_file_id(make_long_path_useable(path)) self.copy_map[path] = dest - def _open_file(self, path: str, retry_on_sharing_violation: bool = True) -> 'winutil.Handle': + def register_folder(self, path: str) -> None: + with suppress(OSError): + # Ensure the folder is not read-only + winutil.set_file_attributes(make_long_path_useable(path), winutil.FILE_ATTRIBUTE_NORMAL) + self.path_to_fileid_map[path] = winutil.get_file_id(make_long_path_useable(path)) + self.folders.append(path) + + def _open_file(self, path: str, retry_on_sharing_violation: bool = True, is_folder: bool = False) -> 'winutil.Handle': + flags = winutil.FILE_FLAG_BACKUP_SEMANTICS if is_folder else winutil.FILE_FLAG_SEQUENTIAL_SCAN try: return winutil.create_file(make_long_path_useable(path), winutil.GENERIC_READ, - winutil.FILE_SHARE_DELETE, - winutil.OPEN_EXISTING, winutil.FILE_FLAG_SEQUENTIAL_SCAN) + winutil.FILE_SHARE_DELETE, winutil.OPEN_EXISTING, flags) except OSError as e: if e.winerror == winutil.ERROR_SHARING_VIOLATION: # The file could be a hardlink to an already opened file, @@ -88,7 +100,7 @@ class WindowsFileCopier: return self.path_to_handle_map[other] if retry_on_sharing_violation: time.sleep(WINDOWS_SLEEP_FOR_RETRY_TIME) - return self._open_file(path, False) + return self._open_file(path, False, is_folder) err = IOError(errno.EACCES, _('File is open in another program')) err.filename = path @@ -100,6 +112,8 @@ class WindowsFileCopier: self.fileid_to_paths_map[file_id].add(path) for src in self.copy_map: self.path_to_handle_map[src] = self._open_file(src) + for path in self.folders: + self.folder_to_handle_map[path] = self._open_file(path, is_folder=True) def __exit__(self, exc_type, exc_val, exc_tb) -> None: try: @@ -108,6 +122,8 @@ class WindowsFileCopier: finally: for h in self.path_to_handle_map.values(): h.close() + for h in self.folder_to_handle_map.values(): + h.close() def copy_all(self) -> None: for src_path, dest_path in self.copy_map.items(): @@ -133,6 +149,8 @@ class WindowsFileCopier: def delete_all_source_files(self) -> None: for src_path in self.copy_map: winutil.delete_file(make_long_path_useable(src_path)) + for path in reversed(self.folders): + os.rmdir(make_long_path_useable(path)) def get_copier(delete_all=False) -> Union[UnixFileCopier, WindowsFileCopier]: @@ -189,12 +207,14 @@ def copy_tree( copier = get_copier(delete_source) + copier.register_folder(src) for (dirpath, dirnames, filenames) in os.walk(src, onerror=raise_error): for d in dirnames: path = os.path.join(dirpath, d) dest = dest_from_entry(dirpath, d) os.makedirs(make_long_path_useable(dest), exist_ok=True) shutil.copystat(make_long_path_useable(path), make_long_path_useable(dest), follow_symlinks=False) + copier.register_folder(path) for f in filenames: path = os.path.join(dirpath, f) dest = dest_from_entry(dirpath, f) @@ -214,6 +234,8 @@ def copy_tree( if delete_source: try: shutil.rmtree(make_long_path_useable(src)) + except FileNotFoundError: + pass except OSError: if iswindows: time.sleep(WINDOWS_SLEEP_FOR_RETRY_TIME) diff --git a/src/calibre/utils/copy_files_test.py b/src/calibre/utils/copy_files_test.py index 4fd17cf33f..a73a103cb4 100644 --- a/src/calibre/utils/copy_files_test.py +++ b/src/calibre/utils/copy_files_test.py @@ -6,12 +6,15 @@ import shutil import tempfile import time import unittest +from contextlib import closing from calibre import walk from calibre.constants import iswindows from .copy_files import copy_tree, rename_files from .filenames import nlinks_file +if iswindows: + from calibre_extensions import winutil class TestCopyFiles(unittest.TestCase): @@ -97,6 +100,11 @@ class TestCopyFiles(unittest.TestCase): self.assertRaises(IOError, copy_tree, src, dest) self.ae(os.listdir(self.d()), ['sub']) self.assertFalse(tuple(walk(self.d()))) + h = winutil.create_file(self.s('sub'), winutil.GENERIC_READ, 0, winutil.OPEN_EXISTING, winutil.FILE_FLAG_BACKUP_SEMANTICS) + with closing(h): + self.assertRaises(IOError, copy_tree, src, dest) + self.ae(os.listdir(self.d()), ['sub']) + self.assertFalse(tuple(walk(self.d()))) def find_tests(): return unittest.defaultTestLoader.loadTestsFromTestCase(TestCopyFiles)