From c009dae1ce670113ba34e9993fe3b8322063772d Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 15 Oct 2018 17:02:04 +0200 Subject: [PATCH] New discovery algorithm: fixups Adapt reviews from ckamm in https://github.com/owncloud/client/pull/6738#pullrequestreview-164623532 - SyncJournalFileRecord: initialize everything inline - Add more comments - And some ENFORCE --- src/common/syncjournalfilerecord.cpp | 2 -- src/common/syncjournalfilerecord.h | 2 -- src/libsync/discovery.h | 34 ++++++++++++++++++++++++---- src/libsync/discoveryphase.cpp | 6 +++-- src/libsync/discoveryphase.h | 11 ++++++++- src/libsync/syncengine.cpp | 2 +- 6 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/common/syncjournalfilerecord.cpp b/src/common/syncjournalfilerecord.cpp index 24244eb12..6ca9f43af 100644 --- a/src/common/syncjournalfilerecord.cpp +++ b/src/common/syncjournalfilerecord.cpp @@ -21,8 +21,6 @@ namespace OCC { -SyncJournalFileRecord::SyncJournalFileRecord() = default; - QByteArray SyncJournalFileRecord::numericFileId() const { // Use the id up until the first non-numeric character diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h index 7f087dc2c..9686bce40 100644 --- a/src/common/syncjournalfilerecord.h +++ b/src/common/syncjournalfilerecord.h @@ -38,8 +38,6 @@ class SyncFileItem; class OCSYNC_EXPORT SyncJournalFileRecord { public: - SyncJournalFileRecord(); - bool isValid() const { return !_path.isEmpty(); diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index c914d5c87..6b11ff138 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -24,6 +24,22 @@ class ExcludedFiles; namespace OCC { class SyncJournalDb; +/** + * Job that handle the discovering of a directory. + * + * This includes: + * - Do a DiscoverySingleDirectoryJob network job which will do a PROPFIND of this directory + * - Stat all the entries in the local file system for this directory + * - Merge all invormation (and the information from the database) in order to know what need + * to be done for every file within this directory. + * - For every sub-directory within this directory, "recursively" create a new ProcessDirectoryJob + * + * This job is tightly couple with the DiscoveryPhase class. + * + * After being start()'ed, one must call progress() on this job until it emit finished(). + * This job will call DiscoveryPhase::scheduleMoreJobs when one of its sub-jobs is finished. + * DiscoveryPhase::scheduleMoreJobs is the one which will call progress(). + */ class ProcessDirectoryJob : public QObject { Q_OBJECT @@ -32,7 +48,7 @@ public: NormalQuery, ParentDontExist, // Do not query this folder because it does not exist ParentNotChanged, // No need to query this folder because it has not changed from what is in the DB - InBlackList // Do not query this folder because it is in th blacklist (remote entries only) + InBlackList // Do not query this folder because it is in the blacklist (remote entries only) }; Q_ENUM(QueryMode) explicit ProcessDirectoryJob(const SyncFileItemPtr &dirItem, QueryMode queryLocal, QueryMode queryServer, @@ -52,10 +68,16 @@ public: SyncFileItemPtr _dirItem; private: + /** Structure representing a path during discovery. A same path may have different value locally + * or on the server in case of renames. + * + * These strings never start or ends with slashes. They are all relative to the folder's root. + * Usually they are all the same and are even shared instance of the same QString. + */ struct PathTuple { - QString _original; // Path as in the DB - QString _target; // Path that will be the result after the sync + QString _original; // Path as in the DB (before the sync) + QString _target; // Path that will be the result after the sync (and will be in the DB) QString _server; // Path on the server QString _local; // Path locally PathTuple addName(const QString &name) const @@ -81,8 +103,12 @@ private: void processFileFinalize(const SyncFileItemPtr &item, PathTuple, bool recurse, QueryMode recurseQueryLocal, QueryMode recurseQueryServer); - // Return false if there is an error and that a directory must not be recursively be taken + /** Checks the permission for this item, if needed, change the item to a restoration item. + * @return false indicate that this is an error and if it is a directory, one should not recurse + * inside it. + */ bool checkPermissions(const SyncFileItemPtr &item); + void processBlacklisted(const PathTuple &, const LocalInfo &, const SyncJournalFileRecord &dbEntry); void subJobFinished(); diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index db9a7c8d4..d582ed1e1 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -147,7 +147,9 @@ QString DiscoveryPhase::adjustRenamedPath(const QString &original) const void DiscoveryPhase::startJob(ProcessDirectoryJob *job) { + ENFORCE(!_currentRootJob); connect(job, &ProcessDirectoryJob::finished, this, [this, job] { + ENFORCE(_currentRootJob == sender()); _currentRootJob = nullptr; if (job->_dirItem) emit itemDiscovered(job->_dirItem); @@ -221,7 +223,7 @@ void DiscoverySingleDirectoryJob::abort() } } -static void propertyMapToFileStat(const QMap &map, RemoteInfo &result) +static void propertyMapToRemoteInfo(const QMap &map, RemoteInfo &result) { for (auto it = map.constBegin(); it != map.constEnd(); ++it) { QString property = it.key(); @@ -289,7 +291,7 @@ void DiscoverySingleDirectoryJob::directoryListingIteratedSlot(QString file, con int slash = file.lastIndexOf('/'); result.name = file.mid(slash + 1); result.size = -1; - propertyMapToFileStat(map, result); + propertyMapToRemoteInfo(map, result); if (result.isDirectory) result.size = 0; if (result.size == -1 diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 486d8bce7..beeaab6b4 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -41,9 +41,12 @@ class Account; class SyncJournalDb; class ProcessDirectoryJob; - +/** + * Represent all the meta-data about a file in the server + */ struct RemoteInfo { + /** FileName of the entry (this does not contains any directory or path, just the plain name */ QString name; QByteArray etag; QByteArray fileId; @@ -60,6 +63,7 @@ struct RemoteInfo struct LocalInfo { + /** FileName of the entry (this does not contains any directory or path, just the plain name */ QString name; time_t modtime = 0; int64_t size = 0; @@ -149,6 +153,11 @@ public: QMap _deletedItem; QMap _queuedDeletedDirectories; QMap _renamedItems; // map source -> destinations + /** Given an original path, return the target path obtained when renaming is done. + * + * Note that it only considers parent directory renames. So if A/B got renamed to C/D, + * checking A/B/file would yield C/D/file, but checking A/B would yield A/B. + */ QString adjustRenamedPath(const QString &original) const; void startJob(ProcessDirectoryJob *); diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 6b063f775..bf2afca5c 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -582,9 +582,9 @@ void SyncEngine::slotStartDiscovery() _discoveryPhase->_localDir = _localPath; _discoveryPhase->_remoteFolder = _remotePath; _discoveryPhase->_syncOptions = _syncOptions; + _discoveryPhase->_shouldDiscoverLocaly = [this](const QString &s) { return shouldDiscoverLocally(s); }; _discoveryPhase->_selectiveSyncBlackList = selectiveSyncBlackList; _discoveryPhase->_selectiveSyncWhiteList = _journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, &ok); - _discoveryPhase->_shouldDiscoverLocaly = [this](const QString &s) { return shouldDiscoverLocally(s); }; if (!ok) { qCWarning(lcEngine) << "Unable to read selective sync list, aborting."; syncError(tr("Unable to read from the sync journal."));