LocalDiscoveryTracker: Separate from Folder and move to libsync

To allow relevant code to be closer together and for testing in
unittests without having to get a gui Folder.

See #6120

src/gui/folder.cpp

@@ -30,6 +30,7 @@
 #include "socketapi.h"
 #include "theme.h"
 #include "filesystem.h"
+#include "localdiscoverytracker.h"
 
 #include "creds/abstractcredentials.h"
 
@@ -110,6 +111,12 @@ Folder::Folder(const FolderDefinition &definition,
 
     connect(ProgressDispatcher::instance(), &ProgressDispatcher::folderConflicts,
         this, &Folder::slotFolderConflicts);
+
+    _localDiscoveryTracker.reset(new LocalDiscoveryTracker);
+    connect(_engine.data(), &SyncEngine::finished,
+        _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotSyncFinished);
+    connect(_engine.data(), &SyncEngine::itemCompleted,
+        _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted);
 }
 
 Folder::~Folder()
@@ -478,8 +485,7 @@ void Folder::slotWatchedPathChanged(const QString &path)
     // We do this before checking for our own sync-related changes to make
     // extra sure to not miss relevant changes.
     auto relativePathBytes = relativePath.toUtf8();
-    _localDiscoveryPaths.insert(relativePathBytes);
-    qCDebug(lcFolder) << "local discovery: inserted" << relativePath << "due to file watcher";
+    _localDiscoveryTracker->addTouchedPath(relativePathBytes);
 
 // The folder watcher fires a lot of bogus notifications during
 // a sync operation, both for actual user files and the database
@@ -681,22 +687,15 @@ void Folder::startSync(const QStringList &pathList)
         && hasDoneFullLocalDiscovery
         && !periodicFullLocalDiscoveryNow) {
         qCInfo(lcFolder) << "Allowing local discovery to read from the database";
-        _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, _localDiscoveryPaths);
-
-        if (lcFolder().isDebugEnabled()) {
-            QByteArrayList paths;
-            for (auto &path : _localDiscoveryPaths)
-                paths.append(path);
-            qCDebug(lcFolder) << "local discovery paths: " << paths;
-        }
-
-        _previousLocalDiscoveryPaths = std::move(_localDiscoveryPaths);
+        _engine->setLocalDiscoveryOptions(
+            LocalDiscoveryStyle::DatabaseAndFilesystem,
+            _localDiscoveryTracker->localDiscoveryPaths());
+        _localDiscoveryTracker->startSyncPartialDiscovery();
     } else {
         qCInfo(lcFolder) << "Forbidding local discovery to read from the database";
         _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::FilesystemOnly);
-        _previousLocalDiscoveryPaths.clear();
+        _localDiscoveryTracker->startSyncFullDiscovery();
     }
-    _localDiscoveryPaths.clear();
 
     _engine->setIgnoreHiddenFiles(_definition.ignoreHiddenFiles);
 
@@ -839,23 +838,14 @@ void Folder::slotSyncFinished(bool success)
         journalDb()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, QStringList());
     }
 
-    // bug: This function uses many different criteria for "sync was successful" - investigate!
     if ((_syncResult.status() == SyncResult::Success
             || _syncResult.status() == SyncResult::Problem)
         && success) {
         if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly) {
             _timeSinceLastFullLocalDiscovery.start();
         }
-        qCDebug(lcFolder) << "Sync success, forgetting last sync's local discovery path list";
-    } else {
-        // On overall-failure we can't forget about last sync's local discovery
-        // paths yet, reuse them for the next sync again.
-        // C++17: Could use std::set::merge().
-        _localDiscoveryPaths.insert(
-            _previousLocalDiscoveryPaths.begin(), _previousLocalDiscoveryPaths.end());
-        qCDebug(lcFolder) << "Sync failed, keeping last sync's local discovery path list";
     }
-    _previousLocalDiscoveryPaths.clear();
+
 
     emit syncStateChange();
 

src/gui/folder.h

