New discovery algoritmh: more on Renames

diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h
index a05711916fc0f2b015fab95248ebca8420af1548..8a98af6654ac1a9fa71043c4d43fb84bb500d002 100644 (file)
--- a/src/libsync/abstractnetworkjob.h
+++ b/src/libsync/abstractnetworkjob.h
@@ -24,6 +24,7 @@
 #include <QDateTime>
 #include <QTimer>
 #include "accountfwd.h"
+#include "common/asserts.h"
 
 class QUrl;
 
@@ -31,6 +32,63 @@ namespace OCC {
 
 class AbstractSslErrorHandler;
 
+
+/**
+ * A Result of type T, or an Error that contains a code and a string
+ **/
+template <typename T>
+class Result
+{
+    struct Error
+    {
+        QString string;
+        int code;
+    };
+    union {
+        T _result;
+        Error _error;
+    };
+    bool _isError;
+
+public:
+    Result(T value)
+        : _result(std::move(value))
+        , _isError(false){};
+    Result(int code, QString str)
+        : _error({ std::move(str), code })
+        , _isError(true)
+    {
+    }
+    ~Result()
+    {
+        if (_isError)
+            _error.~Error();
+        else
+            _result.~T();
+    }
+    explicit operator bool() const { return !_isError; }
+    const T &operator*() const &
+    {
+        ASSERT(!_isError);
+        return _result;
+    }
+    T operator*() &&
+    {
+        ASSERT(!_isError);
+        return std::move(_result);
+    }
+    QString errorMessage() const
+    {
+        ASSERT(_isError);
+        return _error.string;
+    }
+    int errorCode() const
+    {
+        return _isError ? _error.code : 0;
+    }
+};
+
+
 /**
  * @brief The AbstractNetworkJob class
  * @ingroup libsync

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index 86741882ae49dc37d0684c8f60e14e4306cde1f9..b87e2e47c0e729dd225d43accd6afee984dcfc63 100644 (file)
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -55,8 +55,8 @@ DiscoverServerJob::DiscoverServerJob(const AccountPtr &account, const QString &p
     });
 
     connect(this, &DiscoverySingleDirectoryJob::finishedWithError, this,
-        [this](int, const QString &msg) {
-            emit this->finished({ Error, msg });
+        [this](int code, const QString &msg) {
+            emit this->finished({ code, msg });
         });
 }
 
@@ -352,7 +352,7 @@ void ProcessDirectoryJob::processFile(PathTuple path,
         item->_etag = serverEntry.etag;
         item->_previousSize = localEntry.size;
         item->_previousModtime = localEntry.modtime;
-        if (!dbEntry.isValid()) { // New file?
+        if (!dbEntry.isValid()) { // New file on the server
             item->_instruction = CSYNC_INSTRUCTION_NEW;
             item->_direction = SyncFileItem::Down;
             item->_modtime = serverEntry.modtime;
@@ -422,13 +422,13 @@ void ProcessDirectoryJob::processFile(PathTuple path,
                     /* A remote rename can also mean Encryption Mangled Name.
                      * if we find one of those in the database, we ignore it.
                      */
-                    else if (!base._e2eMangledName.isEmpty()) {
+                    if (!base._e2eMangledName.isEmpty()) {
                         qCWarning(lcDisco, "Encrypted file can not rename");
                         done = true;
                         return;
                     }
 
-                    else if (item->_type == ItemTypeFile) {
+                    if (item->_type == ItemTypeFile) {
                         csync_file_stat_t buf;
                         if (csync_vio_local_stat((_discoveryData->_localDir + originalPath).toUtf8(), &buf)) {
                             qCInfo(lcDisco) << "Local file does not exist anymore." << originalPath;
@@ -438,6 +438,11 @@ void ProcessDirectoryJob::processFile(PathTuple path,
                             qCInfo(lcDisco) << "File has changed locally, not a rename." << originalPath;
                             return;
                         }
+                    } else {
+                        if (!QFile::exists(_discoveryData->_localDir + originalPath)) {
+                            qCInfo(lcDisco) << "Local directory does not exist anymore." << originalPath;
+                            return;
+                        }
                     }
 
                     bool wasDeletedOnServer = false;
@@ -473,51 +478,33 @@ void ProcessDirectoryJob::processFile(PathTuple path,
                     } else {
                         // we need to make a request to the server to know that the original file is deleted on the server
                         _pendingAsyncJobs++;
-                        auto job = new PropfindJob(_discoveryData->_account, originalPath, this);
-                        auto considerNew = [=] {
-                            // The original file still exist, consider it is new.
-                            postProcessNew();
-                            qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory();
-                            if (item->isDirectory()) {
-                                auto job = new ProcessDirectoryJob(item, recurseQueryServer, ParentDontExist, _discoveryData, this);
-                                job->_currentFolder = path;
-                                connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered);
-                                connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished);
-                                _queuedJobs.push_back(job);
+                        auto job = new RequestEtagJob(_discoveryData->_account, originalPath, this);
+                        connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result<QString> &etag) mutable {
+                            if (etag.errorCode() != 404 ||
+                                // Somehow another item claimed this original path, consider as if it existed
+                                _discoveryData->_renamedItems.contains(originalPath)) {
+                                // If the file exist or if there is another error, consider it is a new file.
+                                postProcessNew();
                             } else {
-                                emit itemDiscovered(item);
-                            }
+                                // The file do not exist, it is a rename
 
-                            _pendingAsyncJobs--;
-                            progress();
-                        };
-                        connect(job, &PropfindJob::result, this, considerNew);
-                        connect(job, &PropfindJob::finishedWithError, this, [=](QNetworkReply *reply) mutable {
-                            if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() != 404) {
-                                qDebug() << reply->request().url() << reply->error() << reply->errorString();
-                                qFatal("TODO: Handle error");
-                            }
-                            if (_discoveryData->_renamedItems.contains(originalPath)) {
-                                // Somehow another item claimed this original path, consider as if it existed
-                                considerNew();
-                                return;
-                            }
+                                // In case the deleted item was discovered in parallel
+                                auto it = _discoveryData->_deletedItem.find(originalPath);
+                                if (it != _discoveryData->_deletedItem.end()) {
+                                    ASSERT((*it)->_instruction == CSYNC_INSTRUCTION_REMOVE);
+                                    (*it)->_instruction = CSYNC_INSTRUCTION_NONE;
+                                }
+                                delete _discoveryData->_queuedDeletedDirectories.take(originalPath);
 
-                            // In case the deleted item was discovered in parallel
-                            auto it = _discoveryData->_deletedItem.find(originalPath);
-                            if (it != _discoveryData->_deletedItem.end()) {
-                                ASSERT((*it)->_instruction == CSYNC_INSTRUCTION_REMOVE);
-                                (*it)->_instruction = CSYNC_INSTRUCTION_NONE;
+                                postProcessRename(path);
                             }
-                            delete _discoveryData->_queuedDeletedDirectories.take(originalPath);
-
-                            // Normal use case: this is a rename.
-                            postProcessRename(path);
 
                             qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory();
 
                             if (item->isDirectory()) {
-                                auto job = new ProcessDirectoryJob(item, recurseQueryServer, NormalQuery, _discoveryData, this);
+                                auto job = new ProcessDirectoryJob(item, recurseQueryServer,
+                                    item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist,
+                                    _discoveryData, this);
                                 job->_currentFolder = path;
                                 connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered);
                                 connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished);
@@ -551,6 +538,9 @@ void ProcessDirectoryJob::processFile(PathTuple path,
             item->_size = serverEntry.size;
             if (serverEntry.isDirectory && dbEntry._type == ItemTypeDirectory) {
                 item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
+            } else if (!localEntry.isValid()) {
+                // Deleted locally, changed on server
+                item->_instruction = CSYNC_INSTRUCTION_NEW;
             } else {
                 item->_instruction = CSYNC_INSTRUCTION_SYNC;
             }
@@ -659,25 +649,28 @@ void ProcessDirectoryJob::processFile(PathTuple path,
                     isRename = item->_checksumHeader == base._checksumHeader;
                 }
             }
+            auto originalPath = QString::fromUtf8(base._path);
+            if (isRename && _discoveryData->_renamedItems.contains(originalPath))
+                isRename = false;
             if (isRename) {
-                auto originalPath = QString::fromUtf8(base._path);
-                auto it = _discoveryData->_deletedItem.find(originalPath);
                 QByteArray oldEtag;
+                auto it = _discoveryData->_deletedItem.find(originalPath);
                 if (it != _discoveryData->_deletedItem.end()) {
                     if ((*it)->_instruction != CSYNC_INSTRUCTION_REMOVE)
                         isRename = false;
                     else
                         (*it)->_instruction = CSYNC_INSTRUCTION_NONE;
                     oldEtag = (*it)->_etag;
-                    if (!item->isDirectory() && oldEtag != base._etag)
+                    if (!item->isDirectory() && oldEtag != base._etag) {
                         isRename = false;
-                } else {
-                    // FIXME!  We should do a server query to find out if the original path still exist and has the same etag
+                    }
                 }
-                if (_discoveryData->_renamedItems.contains(originalPath))
-                    isRename = false;
-                if (isRename) {
+                if (auto deleteJob = static_cast<ProcessDirectoryJob *>(_discoveryData->_queuedDeletedDirectories.value(originalPath).data())) {
+                    oldEtag = deleteJob->_dirItem->_etag;
                     delete _discoveryData->_queuedDeletedDirectories.take(originalPath);
+                }
+
+                auto processRename = [item, originalPath, base, this](PathTuple &path) {
                     _discoveryData->_renamedItems.insert(originalPath);
 
                     item->_modtime = base._modtime;
@@ -692,12 +685,55 @@ void ProcessDirectoryJob::processFile(PathTuple path,
                     item->_etag = base._etag;
                     path._original = originalPath;
                     path._server = originalPath;
-                    recurseQueryServer = oldEtag == base._etag ? ParentNotChanged : NormalQuery;
                     qCInfo(lcDisco) << "Rename detected (up) " << item->_file << " -> " << item->_renameTarget;
+                };
+                if (isRename && !oldEtag.isEmpty()) {
+                    recurseQueryServer = oldEtag == base._etag ? ParentNotChanged : NormalQuery;
+                    processRename(path);
+                } else if (isRename) {
+                    // We must query the server to know if the etag has not changed
+                    _pendingAsyncJobs++;
+                    auto job = new RequestEtagJob(_discoveryData->_account, originalPath, this);
+                    connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result<QString> &etag) mutable {
+                        if (!etag || (*etag != base._etag && !item->isDirectory()) || _discoveryData->_renamedItems.contains(originalPath)) {
+                            qCInfo(lcDisco) << "Can't rename because the etag has changed or the directory is gone" << originalPath;
+                            // Can't be a rename, leave it as a new.
+                        } else {
+                            // In case the deleted item was discovered in parallel
+                            auto it = _discoveryData->_deletedItem.find(originalPath);
+                            if (it != _discoveryData->_deletedItem.end()) {
+                                ASSERT((*it)->_instruction == CSYNC_INSTRUCTION_REMOVE);
+                                (*it)->_instruction = CSYNC_INSTRUCTION_NONE;
+                            }
+                            delete _discoveryData->_queuedDeletedDirectories.take(originalPath);
+
+                            processRename(path);
+                            recurseQueryServer = *etag == base._etag ? ParentNotChanged : NormalQuery;
+                        }
+
+                        qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory();
+                        if (item->isDirectory()) {
+                            auto job = new ProcessDirectoryJob(item, recurseQueryServer, NormalQuery, _discoveryData, this);
+                            job->_currentFolder = path;
+                            connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered);
+                            connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished);
+                            _queuedJobs.push_back(job);
+                        } else {
+                            emit itemDiscovered(item);
+                        }
+                        _pendingAsyncJobs--;
+                        progress();
+                    });
+                    job->start();
+                    return;
                 }
             }
         } else {
             item->_instruction = CSYNC_INSTRUCTION_SYNC;
+            if (_queryServer != ParentNotChanged && !serverEntry.isValid()) {
+                // Special case! deleted on server, modified on client, the instruction is then NEW
+                item->_instruction = CSYNC_INSTRUCTION_NEW;
+            }
             item->_direction = SyncFileItem::Up;
             item->_checksumHeader.clear();
             item->_size = localEntry.size;

diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h
index 10647690f50a25cb125938ebef42d09406c1cda4..727c44b2345cf034d1e8ea842b3d4f212d6571d5 100644 (file)
--- a/src/libsync/discovery.h
+++ b/src/libsync/discovery.h
@@ -24,51 +24,6 @@ class ExcludedFiles;
 namespace OCC {
 class SyncJournalDb;
 
-enum ErrorTag { Error };
-
-template <typename T>
-class Result
-{
-    union {
-        T _result;
-        QString _errorString;
-    };
-    bool _isError;
-
-public:
-    Result(T value)
-        : _result(std::move(value))
-        , _isError(false){};
-    Result(ErrorTag, QString str)
-        : _errorString(std::move(str))
-        , _isError(true)
-    {
-    }
-    ~Result()
-    {
-        if (_isError)
-            _errorString.~QString();
-        else
-            _result.~T();
-    }
-    explicit operator bool() const { return !_isError; }
-    const T &operator*() const &
-    {
-        ASSERT(!_isError);
-        return _result;
-    }
-    T operator*() &&
-    {
-        ASSERT(!_isError);
-        return std::move(_result);
-    }
-    QString errorMessage() const
-    {
-        ASSERT(_isError);
-        return _errorString;
-    }
-};
-
 struct RemoteInfo
 {
     QString name;

diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp
index fc8464558d3646697b8f8a2f5fb40ddf55adf17a..a32286b411b5fbf17ed8e505ed719bad1e3bb0af 100644 (file)
--- a/src/libsync/networkjobs.cpp
+++ b/src/libsync/networkjobs.cpp
@@ -96,7 +96,8 @@ bool RequestEtagJob::finished()
     qCInfo(lcEtagJob) << "Request Etag of" << reply()->request().url() << "FINISHED WITH STATUS"
                       <<  replyStatusString();
 
-    if (reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute) == 207) {
+    auto httpCode = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+    if (httpCode == 207) {
         // Parse DAV response
         QXmlStreamReader reader(reply());
         reader.addExtraNamespaceDeclaration(QXmlStreamNamespaceDeclaration("d", "DAV:"));
@@ -111,6 +112,9 @@ bool RequestEtagJob::finished()
             }
         }
         emit etagRetrieved(etag);
+        emit finishedWithResult(etag);
+    } else {
+        emit finishedWithResult({ httpCode, errorString() });
     }
     return true;
 }

diff --git a/src/libsync/networkjobs.h b/src/libsync/networkjobs.h
index 510a2a6bfb9e984ed947fd723967a2448dcfffec..743b87ac507dc9552035f4cba1398b6c13d7e4c3 100644 (file)
--- a/src/libsync/networkjobs.h
+++ b/src/libsync/networkjobs.h
@@ -335,6 +335,7 @@ public:
 
 signals:
     void etagRetrieved(const QString &etag);
+    void finishedWithResult(const Result<QString> &etag);
 
 private slots:
     bool finished() override;

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
index 33aa7414f6456e4578f84923ad4f4c7925a144bd..c5d43afe383f4bbcc32bbbf83e2f2d40b3aaadaf 100644 (file)
--- a/test/testsyncmove.cpp
+++ b/test/testsyncmove.cpp
@@ -470,6 +470,11 @@ private slots:
         {
             resetCounters();
             local.setContents("AM/a2m", 'C');
+            // We must change the modtime for it is likely that it did not change between sync.
+            // (Previous version of the client (<=2.5) would not need this because it was always doing
+            // checksum comparison for all renames. But newer version no longer does it if the file is
+            // renamed because the parent folder is renamed)
+            local.setModTime("AM/a2m", QDateTime::currentDateTimeUtc().addDays(3));
             local.rename("AM", "A2");
             remote.setContents("BM/b2m", 'C');
             remote.rename("BM", "B2");