FileSystem: make removeRecursively() reusable

We want to use it for deleting directory conflicts.

src/libsync/filesystem.cpp

@@ -17,6 +17,9 @@
 #include "common/utility.h"
 #include <QFile>
 #include <QFileInfo>
+#include <QDir>
+#include <QDirIterator>
+#include <QCoreApplication>
 
 // We use some internals of csync:
 extern "C" int c_utimes(const char *, const struct timeval *);
@@ -138,5 +141,53 @@ qint64 FileSystem::getSize(const QString &filename)
     return QFileInfo(filename).size();
 }
 
+// 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 allRemoved = true;
+    QDirIterator di(path, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
+
+    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());
+        if (isDir) {
+            removeOk = removeRecursively(path + QLatin1Char('/') + di.fileName(), onDeleted, errors); // recursive
+        } else {
+            QString removeError;
+            removeOk = FileSystem::remove(di.filePath(), &removeError);
+            if (removeOk) {
+                if (onDeleted)
+                    onDeleted(di.filePath(), false);
+            } else {
+                if (errors) {
+                    errors->append(QCoreApplication::translate("FileSystem", "Error removing '%1': %2")
+                                       .arg(QDir::toNativeSeparators(di.filePath()), removeError));
+                }
+                qCWarning(lcFileSystem) << "Error removing " << di.filePath() << ':' << removeError;
+            }
+        }
+        if (!removeOk)
+            allRemoved = false;
+    }
+    if (allRemoved) {
+        allRemoved = QDir().rmdir(path);
+        if (allRemoved) {
+            if (onDeleted)
+                onDeleted(path, true);
+        } else {
+            if (errors) {
+                errors->append(QCoreApplication::translate("FileSystem", "Could not remove folder '%1'")
+                                   .arg(QDir::toNativeSeparators(path)));
+            }
+            qCWarning(lcFileSystem) << "Error removing folder" << path;
+        }
+    }
+    return allRemoved;
+}
+
 
 } // namespace OCC

src/libsync/filesystem.h

@@ -27,6 +27,8 @@ class QFile;
 
 namespace OCC {
 
+class SyncJournal;
+
 /**
  *  \addtogroup libsync
  *  @{
@@ -77,6 +79,17 @@ namespace FileSystem {
     bool verifyFileUnchanged(const QString &fileName,
         qint64 previousSize,
         time_t previousMtime);
+
+    /**
+     * 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);
 }
 
 /** @} */

src/libsync/propagatorjobs.cpp

@@ -47,7 +47,6 @@ QByteArray localFileIdFromFullId(const QByteArray &id)
 }
 
 /**
- * Code inspired from Qt5's QDir::removeRecursively
  * The code will update the database in case of error.
  * If everything goes well (no error, returns true), the caller is responsible for removing the entries
  * in the database.  But in case of error, we need to remove the entries from the database of the files
@@ -57,55 +56,34 @@ QByteArray localFileIdFromFullId(const QByteArray &id)
  */
 bool PropagateLocalRemove::removeRecursively(const QString &path)
 {
-    bool success = true;
-    QString absolute = propagator()->_localDir + _item->_file + path;
-    QDirIterator di(absolute, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
+    auto folderDir = propagator()->_localDir;
+    QString absolute = folderDir + _item->_file + path;
+    QStringList errors;
+    QList<QPair<QString, bool>> deleted;
+    bool success = FileSystem::removeRecursively(
+        absolute,
+        [this, &deleted](const QString &path, bool isDir) {
+            // by prepending, a folder deletion may be followed by content deletions
+            deleted.prepend(qMakePair(path, isDir));
+        },
+        &errors);
 
-    QVector<QPair<QString, bool>> deleted;
-
-    while (di.hasNext()) {
-        di.next();
-        const QFileInfo &fi = di.fileInfo();
-        bool ok = 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());
-        if (isDir) {
-            ok = removeRecursively(path + QLatin1Char('/') + di.fileName()); // recursive
-        } else {
-            QString removeError;
-            ok = FileSystem::remove(di.filePath(), &removeError);
-            if (!ok) {
-                _error += PropagateLocalRemove::tr("Error removing '%1': %2;").arg(QDir::toNativeSeparators(di.filePath()), removeError) + " ";
-                qCWarning(lcPropagateLocalRemove) << "Error removing " << di.filePath() << ':' << removeError;
-            }
-        }
-        if (success && !ok) {
-            // We need to delete the entries from the database now from the deleted vector
-            foreach (const auto &it, deleted) {
-                propagator()->_journal->deleteFileRecord(_item->_originalFile + path + QLatin1Char('/') + it.first,
-                    it.second);
-            }
-            success = false;
-            deleted.clear();
-        }
-        if (success) {
-            deleted.append(qMakePair(di.fileName(), isDir));
-        }
-        if (!success && ok) {
-            // This succeeded, so we need to delete it from the database now because the caller won't
-            propagator()->_journal->deleteFileRecord(_item->_originalFile + path + QLatin1Char('/') + di.fileName(),
-                isDir);
-        }
-    }
-    if (success) {
-        success = QDir().rmdir(absolute);
     if (!success) {
-            _error += PropagateLocalRemove::tr("Could not remove folder '%1'")
-                          .arg(QDir::toNativeSeparators(absolute))
-                + " ";
-            qCWarning(lcPropagateLocalRemove) << "Error removing folder" << absolute;
+        // 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;
+        foreach (const auto &it, deleted) {
+            if (!it.first.startsWith(folderDir))
+                continue;
+            if (!deletedDir.isEmpty() && it.first.startsWith(deletedDir))
+                continue;
+            if (it.second) {
+                deletedDir = it.first;
             }
+            propagator()->_journal->deleteFileRecord(it.first.mid(folderDir.size()), it.second);
+        }
+
+        _error = errors.join(", ");
     }
     return success;
 }