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

@@ -21,8 +21,6 @@
 
 namespace OCC {
 
-SyncJournalFileRecord::SyncJournalFileRecord() = default;
-
 QByteArray SyncJournalFileRecord::numericFileId() const
 {
     // Use the id up until the first non-numeric character

src/common/syncjournalfilerecord.h

@@ -38,8 +38,6 @@ class SyncFileItem;
 class OCSYNC_EXPORT SyncJournalFileRecord
 {
 public:
-    SyncJournalFileRecord();
-
     bool isValid() const
     {
         return !_path.isEmpty();

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();
 

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<QString, QString> &map, RemoteInfo &result)
+static void propertyMapToRemoteInfo(const QMap<QString, QString> &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

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<QString, SyncFileItemPtr> _deletedItem;
     QMap<QString, ProcessDirectoryJob *> _queuedDeletedDirectories;
     QMap<QString, QString> _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 *);

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."));