@@ -39,6 +39,7 @@ class SyncEngine;
 class AccountState;
 class SyncRunFileLog;
 class FolderWatcher;
+class LocalDiscoveryTracker;
 
 /**
  * @brief The FolderDefinition class
@@ -397,20 +398,9 @@ private:
     QScopedPointer<FolderWatcher> _folderWatcher;
 
     /**
-     * The paths that should be checked by the next local discovery.
-     *
-     * Mostly a collection of files the filewatchers have reported as touched.
-     * Also includes files that have had errors in the last sync run.
+     * Keeps track of locally dirty files so we can skip local discovery sometimes.
      */
-    std::set<QByteArray> _localDiscoveryPaths;
-
-    /**
-     * The paths that the current sync run used for local discovery.
-     *
-     * For failing syncs, this list will be merged into _localDiscoveryPaths
-     * again when the sync is done to make sure everything is retried.
-     */
-    std::set<QByteArray> _previousLocalDiscoveryPaths;
+    QScopedPointer<LocalDiscoveryTracker> _localDiscoveryTracker;
 };
 }
 

src/libsync/CMakeLists.txt

@@ -48,6 +48,7 @@ set(libsync_SRCS
     syncfileitem.cpp
     syncfilestatus.cpp
     syncfilestatustracker.cpp
+    localdiscoverytracker.cpp
     syncresult.cpp
     theme.cpp
     clientsideencryption.cpp

src/libsync/localdiscoverytracker.cpp

@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) by Christian Kamm <mail@ckamm.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "localdiscoverytracker.h"
+
+#include "syncfileitem.h"
+
+#include <QLoggingCategory>
+
+using namespace OCC;
+
+Q_LOGGING_CATEGORY(lcLocalDiscoveryTracker, "sync.localdiscoverytracker", QtInfoMsg)
+
+LocalDiscoveryTracker::LocalDiscoveryTracker()
+{
+}
+
+void LocalDiscoveryTracker::addTouchedPath(const QByteArray &relativePath)
+{
+    qCDebug(lcLocalDiscoveryTracker) << "inserted touched" << relativePath;
+    _localDiscoveryPaths.insert(relativePath);
+}
+
+void LocalDiscoveryTracker::startSyncFullDiscovery()
+{
+    _localDiscoveryPaths.clear();
+    _previousLocalDiscoveryPaths.clear();
+    qCDebug(lcLocalDiscoveryTracker) << "full discovery";
+}
+
+void LocalDiscoveryTracker::startSyncPartialDiscovery()
+{
+    if (lcLocalDiscoveryTracker().isDebugEnabled()) {
+        QByteArrayList paths;
+        for (auto &path : _localDiscoveryPaths)
+            paths.append(path);
+        qCDebug(lcLocalDiscoveryTracker) << "partial discovery with paths: " << paths;
+    }
+
+    _previousLocalDiscoveryPaths = std::move(_localDiscoveryPaths);
+    _localDiscoveryPaths.clear();
+}
+
+const std::set<QByteArray> &LocalDiscoveryTracker::localDiscoveryPaths() const
+{
+    return _localDiscoveryPaths;
+}
+
+void LocalDiscoveryTracker::slotItemCompleted(const SyncFileItemPtr &item)
+{
+    // For successes, we want to wipe the file from the list to ensure we don't
+    // rediscover it even if this overall sync fails.
+    //
+    // For failures, we want to add the file to the list so the next sync
+    // will be able to retry it.
+    if (item->_status == SyncFileItem::Success
+        || item->_status == SyncFileItem::FileIgnored
+        || item->_status == SyncFileItem::Restoration
+        || item->_status == SyncFileItem::Conflict
+        || (item->_status == SyncFileItem::NoStatus
+               && (item->_instruction == CSYNC_INSTRUCTION_NONE
+                      || item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA))) {
+        if (_previousLocalDiscoveryPaths.erase(item->_file.toUtf8()))
+            qCDebug(lcLocalDiscoveryTracker) << "wiped successful item" << item->_file;
+    } else {
+        _localDiscoveryPaths.insert(item->_file.toUtf8());
+        qCDebug(lcLocalDiscoveryTracker) << "inserted error item" << item->_file;
+    }
+}
+
+void LocalDiscoveryTracker::slotSyncFinished(bool success)
+{
+    if (success) {
+        qCDebug(lcLocalDiscoveryTracker) << "sync success, forgetting last sync's local discovery path list";
+    } else {
+        // On overall-failure we can't forget about last sync's local discovery
+        // paths yet, reuse them for the next sync again.
+        // C++17: Could use std::set::merge().
+        _localDiscoveryPaths.insert(
+            _previousLocalDiscoveryPaths.begin(), _previousLocalDiscoveryPaths.end());
+        qCDebug(lcLocalDiscoveryTracker) << "sync failed, keeping last sync's local discovery path list";
+    }
+    _previousLocalDiscoveryPaths.clear();
+}

