Skip to content

Commit ad1086d

Browse files
mgallienmgallien
committed
fix: do not interrupt sync when cleaning invalid read-only items
Signed-off-by: Matthieu Gallien <[email protected]>
1 parent 9df1ce1 commit ad1086d

File tree

8 files changed

+43
-140
lines changed

8 files changed

+43
-140
lines changed

‎src/gui/folder.cpp‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,6 @@ Folder::Folder(const FolderDefinition &definition,
107107

108108
connect(_engine.data(), &SyncEngine::aboutToRemoveAllFiles,
109109
this, &Folder::slotAboutToRemoveAllFiles);
110-
connect(_engine.data(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
111-
this, &Folder::slotNeedToRemoveRemnantsReadOnlyFolders);
112110
connect(_engine.data(), &SyncEngine::transmissionProgress, this, &Folder::slotTransmissionProgress);
113111
connect(_engine.data(), &SyncEngine::itemCompleted,
114112
this, &Folder::slotItemCompleted);
@@ -1723,34 +1721,6 @@ void Folder::slotAboutToRemoveAllFiles(SyncFileItem::Direction dir, std::functio
17231721
msgBox->open();
17241722
}
17251723

1726-
void Folder::slotNeedToRemoveRemnantsReadOnlyFolders(const QList<SyncFileItemPtr> &folders,
1727-
const QString &localPath,
1728-
std::function<void (bool)> callback)
1729-
{
1730-
auto listOfFolders = QStringList{};
1731-
for (const auto &oneFolder : folders) {
1732-
listOfFolders.push_back(oneFolder->_file);
1733-
}
1734-
1735-
qCInfo(lcFolder()) << "will delete invalid read-only folders:" << listOfFolders.join(", ");
1736-
1737-
setSyncPaused(true);
1738-
for(const auto &oneFolder : folders) {
1739-
const auto fileInfo = QFileInfo{localPath + oneFolder->_file};
1740-
const auto parentFolderPath = fileInfo.dir().absolutePath();
1741-
const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
1742-
if (oneFolder->_type == ItemType::ItemTypeDirectory) {
1743-
FileSystem::removeRecursively(localPath + oneFolder->_file);
1744-
} else {
1745-
FileSystem::remove(localPath + oneFolder->_file);
1746-
}
1747-
}
1748-
callback(true);
1749-
setSyncPaused(false);
1750-
_lastEtag.clear();
1751-
slotScheduleThisFolder();
1752-
}
1753-
17541724
void Folder::removeLocalE2eFiles()
17551725
{
17561726
qCDebug(lcFolder) << "Removing local E2EE files";

‎src/gui/folder.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -336,10 +336,6 @@ public slots:
336336
// connected to the corresponding signals in the SyncEngine
337337
void slotAboutToRemoveAllFiles(OCC::SyncFileItem::Direction, std::function<void(bool)> callback);
338338

339-
void slotNeedToRemoveRemnantsReadOnlyFolders(const QList<SyncFileItemPtr> &folders,
340-
const QString &localPath,
341-
std::function<void(bool)> callback);
342-
343339
/**
344340
* Starts a sync operation
345341
*

‎src/libsync/discovery.cpp‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1867,16 +1867,14 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item)
18671867
// No permissions set
18681868
return true;
18691869
} else if (item->isDirectory() && !perms.hasPermission(RemotePermissions::CanAddSubDirectories)) {
1870-
qCWarning(lcDisco) << "checkForPermission: ERROR" << item->_file;
1871-
item->_instruction = CSYNC_INSTRUCTION_ERROR;
1870+
qCWarning(lcDisco) << "checkForPermission: Not allowed because you don't have permission to add subfolders to that folder:" << item->_file;
1871+
item->_instruction = CSYNC_INSTRUCTION_IGNORE;
18721872
item->_errorString = tr("Not allowed because you don't have permission to add subfolders to that folder");
1873-
const auto localPath = QString{_discoveryData->_localDir + item->_file};
1874-
qCWarning(lcDisco) << "unexpected new folder in a read-only folder will be made read-write" << localPath;
18751873
emit _discoveryData->remnantReadOnlyFolderDiscovered(item);
18761874
return false;
18771875
} else if (!item->isDirectory() && !perms.hasPermission(RemotePermissions::CanAddFile)) {
1878-
qCWarning(lcDisco) << "checkForPermission: ERROR" << item->_file;
1879-
item->_instruction = CSYNC_INSTRUCTION_ERROR;
1876+
qCWarning(lcDisco) << "checkForPermission: Not allowed because you don't have permission to add files in that folder:" << item->_file;
1877+
item->_instruction = CSYNC_INSTRUCTION_IGNORE;
18801878
item->_errorString = tr("Not allowed because you don't have permission to add files in that folder");
18811879
emit _discoveryData->remnantReadOnlyFolderDiscovered(item);
18821880
return false;

‎src/libsync/filesystem.cpp‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,13 @@ qint64 FileSystem::getSize(const QString &filename)
254254
}
255255

256256
// Code inspired from Qt5's QDir::removeRecursively
257-
bool FileSystem::removeRecursively(const QString &path, const std::function<void(const QString &path, bool isDir)> &onDeleted, QStringList *errors)
257+
bool FileSystem::removeRecursively(const QString &path, const std::function<void(const QString &path, bool isDir)> &onDeleted, QStringList *errors, const std::function<void(const QString &path, bool isDir)> &onError)
258258
{
259-
FileSystem::setFolderPermissions(path, FileSystem::FolderPermissions::ReadWrite);
259+
if (!FileSystem::setFolderPermissions(path, FileSystem::FolderPermissions::ReadWrite)) {
260+
if (onError) {
261+
onError(path, true);
262+
}
263+
}
260264

261265
bool allRemoved = true;
262266
QDirIterator di(path, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
@@ -269,7 +273,7 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
269273
// we never want to go into this branch for .lnk files
270274
bool isDir = FileSystem::isDir(fi.absoluteFilePath()) && !FileSystem::isSymLink(fi.absoluteFilePath()) && !FileSystem::isJunction(fi.absoluteFilePath());
271275
if (isDir) {
272-
removeOk = removeRecursively(path + QLatin1Char('/') + di.fileName(), onDeleted, errors); // recursive
276+
removeOk = removeRecursively(path + QLatin1Char('/') + di.fileName(), onDeleted, errors, onError); // recursive
273277
} else {
274278
QString removeError;
275279

@@ -285,6 +289,9 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
285289
errors->append(QCoreApplication::translate("FileSystem", "Error removing \"%1\": %2")
286290
.arg(QDir::toNativeSeparators(di.filePath()), removeError));
287291
}
292+
if (onError) {
293+
onError(di.filePath(), false);
294+
}
288295
qCWarning(lcFileSystem) << "Error removing " << di.filePath() << ':' << removeError;
289296
}
290297
}
@@ -305,6 +312,9 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
305312
errors->append(QCoreApplication::translate("FileSystem", "Could not remove folder \"%1\"")
306313
.arg(QDir::toNativeSeparators(path)));
307314
}
315+
if (onError) {
316+
onError(di.filePath(), false);
317+
}
308318
qCWarning(lcFileSystem) << "Error removing folder" << path;
309319
}
310320
}

‎src/libsync/filesystem.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,9 @@ namespace FileSystem {
122122
* errors are collected in errors.
123123
*/
124124
bool OWNCLOUDSYNC_EXPORT removeRecursively(const QString &path,
125-
const std::function<void(const QString &path, bool isDir)> &onDeleted = nullptr,
126-
QStringList *errors = nullptr);
125+
const std::function<void(const QString &path, bool isDir)> &onDeleted = nullptr,
126+
QStringList *errors = nullptr,
127+
const std::function<void(const QString &path, bool isDir)> &onError = nullptr);
127128

128129
bool OWNCLOUDSYNC_EXPORT setFolderPermissions(const QString &path,
129130
FileSystem::FolderPermissions permissions) noexcept;

‎src/libsync/syncengine.cpp‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -840,7 +840,6 @@ void SyncEngine::slotDiscoveryFinished()
840840

841841
if (!_remnantReadOnlyFolders.isEmpty()) {
842842
handleRemnantReadOnlyFolders();
843-
return;
844843
}
845844

846845
finishSync();
@@ -1169,9 +1168,29 @@ bool SyncEngine::handleMassDeletion()
11691168

11701169
void SyncEngine::handleRemnantReadOnlyFolders()
11711170
{
1172-
promptUserBeforePropagation([this](auto &&callback) {
1173-
emit aboutToRemoveRemnantsReadOnlyFolders(_remnantReadOnlyFolders, _localPath, callback);
1174-
});
1171+
auto listOfFolders = QStringList{};
1172+
for (const auto &oneFolder : std::as_const(_remnantReadOnlyFolders)) {
1173+
listOfFolders.push_back(oneFolder->_file);
1174+
}
1175+
1176+
qCInfo(lcEngine()) << "will delete invalid read-only folders:" << listOfFolders.join(", ");
1177+
1178+
for(const auto &oneFolder : std::as_const(_remnantReadOnlyFolders)) {
1179+
const auto fileInfo = QFileInfo{_localPath + oneFolder->_file};
1180+
const auto parentFolderPath = fileInfo.dir().absolutePath();
1181+
slotAddTouchedFile(parentFolderPath);
1182+
const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
1183+
slotAddTouchedFile(_localPath + oneFolder->_file);
1184+
1185+
if (oneFolder->_type == ItemType::ItemTypeDirectory) {
1186+
const auto deletionCallback = [this] (const QString &deleteItem, bool) {
1187+
slotAddTouchedFile(deleteItem);
1188+
};
1189+
FileSystem::removeRecursively(_localPath + oneFolder->_file, deletionCallback, nullptr, deletionCallback);
1190+
} else {
1191+
FileSystem::remove(_localPath + oneFolder->_file);
1192+
}
1193+
}
11751194
}
11761195

11771196
template <typename T>

‎src/libsync/syncengine.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,6 @@ public slots:
190190
*/
191191
void aboutToRemoveAllFiles(OCC::SyncFileItem::Direction direction, std::function<void(bool)> f);
192192

193-
void aboutToRemoveRemnantsReadOnlyFolders(const QList<SyncFileItemPtr> &folders,
194-
const QString &localPath,
195-
std::function<void(bool)> f);
196-
197193
// A new folder was discovered and was not synced because of the confirmation feature
198194
void newBigFolder(const QString &folder, bool isExternal);
199195

‎test/testpermissions.cpp‎

Lines changed: 0 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,6 @@ private slots:
9494
FakeFolder fakeFolder{ FileInfo() };
9595
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
9696

97-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
98-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
99-
qDebug() << "aboutToRemoveRemnantsReadOnlyFolders called";
100-
Q_UNUSED(folders);
101-
Q_UNUSED(localPath);
102-
callback(false);
103-
});
104-
10597
// Some of this test depends on the order of discovery. With threading
10698
// that order becomes effectively random, but we want to make sure to test
10799
// all cases and thus disable threading.
@@ -438,13 +430,6 @@ private slots:
438430
{
439431
FakeFolder fakeFolder{FileInfo{}};
440432

441-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
442-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
443-
Q_UNUSED(folders)
444-
Q_UNUSED(localPath)
445-
callback(false);
446-
});
447-
448433
// Some of this test depends on the order of discovery. With threading
449434
// that order becomes effectively random, but we want to make sure to test
450435
// all cases and thus disable threading.
@@ -564,14 +549,6 @@ private slots:
564549
{
565550
FakeFolder fakeFolder{FileInfo{}};
566551

567-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
568-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
569-
for(const auto &oneFolder : folders) {
570-
FileSystem::removeRecursively(localPath + oneFolder->_file);
571-
}
572-
callback(false);
573-
});
574-
575552
auto &lm = fakeFolder.localModifier();
576553
auto &rm = fakeFolder.remoteModifier();
577554
rm.mkdir("forbidden-move");
@@ -623,14 +600,6 @@ private slots:
623600
{
624601
FakeFolder fakeFolder{FileInfo{}};
625602

626-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
627-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
628-
for(const auto &oneFolder : folders) {
629-
FileSystem::removeRecursively(localPath + oneFolder->_file);
630-
}
631-
callback(false);
632-
});
633-
634603
auto &remote = fakeFolder.remoteModifier();
635604

