From 99d48e20f68937c8a98c79981594992406dc2303 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 15 May 2025 12:04:43 +0200 Subject: [PATCH 1/5] fix(delete): always print warnings when file deletions fail Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 7c5c527c4d757..a4c0146ac7420 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -101,6 +101,7 @@ void PropagateLocalRemove::start() if (_moveToTrash && propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) { if ((QDir(filename).exists() || FileSystem::fileExists(filename)) && !FileSystem::moveToTrash(filename, &removeError)) { + qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << removeError; done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); return; } @@ -118,6 +119,7 @@ void PropagateLocalRemove::start() const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite}; if (!FileSystem::remove(filename, &removeError)) { + qCWarning(lcPropagateLocalRemove()) << "remove failed" << filename << removeError; done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); return; } From 584c81321fcef17202dcabb9a38a8f262fd83e9f Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 15 May 2025 12:28:10 +0200 Subject: [PATCH 2/5] fix(delete): optimize item exist checks when propagating from server QFileInfo::exists(filename) is the fastest method alos avoid creating too many QFileInfo instances when we need it for multiple purposes Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 3 +-- src/libsync/propagatorjobs.cpp | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 1f84796a17038..91e34537c5b47 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -381,8 +381,7 @@ bool FileSystem::fileExists(const QString &filename, const QFileInfo &fileInfo) // not valid. There needs to be one initialised here. Otherwise the incoming // fileInfo is re-used. if (fileInfo.filePath() != filename) { - QFileInfo myFI(filename); - re = myFI.exists(); + re = QFileInfo::exists(filename); } return re; } diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index a4c0146ac7420..23fe2451109d7 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -99,23 +99,26 @@ void PropagateLocalRemove::start() QString removeError; if (_moveToTrash && propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) { - if ((QDir(filename).exists() || FileSystem::fileExists(filename)) - && !FileSystem::moveToTrash(filename, &removeError)) { - qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << removeError; - done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); - return; + const auto fileInfo = QFileInfo{filename}; + if (FileSystem::fileExists(filename, fileInfo)) { + if (!FileSystem::moveToTrash(filename, &removeError)) { + qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << removeError; + done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); + return; + } + } else { + qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << "was already deleted"; } } else { if (_item->isDirectory()) { - if (QDir(filename).exists() && !removeRecursively(QString())) { + if (FileSystem::fileExists(filename) && !removeRecursively(QString())) { done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); return; } } else { - if (FileSystem::fileExists(filename)) { - const auto fileInfo = QFileInfo{filename}; + const auto fileInfo = QFileInfo{filename}; + if (FileSystem::fileExists(filename, fileInfo)) { const auto parentFolderPath = fileInfo.dir().absolutePath(); - const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite}; if (!FileSystem::remove(filename, &removeError)) { From db58703fc08672fd0ebd2d88db09c93cc71dc976 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 15 May 2025 12:30:25 +0200 Subject: [PATCH 3/5] fix(delete): fix move to trash when parent folder is read-only Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 23fe2451109d7..4886d0991b159 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -101,6 +101,9 @@ void PropagateLocalRemove::start() if (_moveToTrash && propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) { const auto fileInfo = QFileInfo{filename}; if (FileSystem::fileExists(filename, fileInfo)) { + const auto parentFolderPath = fileInfo.dir().absolutePath(); + const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite}; + if (!FileSystem::moveToTrash(filename, &removeError)) { qCWarning(lcPropagateLocalRemove()) << "move to trash failed" << filename << removeError; done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); From c62ce64b01ddecc6fed25560871e1ee77638fc45 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Tue, 20 May 2025 23:35:29 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(readonly):=20ignore=20move=20to=20trash?= =?UTF-8?q?=20for=20read-=C3=A8only=20items?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matthieu Gallien --- src/libsync/propagatorjobs.cpp | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 4886d0991b159..5869b30f92c50 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -98,8 +98,35 @@ void PropagateLocalRemove::start() } QString removeError; - if (_moveToTrash && propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) { - const auto fileInfo = QFileInfo{filename}; + auto moveToTrashIsFeasible = true; + if (propagator()->syncOptions()._vfs->mode() != OCC::Vfs::WindowsCfApi) { + moveToTrashIsFeasible = false; + } + const auto fileInfo = QFileInfo{filename}; + if (fileInfo.isDir()) { + try { + if (FileSystem::isFolderReadOnly(fileInfo.filesystemAbsolutePath())) { + moveToTrashIsFeasible = false; + } + } + catch (const std::filesystem::filesystem_error &e) + { + qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status" << e.what() << e.path1().c_str() << e.path2().c_str(); + } + catch (const std::system_error &e) + { + qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status" << e.what(); + } + catch (...) + { + qCWarning(lcPropagateLocalRemove) << "exception when checking parent folder read only status"; + } + } else { + if (!FileSystem::isWritable(filename, fileInfo)) { + moveToTrashIsFeasible = false; + } + } + if (_moveToTrash && moveToTrashIsFeasible) { if (FileSystem::fileExists(filename, fileInfo)) { const auto parentFolderPath = fileInfo.dir().absolutePath(); const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite}; @@ -114,12 +141,11 @@ void PropagateLocalRemove::start() } } else { if (_item->isDirectory()) { - if (FileSystem::fileExists(filename) && !removeRecursively(QString())) { + if (FileSystem::fileExists(filename, fileInfo) && !removeRecursively(QString())) { done(SyncFileItem::NormalError, tr("Temporary error when removing local item removed from server."), ErrorCategory::GenericError); return; } } else { - const auto fileInfo = QFileInfo{filename}; if (FileSystem::fileExists(filename, fileInfo)) { const auto parentFolderPath = fileInfo.dir().absolutePath(); const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite}; From 91c8a12d466672600a3ca7b98ee2e67209691c46 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 21 May 2025 10:37:09 +0200 Subject: [PATCH 5/5] fix(moveToTrash): enable use of move to trash in some automated tests should improve coverage for move to trash feature to ensure this is working as expected by users Signed-off-by: Matthieu Gallien --- test/testpermissions.cpp | 14 ++++++++++ test/testsyncengine.cpp | 57 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index e0cee82878887..88b4a5b3702e2 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -97,9 +97,23 @@ private slots: QStandardPaths::setTestModeEnabled(true); } + void t7pl_data() + { + QTest::addColumn("moveToTrashEnabled"); + QTest::newRow("move to trash") << true; + QTest::newRow("delete") << false; + } + void t7pl() { + QFETCH(bool, moveToTrashEnabled); + FakeFolder fakeFolder{ FileInfo() }; + + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._moveFilesToTrash = moveToTrashEnabled; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); // Some of this test depends on the order of discovery. With threading diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 77b17a3908e4d..b08f6a0e6abaf 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -219,8 +219,23 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + + void testLocalDelete_data() + { + QTest::addColumn("moveToTrashEnabled"); + QTest::newRow("move to trash") << true; + QTest::newRow("delete") << false; + } + void testLocalDelete() { + QFETCH(bool, moveToTrashEnabled); + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._moveFilesToTrash = moveToTrashEnabled; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + ItemCompletedSpy completeSpy(fakeFolder); fakeFolder.remoteModifier().remove("A/a1"); fakeFolder.syncOnce(); @@ -237,10 +252,23 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + void testLocalDeleteWithReuploadForNewLocalFiles_data() + { + QTest::addColumn("moveToTrashEnabled"); + QTest::newRow("move to trash") << true; + QTest::newRow("delete") << false; + } + void testLocalDeleteWithReuploadForNewLocalFiles() { + QFETCH(bool, moveToTrashEnabled); + FakeFolder fakeFolder{FileInfo{}}; + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._moveFilesToTrash = moveToTrashEnabled; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + // create folders hierarchy with some nested dirs and files fakeFolder.localModifier().mkdir("A"); fakeFolder.localModifier().insert("A/existingfile_A.txt", 100); @@ -1566,9 +1594,23 @@ private slots: QCOMPARE(fileThirdSync->lastModified.toSecsSinceEpoch(), CURRENT_MTIME); } + void testFolderRemovalWithCaseClash_data() + { + QTest::addColumn("moveToTrashEnabled"); + QTest::newRow("move to trash") << true; + QTest::newRow("delete") << false; + } + void testFolderRemovalWithCaseClash() { - FakeFolder fakeFolder{ FileInfo{} }; + QFETCH(bool, moveToTrashEnabled); + + FakeFolder fakeFolder{FileInfo{}}; + + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._moveFilesToTrash = moveToTrashEnabled; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + fakeFolder.remoteModifier().mkdir("A"); fakeFolder.remoteModifier().mkdir("toDelete"); fakeFolder.remoteModifier().insert("A/file"); @@ -1851,8 +1893,17 @@ private slots: } } + void testServer_caseClash_createConflict_thenRemoveOneRemoteFile_data() + { + QTest::addColumn("moveToTrashEnabled"); + QTest::newRow("move to trash") << true; + QTest::newRow("delete") << false; + } + void testServer_caseClash_createConflict_thenRemoveOneRemoteFile() { + QFETCH(bool, moveToTrashEnabled); + constexpr auto testLowerCaseFile = "test"; constexpr auto testUpperCaseFile = "TEST"; @@ -1864,6 +1915,10 @@ private slots: FakeFolder fakeFolder{FileInfo{}}; + auto syncOptions = fakeFolder.syncEngine().syncOptions(); + syncOptions._moveFilesToTrash = moveToTrashEnabled; + fakeFolder.syncEngine().setSyncOptions(syncOptions); + fakeFolder.remoteModifier().insert("otherFile.txt"); fakeFolder.remoteModifier().insert(testLowerCaseFile); fakeFolder.remoteModifier().insert(testUpperCaseFile);