src/libsync/localdiscoverytracker.h

@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) by Christian Kamm <mail@ckamm.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#ifndef LOCALDISCOVERYTRACKER_H
+#define LOCALDISCOVERYTRACKER_H
+
+#include "owncloudlib.h"
+#include <set>
+#include <QObject>
+#include <QByteArray>
+#include <QSharedPointer>
+
+namespace OCC {
+
+class SyncFileItem;
+typedef QSharedPointer<SyncFileItem> SyncFileItemPtr;
+
+/**
+ * @brief Tracks files that must be rediscovered locally
+ *
+ * It does this by being notified about
+ * - modified files (addTouchedPath(), from file watcher)
+ * - starting syncs (startSync*())
+ * - finished items (slotItemCompleted(), by SyncEngine signal)
+ * - finished syncs (slotSyncFinished(), by SyncEngine signal)
+ *
+ * Then localDiscoveryPaths() can be used to determine paths to rediscover
+ * and send to SyncEngine::setLocalDiscoveryOptions().
+ *
+ * This class is primarily used from Folder and separate primarily for
+ * readability and testing purposes.
+ *
+ * All paths used in this class are expected to be utf8 encoded byte arrays,
+ * relative to the folder that is being synced, without a starting slash.
+ *
+ * @ingroup libsync
+ */
+class OWNCLOUDSYNC_EXPORT LocalDiscoveryTracker : public QObject
+{
+    Q_OBJECT
+public:
+    LocalDiscoveryTracker();
+
+    /** Adds a path that must be locally rediscovered later.
+     *
+     * This should be a full relative file path, example:
+     *   foo/bar/file.txt
+     */
+    void addTouchedPath(const QByteArray &relativePath);
+
+    /** Call when a sync run starts that rediscovers all local files */
+    void startSyncFullDiscovery();
+
+    /** Call when a sync using localDiscoveryPaths() starts */
+    void startSyncPartialDiscovery();
+
+    /** Access list of files that shall be locally rediscovered. */
+    const std::set<QByteArray> &localDiscoveryPaths() const;
+
+public slots:
+    /**
+     * Success and failure of sync items adjust what the next sync is
+     * supposed to do.
+     */
+    void slotItemCompleted(const SyncFileItemPtr &item);
+
+    /**
+     * When a sync finishes, the lists must be updated
+     */
+    void slotSyncFinished(bool success);
+
+private:
+    /**
+     * The paths that should be checked by the next local discovery.
+     *
+     * Mostly a collection of files the filewatchers have reported as touched.
+     * Also includes files that have had errors in the last sync run.
+     */
+    std::set<QByteArray> _localDiscoveryPaths;
+
+    /**
+     * The paths that the current sync run used for local discovery.
+     *
+     * For failing syncs, this list will be merged into _localDiscoveryPaths
+     * again when the sync is done to make sure everything is retried.
+     */
+    std::set<QByteArray> _previousLocalDiscoveryPaths;
+};
+
+} // namespace OCC
+
+#endif