636605
remote.mkdir("readOnlyFolder");
@@ -648,14 +617,6 @@ private slots:
648617
{
649618
FakeFolder fakeFolder{FileInfo{}};
650619

651-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
652-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
653-
for(const auto &oneFolder : folders) {
654-
FileSystem::removeRecursively(localPath + oneFolder->_file);
655-
}
656-
callback(false);
657-
});
658-
659620
auto &remote = fakeFolder.remoteModifier();
660621

661622
remote.mkdir("readWriteFolder");
@@ -674,14 +635,6 @@ private slots:
674635
{
675636
FakeFolder fakeFolder{FileInfo{}};
676637

677-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
678-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
679-
for(const auto &oneFolder : folders) {
680-
FileSystem::removeRecursively(localPath + oneFolder->_file);
681-
}
682-
callback(false);
683-
});
684-
685638
auto &remote = fakeFolder.remoteModifier();
686639

687640
remote.mkdir("testFolder");
@@ -718,14 +671,6 @@ private slots:
718671
{
719672
FakeFolder fakeFolder{FileInfo{}};
720673

721-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
722-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
723-
for(const auto &oneFolder : folders) {
724-
FileSystem::removeRecursively(localPath + oneFolder->_file);
725-
}
726-
callback(false);
727-
});
728-
729674
auto &remote = fakeFolder.remoteModifier();
730675

