From: Olivier Goffart Date: Thu, 11 Oct 2018 10:15:44 +0000 (+0200) Subject: Local discovery: always recurse within touched directory X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~475 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=75f66ddaa16f9d816c3b37dfa4ba9e36f6b78c0d;p=nextcloud-desktop.git 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 750ef9e3c..32efc0bf4 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -919,6 +919,22 @@ void SyncEngine::setLocalDiscoveryOptions(LocalDiscoveryStyle style, std::setstartsWith(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 34e1e2f47..9dd36012f 100644 --- 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)