test/CMakeLists.txt

@@ -55,6 +55,7 @@ nextcloud_add_test(ChunkingNg "syncenginetestutils.h")
 nextcloud_add_test(UploadReset "syncenginetestutils.h")
 nextcloud_add_test(AllFilesDeleted "syncenginetestutils.h")
 nextcloud_add_test(Blacklist "syncenginetestutils.h")
+nextcloud_add_test(LocalDiscovery "syncenginetestutils.h")
 nextcloud_add_test(FolderWatcher "${FolderWatcher_SRC}")
 
 if( UNIX AND NOT APPLE )

test/testlocaldiscovery.cpp

@@ -0,0 +1,103 @@
+/*
+ *    This software is in the public domain, furnished "as is", without technical
+ *    support, and with no warranty, express or implied, as to its usefulness for
+ *    any purpose.
+ *
+ */
+
+#include <QtTest>
+#include "syncenginetestutils.h"
+#include <syncengine.h>
+#include <localdiscoverytracker.h>
+
+using namespace OCC;
+
+class TestLocalDiscovery : public QObject
+{
+    Q_OBJECT
+
+private slots:
+    // Check correct behavior when local discovery is partially drawn from the db
+    void testLocalDiscoveryStyle()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+
+        LocalDiscoveryTracker tracker;
+        connect(&fakeFolder.syncEngine(), &SyncEngine::itemCompleted, &tracker, &LocalDiscoveryTracker::slotItemCompleted);
+        connect(&fakeFolder.syncEngine(), &SyncEngine::finished, &tracker, &LocalDiscoveryTracker::slotSyncFinished);
+
+        // More subdirectories are useful for testing
+        fakeFolder.localModifier().mkdir("A/X");
+        fakeFolder.localModifier().mkdir("A/Y");
+        fakeFolder.localModifier().insert("A/X/x1");
+        fakeFolder.localModifier().insert("A/Y/y1");
+        tracker.addTouchedPath("A/X");
+
+        tracker.startSyncFullDiscovery();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QVERIFY(tracker.localDiscoveryPaths().empty());
+
+        // Test begins
+        fakeFolder.localModifier().insert("A/a3");
+        fakeFolder.localModifier().insert("A/X/x2");
+        fakeFolder.localModifier().insert("A/Y/y2");
+        fakeFolder.localModifier().insert("B/b3");
+        fakeFolder.remoteModifier().insert("C/c3");
+
+        tracker.addTouchedPath("A/X");
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths());
+
+        tracker.startSyncPartialDiscovery();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/a3"));
+        QVERIFY(fakeFolder.currentRemoteState().find("A/X/x2"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("A/Y/y2"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("B/b3"));
+        QVERIFY(fakeFolder.currentLocalState().find("C/c3"));
+        QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::DatabaseAndFilesystem);
+        QVERIFY(tracker.localDiscoveryPaths().empty());
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::FilesystemOnly);
+        QVERIFY(tracker.localDiscoveryPaths().empty());
+    }
+
+    void testLocalDiscoveryDecision()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        auto &engine = fakeFolder.syncEngine();
+
+        QVERIFY(engine.shouldDiscoverLocally(""));
+        QVERIFY(engine.shouldDiscoverLocally("A"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X"));
+
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(
+            LocalDiscoveryStyle::DatabaseAndFilesystem,
+            { "A/X", "foo bar space/touch", "foo/", "zzz" });
+
+        QVERIFY(engine.shouldDiscoverLocally(""));
+        QVERIFY(engine.shouldDiscoverLocally("A"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X"));
+        QVERIFY(!engine.shouldDiscoverLocally("B"));
+        QVERIFY(!engine.shouldDiscoverLocally("A B"));
+        QVERIFY(!engine.shouldDiscoverLocally("B/X"));
+        QVERIFY(!engine.shouldDiscoverLocally("A/X/Y"));
+        QVERIFY(engine.shouldDiscoverLocally("foo bar space"));
+        QVERIFY(engine.shouldDiscoverLocally("foo"));
+        QVERIFY(!engine.shouldDiscoverLocally("foo bar"));
+        QVERIFY(!engine.shouldDiscoverLocally("foo bar/touch"));
+
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(
+            LocalDiscoveryStyle::DatabaseAndFilesystem,
+            {});
+
+        QVERIFY(!engine.shouldDiscoverLocally(""));
+    }
+};
+
+QTEST_GUILESS_MAIN(TestLocalDiscovery)
+#include "testlocaldiscovery.moc"

test/testsyncengine.cpp

@@ -572,72 +572,6 @@ private slots:
         QVERIFY(!fakeFolder.currentRemoteState().find("C/myfile.txt"));
     }
 
-    // Check correct behavior when local discovery is partially drawn from the db
-    void testLocalDiscoveryStyle()
-    {
-        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
-
-        // More subdirectories are useful for testing
-        fakeFolder.localModifier().mkdir("A/X");
-        fakeFolder.localModifier().mkdir("A/Y");
-        fakeFolder.localModifier().insert("A/X/x1");
-        fakeFolder.localModifier().insert("A/Y/y1");
-        QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-
-        // Test begins
-        fakeFolder.localModifier().insert("A/a3");
-        fakeFolder.localModifier().insert("A/X/x2");
-        fakeFolder.localModifier().insert("A/Y/y2");
-        fakeFolder.localModifier().insert("B/b3");
-        fakeFolder.remoteModifier().insert("C/c3");
-
-        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, { "A/X" });
-        QVERIFY(fakeFolder.syncOnce());
-        QVERIFY(fakeFolder.currentRemoteState().find("A/a3"));
-        QVERIFY(fakeFolder.currentRemoteState().find("A/X/x2"));
-        QVERIFY(!fakeFolder.currentRemoteState().find("A/Y/y2"));
-        QVERIFY(!fakeFolder.currentRemoteState().find("B/b3"));
-        QVERIFY(fakeFolder.currentLocalState().find("C/c3"));
-        QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::DatabaseAndFilesystem);
-
-        QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-        QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::FilesystemOnly);
-    }
-
-    void testLocalDiscoveryDecision()
-    {
-        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
-        auto &engine = fakeFolder.syncEngine();
-
-        QVERIFY(engine.shouldDiscoverLocally(""));
-        QVERIFY(engine.shouldDiscoverLocally("A"));
-        QVERIFY(engine.shouldDiscoverLocally("A/X"));
-
-        fakeFolder.syncEngine().setLocalDiscoveryOptions(
-            LocalDiscoveryStyle::DatabaseAndFilesystem,
-            { "A/X", "foo bar space/touch", "foo/", "zzz" });
-
-        QVERIFY(engine.shouldDiscoverLocally(""));
-        QVERIFY(engine.shouldDiscoverLocally("A"));
-        QVERIFY(engine.shouldDiscoverLocally("A/X"));
-        QVERIFY(!engine.shouldDiscoverLocally("B"));
-        QVERIFY(!engine.shouldDiscoverLocally("A B"));
-        QVERIFY(!engine.shouldDiscoverLocally("B/X"));
-        QVERIFY(!engine.shouldDiscoverLocally("A/X/Y"));
-        QVERIFY(engine.shouldDiscoverLocally("foo bar space"));
-        QVERIFY(engine.shouldDiscoverLocally("foo"));
-        QVERIFY(!engine.shouldDiscoverLocally("foo bar"));
-        QVERIFY(!engine.shouldDiscoverLocally("foo bar/touch"));
-
-        fakeFolder.syncEngine().setLocalDiscoveryOptions(
-            LocalDiscoveryStyle::DatabaseAndFilesystem,
-            {});
-
-        QVERIFY(!engine.shouldDiscoverLocally(""));
-    }
-
     void testDiscoveryHiddenFile()
     {
         FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };