Handle locks in recursive delete

4 files changed, 77 insertions, 49 deletions

diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
index e0cdea7df..faca9cb60 100644
--- a/src/libsync/filesystem.cpp
+++ b/src/libsync/filesystem.cpp
@@ -138,47 +138,44 @@ qint64 FileSystem::getSize(const QString &filename)
 }
 
 // Code inspired from Qt5's QDir::removeRecursively
-bool FileSystem::removeRecursively(const QString &path, const std::function<void(const QString &path, bool isDir)> &onDeleted, QStringList *errors)
+bool FileSystem::removeRecursively(const QString &path,
+    RemoveEntryList *success,
+    RemoveEntryList *locked,
+    RemoveErrorList *errors)
 {
     bool allRemoved = true;
     QDirIterator di(path, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
 
+    QString removeError;
     while (di.hasNext()) {
         di.next();
         const QFileInfo &fi = di.fileInfo();
-        bool removeOk = false;
         // The use of isSymLink here is okay:
         // we never want to go into this branch for .lnk files
-        bool isDir = fi.isDir() && !fi.isSymLink() && !FileSystem::isJunction(fi.absoluteFilePath());
+        const bool isDir = fi.isDir() && !fi.isSymLink() && !FileSystem::isJunction(fi.absoluteFilePath());
         if (isDir) {
-            removeOk = removeRecursively(path + QLatin1Char('/') + di.fileName(), onDeleted, errors); // recursive
+            allRemoved &= removeRecursively(path + QLatin1Char('/') + di.fileName(), success, locked, errors); // recursive
         } else {
-            QString removeError;
-            removeOk = FileSystem::remove(di.filePath(), &removeError);
-            if (removeOk) {
-                if (onDeleted)
-                    onDeleted(di.filePath(), false);
+            if (FileSystem::isFileLocked(di.filePath(), FileSystem::LockMode::Exclusive)) {
+                locked->push_back({ di.filePath(), isDir });
+                allRemoved = false;
             } else {
-                if (errors) {
-                    errors->append(QCoreApplication::translate("FileSystem", "Error removing '%1': %2")
-                                       .arg(QDir::toNativeSeparators(di.filePath()), removeError));
+                if (FileSystem::remove(di.filePath(), &removeError)) {
+                    success->push_back({ di.filePath(), isDir });
+                } else {
+                    errors->push_back({ { di.filePath(), isDir }, removeError });
+                    qCWarning(lcFileSystem) << "Error removing " << di.filePath() << ':' << removeError;
+                    allRemoved = false;
                 }
-                qCWarning(lcFileSystem) << "Error removing " << di.filePath() << ':' << removeError;
             }
         }
-        if (!removeOk)
-            allRemoved = false;
     }
     if (allRemoved) {
         allRemoved = QDir().rmdir(path);
         if (allRemoved) {
-            if (onDeleted)
-                onDeleted(path, true);
+            success->push_back({ path, true });
         } else {
-            if (errors) {
-                errors->append(QCoreApplication::translate("FileSystem", "Could not remove folder '%1'")
-                                   .arg(QDir::toNativeSeparators(path)));
-            }
+            errors->push_back({ { path, true }, QCoreApplication::translate("FileSystem", "Could not remove folder") });
             qCWarning(lcFileSystem) << "Error removing folder" << path;
         }
     }
diff --git a/src/libsync/filesystem.h b/src/libsync/filesystem.h
index 602f74bb7..f3e00f378 100644
--- a/src/libsync/filesystem.h
+++ b/src/libsync/filesystem.h
@@ -86,16 +86,30 @@ namespace FileSystem {
         qint64 previousSize,
         time_t previousMtime);
 
+
+    struct RemoveEntry
+    {
+        const QString path;
+        const bool isDir;
+    };
+    struct RemoveError
+    {
+        const RemoveEntry entry;
+        const QString error;
+    };
+
+    using RemoveEntryList = std::vector<RemoveEntry>;
+    using RemoveErrorList = std::vector<RemoveError>;
+
     /**
      * Removes a directory and its contents recursively
      *
      * Returns true if all removes succeeded.
-     * onDeleted() is called for each deleted file or directory, including the root.
-     * errors are collected in errors.
      */
     bool OWNCLOUDSYNC_EXPORT removeRecursively(const QString &path,
-        const std::function<void(const QString &path, bool isDir)> &onDeleted = nullptr,
-        QStringList *errors = nullptr);
+        RemoveEntryList *success,
+        RemoveEntryList *locked,
+        RemoveErrorList *errors);
 }
 
 /** @} */
diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
index 3c4316270..dddf0a817 100644
--- a/src/libsync/propagatorjobs.cpp
+++ b/src/libsync/propagatorjobs.cpp
@@ -47,35 +47,50 @@ Q_LOGGING_CATEGORY(lcPropagateLocalRename, "sync.propagator.localrename", QtInfo
  *
  * \a path is relative to propagator()->_localDir + _item->_file and should start with a slash
  */
-bool PropagateLocalRemove::removeRecursively(const QString &absolute, QString *error)
+bool PropagateLocalRemove::removeRecursively(const QString &absolute)
 {
-    QStringList errors;
-    // path, isDir
-    QList<QPair<QString, bool>> deleted;
-    bool success = FileSystem::removeRecursively(
+    FileSystem::RemoveEntryList removed;
+    FileSystem::RemoveEntryList locked;
+    FileSystem::RemoveErrorList errors;
+    const bool success = FileSystem::removeRecursively(
         absolute,
-        [&deleted](const QString &path, bool isDir) {
-            // by prepending, a folder deletion may be followed by content deletions
-            deleted.prepend(qMakePair(path, isDir));
-        },
+        &removed,
+        &locked,
         &errors);
 
     if (!success) {
         // We need to delete the entries from the database now from the deleted vector.
         // Do it while avoiding redundant delete calls to the journal.
         QString deletedDir;
-        for (const auto &it : qAsConst(deleted)) {
-            if (!it.first.startsWith(propagator()->localPath()))
+        for (const auto &it : qAsConst(removed)) {
+            if (!it.path.startsWith(propagator()->localPath()))
                 continue;
-            if (!deletedDir.isEmpty() && it.first.startsWith(deletedDir))
+            if (!deletedDir.isEmpty() && it.path.startsWith(deletedDir))
                 continue;
-            if (it.second) {
-                deletedDir = it.first;
+            if (it.isDir) {
+                deletedDir = it.path;
             }
-            propagator()->_journal->deleteFileRecord(it.first.mid(propagator()->localPath().size()), it.second);
+            propagator()->_journal->deleteFileRecord(it.path.mid(propagator()->localPath().size()), it.isDir);
+        }
+        if (!errors.empty()) {
+            QStringList errorList;
+            errorList.reserve(errors.size());
+            for (const auto &err : errors) {
+                errorList.append(tr("%1 failed with: %2").arg(QDir::toNativeSeparators(err.entry.path), err.error));
+            }
+            done(SyncFileItem::NormalError, errorList.join(QStringLiteral(", ")));
+            return false;
+        } else if (!locked.empty()) {
+            QStringList errorList;
+            errorList.reserve(errors.size());
+            for (const auto &l : locked) {
+                // unlock is handled in hack in `void Folder::slotWatchedPathChanged`
+                emit propagator()->seenLockedFile(l.path, FileSystem::LockMode::Exclusive);
+                errorList.append(tr("%1 the file is currently in use").arg(QDir::toNativeSeparators(l.path)));
+            }
+            done(SyncFileItem::SoftError, errorList.join(QStringLiteral(", ")));
+            return false;
         }
-
-        *error = errors.join(QStringLiteral(", "));
     }
     return success;
 }
@@ -96,20 +111,22 @@ void PropagateLocalRemove::start()
     }
 
     if (FileSystem::fileExists(filename)) {
-        bool ok;
-        QString removeError;
-        const auto lockMode = propagator()->syncOptions().requiredLockMode();
-        if (FileSystem::isFileLocked(filename, lockMode)) {
-            emit propagator()->seenLockedFile(filename, lockMode);
+        if (FileSystem::isFileLocked(filename, FileSystem::LockMode::Exclusive)) {
+            emit propagator()->seenLockedFile(filename, FileSystem::LockMode::Exclusive);
             done(SyncFileItem::SoftError, tr("%1 the file is currently in use").arg(QDir::toNativeSeparators(filename)));
             return;
         }
 
+        bool ok = false;
+        QString removeError;
         if (_moveToTrash) {
             ok = FileSystem::moveToTrash(filename, &removeError);
         } else {
             if (_item->isDirectory()) {
-                ok = removeRecursively(filename, &removeError);
+                // removeRecursively will call done on error
+                if (!(ok = removeRecursively(filename))) {
+                    return;
+                }
             } else {
                 ok = FileSystem::remove(filename, &removeError);
             }
diff --git a/src/libsync/propagatorjobs.h b/src/libsync/propagatorjobs.h
index 9ab839abf..1e9b25860 100644
--- a/src/libsync/propagatorjobs.h
+++ b/src/libsync/propagatorjobs.h
@@ -42,7 +42,7 @@ public:
     void start() override;
 
 private:
-    bool removeRecursively(const QString &absolute, QString *error);
+    bool removeRecursively(const QString &absolute);
     bool _moveToTrash;
 };