731676
remote.mkdir("testFolder");
@@ -786,14 +731,6 @@ private slots:
786731
{
787732
FakeFolder fakeFolder{FileInfo{}};
788733

789-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
790-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
791-
for(const auto &oneFolder : folders) {
792-
FileSystem::removeRecursively(localPath + oneFolder->_file);
793-
}
794-
callback(false);
795-
});
796-
797734
auto &remote = fakeFolder.remoteModifier();
798735

799736
remote.mkdir("readOnlyFolder");
@@ -824,14 +761,6 @@ private slots:
824761
{
825762
FakeFolder fakeFolder{FileInfo{}};
826763

827-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
828-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
829-
for(const auto &oneFolder : folders) {
830-
FileSystem::removeRecursively(localPath + oneFolder->_file);
831-
}
832-
callback(false);
833-
});
834-
835764
auto &remote = fakeFolder.remoteModifier();
836765

837766
remote.mkdir("readOnlyFolder");
@@ -864,14 +793,6 @@ private slots:
864793
{
865794
FakeFolder fakeFolder{FileInfo{}};
866795

867-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
868-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
869-
for(const auto &oneFolder : folders) {
870-
FileSystem::removeRecursively(localPath + oneFolder->_file);
871-
}
872-
callback(false);
873-
});
874-
875796
auto &remote = fakeFolder.remoteModifier();
876797

877798
remote.mkdir("readOnlyFolder");
@@ -907,14 +828,6 @@ private slots:
907828
{
908829
FakeFolder fakeFolder{FileInfo{}};
909830

910-
QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToRemoveRemnantsReadOnlyFolders,
911-
[&](const QList<SyncFileItemPtr> &folders, const QString &localPath, std::function<void(bool)> callback) {
912-
for(const auto &oneFolder : folders) {
913-
FileSystem::removeRecursively(localPath + oneFolder->_file);
914-
}
915-
callback(false);
916-
});
917-
918831
auto &remote = fakeFolder.remoteModifier();
919832

920833
remote.mkdir("readOnlyFolder");

0 commit comments

Comments
 (0)