PropagateLocalRemove: remove entries from the DB even if there was an error.

Previously, in case of an error while deleting a directory, we would not
remove the entries from the local db, despite most of the files would
be deleted.

Which means that if the files re-appear on the server with the same etag,
we would think the file were deleted from the client and propagate the change.

In 1.8.0 we had this bug that we would not see some directory in the server in
some cases. This would lead us to delete the file on the client.  Normaly the
files are deleted from the local database and next sync would re-download the files.
But in the cases where there was an error deleting one of the file (for example
if it was locked) we would then propagate the delete to the server.

Fix this by always deleting from the database the files that we deleted.

Issue #3206

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
index 4cb7872b1e3604a2c823de0fde82ed04cf702314..7c4df9cb8be2ab86914825257f7dbf67648edcc9 100644 (file)
--- a/src/libsync/propagatorjobs.cpp
+++ b/src/libsync/propagatorjobs.cpp
@@ -39,38 +39,65 @@
 
 namespace OCC {
 
-// Code copied from Qt5's QDir::removeRecursively
-// (and modified to report the error)
-static bool removeRecursively(const QString &path, QString &error)
+/**
+ * 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 of removing the entries
+ * in the database.  But in case of error, we need to remove the entries from the database of the files
+ * that were deleted.
+ *
+ * \a path is relative to _propagator->_localDir + _item->_file and should start with a slash
+ */
+bool PropagateLocalRemove::removeRecursively(const QString& path)
 {
     bool success = true;
-    QDirIterator di(path, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
+    QString absolute = _propagator->_localDir + _item->_file + path;
+    QDirIterator di(absolute, QDir::AllEntries | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot);
+
+    QVector<QPair<QString, bool>> deleted;
+
     while (di.hasNext()) {
         di.next();
         const QFileInfo& fi = di.fileInfo();
         bool ok;
         // The use of isSymLink here is okay:
         // we never want to go into this branch for .lnk files
-        if (fi.isDir() && !fi.isSymLink()) {
-            ok = removeRecursively(di.filePath(), error); // recursive
+        bool isDir = fi.isDir() && !fi.isSymLink();
+        if (isDir) {
+            ok = removeRecursively(path + QLatin1Char('/') + di.fileName()); // recursive
         } else {
             QFile f(di.filePath());
             ok = f.remove();
             if (!ok) {
-                error += PropagateLocalRemove::tr("Error removing '%1': %2;").
+                _error += PropagateLocalRemove::tr("Error removing '%1': %2;").
                     arg(QDir::toNativeSeparators(f.fileName()), f.errorString()) + " ";
                 qDebug() << "Error removing " << f.fileName() << ':' << f.errorString();
             }
         }
-        if (!ok)
+        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(path);
+        success = QDir().rmdir(absolute);
         if (!success) {
-            error += PropagateLocalRemove::tr("Could not remove directory '%1';")
-                .arg(QDir::toNativeSeparators(path)) + " ";
-            qDebug() << "Error removing directory" << path;
+            _error += PropagateLocalRemove::tr("Could not remove directory '%1';")
+                .arg(QDir::toNativeSeparators(absolute)) + " ";
+            qDebug() << "Error removing directory" << absolute;
         }
     }
     return success;
@@ -89,9 +116,8 @@ void PropagateLocalRemove::start()
     }
 
     if (_item->_isDirectory) {
-        QString error;
-        if (QDir(filename).exists() && !removeRecursively(filename, error)) {
-            done(SyncFileItem::NormalError, error);
+        if (QDir(filename).exists() && !removeRecursively(QString())) {
+            done(SyncFileItem::NormalError, _error);
             return;
         }
     } else {

diff --git a/src/libsync/propagatorjobs.h b/src/libsync/propagatorjobs.h
index 537bba940604946b32a761591337c18de950c7c6..5dd21e68251009e196603bb90c9176fa817d0217 100644 (file)
--- a/src/libsync/propagatorjobs.h
+++ b/src/libsync/propagatorjobs.h
@@ -43,6 +43,9 @@ class PropagateLocalRemove : public PropagateItemJob {
 public:
     PropagateLocalRemove (OwncloudPropagator* propagator,const SyncFileItemPtr& item)  : PropagateItemJob(propagator, item) {}
     void start() Q_DECL_OVERRIDE;
+private:
+    bool removeRecursively(const QString &path);
+    QString _error;
 };
 
 /**