Local discovery: always recurse within touched directory

If the file system watcher tells us a directory was modified, we should
recurse into it because it means it is probably a new directory

Issue #6804

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index 750ef9e3cf24a5880b3bb24e021d5507128e1333..32efc0bf404d194d8edee1bb1ef86b6722bee007 100644 (file)
--- a/src/libsync/syncengine.cpp
+++ b/src/libsync/syncengine.cpp
@@ -919,6 +919,22 @@ void SyncEngine::setLocalDiscoveryOptions(LocalDiscoveryStyle style, std::set<QS
 {
     _localDiscoveryStyle = style;
     _localDiscoveryPaths = std::move(paths);
+
+    // Normalize to make sure that no path is a contained in another.
+    // Note: for simplicity, this code consider anything less than '/' as a path separator, so for
+    // example, this will remove "foo.bar" if "foo" is in the list. This will mean we might have
+    // some false positive, but that's Ok.
+    // This invariant is used in SyncEngine::shouldDiscoverLocally
+    QString prev;
+    auto it = _localDiscoveryPaths.begin();
+    while(it != _localDiscoveryPaths.end()) {
+        if (!prev.isNull() && it->startsWith(prev) && (prev.endsWith('/') || *it == prev || it->at(prev.size()) <= '/')) {
+            it = _localDiscoveryPaths.erase(it);
+        } else {
+            prev = *it;
+            ++it;
+        }
+    }
 }
 
 bool SyncEngine::shouldDiscoverLocally(const QString &path) const
@@ -926,14 +942,28 @@ bool SyncEngine::shouldDiscoverLocally(const QString &path) const
     if (_localDiscoveryStyle == LocalDiscoveryStyle::FilesystemOnly)
         return true;
 
+    // The intention is that if "A/X" is in _localDiscoveryPaths:
+    // - parent folders like "/", "A" will be discovered (to make sure the discovery reaches the
+    //   point where something new happened)
+    // - the folder itself "A/X" will be discovered
+    // - subfolders like "A/X/Y" will be discovered (so data inside a new or renamed folder will be
+    //   discovered in full)
+    // Check out TestLocalDiscovery::testLocalDiscoveryDecision()
+
     auto it = _localDiscoveryPaths.lower_bound(path);
-    if (it == _localDiscoveryPaths.end() || !it->startsWith(path))
+    if (it == _localDiscoveryPaths.end() || !it->startsWith(path)) {
+        // Maybe a subfolder of something in the list?
+        if (it != _localDiscoveryPaths.begin() && path.startsWith(*(--it))) {
+            return it->endsWith('/') || (path.size() > it->size() && path.at(it->size()) <= '/');
+        }
         return false;
+    }
 
     // maybe an exact match or an empty path?
     if (it->size() == path.size() || path.isEmpty())
         return true;
 
+    // Maybe a parent folder of something in the list?
     // check for a prefix + / match
     forever {
         if (it->size() > path.size() && it->at(path.size()) == '/')

diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp
index 34e1e2f4721098b42ce262b9a1bdcd8a66d531ca..9dd36012fc7f5135f2c3d8d3f152a259532d7eca 100644 (file)
--- a/test/testlocaldiscovery.cpp
+++ b/test/testlocaldiscovery.cpp
@@ -77,7 +77,7 @@ private slots:
 
         fakeFolder.syncEngine().setLocalDiscoveryOptions(
             LocalDiscoveryStyle::DatabaseAndFilesystem,
-            { "A/X", "foo bar space/touch", "foo/", "zzz" });
+            { "A/X", "A/X space", "A/X/beta", "foo bar space/touch", "foo/", "zzz", "zzzz" });
 
         QVERIFY(engine.shouldDiscoverLocally(""));
         QVERIFY(engine.shouldDiscoverLocally("A"));
@@ -85,11 +85,23 @@ private slots:
         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"));
+        // These are within "A/X" so they should be discovered
+        QVERIFY(engine.shouldDiscoverLocally("A/X/alpha"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X beta"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X/Y"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X space"));
+        QVERIFY(engine.shouldDiscoverLocally("A/X space/alpha"));
+        QVERIFY(!engine.shouldDiscoverLocally("A/Xylo/foo"));
+        QVERIFY(engine.shouldDiscoverLocally("zzzz/hello"));
+        QVERIFY(!engine.shouldDiscoverLocally("zzza/hello"));
+
+        QEXPECT_FAIL("", "There is a possibility of false positives if the set contains a path "
+            "which is a prefix, and that prefix is followed by a character less than '/'", Continue);
+        QVERIFY(!engine.shouldDiscoverLocally("A/X o"));
 
         fakeFolder.syncEngine().setLocalDiscoveryOptions(
             LocalDiscoveryStyle::DatabaseAndFilesystem,
@@ -97,6 +109,82 @@ private slots:
 
         QVERIFY(!engine.shouldDiscoverLocally(""));
     }
+
+    // Check whether item success and item failure adjusts the
+    // tracker correctly.
+    void testTrackerItemCompletion()
+    {
+        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);
+        auto trackerContains = [&](const char *path) {
+            return tracker.localDiscoveryPaths().find(path) != tracker.localDiscoveryPaths().end();
+        };
+
+        tracker.addTouchedPath("A/spurious");
+
+        fakeFolder.localModifier().insert("A/a3");
+        tracker.addTouchedPath("A/a3");
+
+        fakeFolder.localModifier().insert("A/a4");
+        fakeFolder.serverErrorPaths().append("A/a4");
+        // We're not adding a4 as touched, it's in the same folder as a3 and will be seen.
+        // And due to the error it should be added to the explicit list while a3 gets removed.
+
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths());
+        tracker.startSyncPartialDiscovery();
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/a3"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("A/a4"));
+        QVERIFY(!trackerContains("A/a3"));
+        QVERIFY(trackerContains("A/a4"));
+        QVERIFY(trackerContains("A/spurious")); // not removed since overall sync not successful
+
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::FilesystemOnly);
+        tracker.startSyncFullDiscovery();
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(!fakeFolder.currentRemoteState().find("A/a4"));
+        QVERIFY(trackerContains("A/a4")); // had an error, still here
+        QVERIFY(!trackerContains("A/spurious")); // removed due to full discovery
+
+        fakeFolder.serverErrorPaths().clear();
+        fakeFolder.syncJournal().wipeErrorBlacklist();
+        tracker.addTouchedPath("A/newspurious"); // will be removed due to successful sync
+
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths());
+        tracker.startSyncPartialDiscovery();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/a4"));
+        QVERIFY(tracker.localDiscoveryPaths().empty());
+    }
+
+    void testDirectoryAndSubDirectory()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+
+        fakeFolder.localModifier().mkdir("A/newDir");
+        fakeFolder.localModifier().mkdir("A/newDir/subDir");
+        fakeFolder.localModifier().insert("A/newDir/subDir/file", 10);
+
+        auto expectedState = fakeFolder.currentLocalState();
+
+        // Only "A" was modified according to the file system tracker
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(
+            LocalDiscoveryStyle::DatabaseAndFilesystem,
+            { "A" });
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(fakeFolder.currentLocalState(), expectedState);
+        QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
+    }
+
+
 };
 
 QTEST_GUILESS_MAIN(TestLocalDiscovery)