FolderWatcher (linux): optimize slotAddFolderRecursive

- Instead of putting all the pathes in a big QStringList and then process them,
  do the processing as we iterate with QDirIterator

- Since we know that the parent directory is not ignored, we can simplify the
  ignore check by not checking the part before (i.e: we call
  ExcludedFiles::isExcluded with the parent 'path' as base path) This is what
  speeds up considerably the function.

Note that the OWNCLOUD_TEST check is already there in FolderWatcher::pathIsIgnored
so the behavior is not changed

csync/src/csync_rename.cc

@@ -20,6 +20,7 @@
 
 extern "C" {
 #include "csync_private.h"
+#include "csync_rename.h"
 }
 
 #include <map>
@@ -93,5 +94,9 @@ char* csync_rename_adjust_path_source(CSYNC* ctx, const char* path)
     return c_strdup(path);
 }
 
+bool csync_rename_count(CSYNC *ctx) {
+    csync_rename_s* d = csync_rename_s::get(ctx);
+    return d->folder_renamed_from.size();
+}
 
 }

csync/src/csync_rename.h

@@ -32,6 +32,8 @@ char OCSYNC_EXPORT *csync_rename_adjust_path(CSYNC *ctx, const char *path);
 char OCSYNC_EXPORT *csync_rename_adjust_path_source(CSYNC *ctx, const char *path);
 void OCSYNC_EXPORT csync_rename_destroy(CSYNC *ctx);
 void OCSYNC_EXPORT csync_rename_record(CSYNC *ctx, const char *from, const char *to);
+/*  Return the amount of renamed item recorded */
+bool OCSYNC_EXPORT csync_rename_count(CSYNC *ctx);
 
 #ifdef __cplusplus
 }

src/gui/application.cpp

@@ -336,8 +336,13 @@ void Application::slotownCloudWizardDone( int res )
         _checkConnectionTimer.start();
         slotCheckConnection();
 
-        // The very first time an account is configured: enabled autostart
-        // TODO: Doing this every time the account wizard finishes will annoy users.
+        // If one account is configured: enable autostart
+        bool shouldSetAutoStart = (accountMan->accounts().size() == 1);
+#ifdef Q_OS_MAC
+        // Don't auto start when not being 'installed'
+        shouldSetAutoStart = shouldSetAutoStart
+                && QCoreApplication::applicationDirPath().startsWith("/Applications/");
+#endif
         Utility::setLaunchOnStartup(_theme->appName(), _theme->appNameGUI(), true);
     }
 }

src/gui/folder.cpp

@@ -188,7 +188,7 @@ QString Folder::shortGuiLocalPath() const
 }
 
 
-bool Folder::ignoreHiddenFiles()
+bool Folder::ignoreHiddenFiles() const
 {
     bool re(_definition.ignoreHiddenFiles);
     return re;

src/gui/folder.h

@@ -167,7 +167,7 @@ public:
       * Ignore syncing of hidden files or not. This is defined in the
       * folder definition
       */
-     bool ignoreHiddenFiles();
+     bool ignoreHiddenFiles() const;
      void setIgnoreHiddenFiles(bool ignore);
 
      // Used by the Socket API

src/gui/folderwatcher_linux.cpp

@@ -23,6 +23,7 @@
 #include <QStringList>
 #include <QObject>
 #include <QVarLengthArray>
+#include <syncengine.h>
 
 namespace OCC {
 
@@ -48,31 +49,6 @@ FolderWatcherPrivate::~FolderWatcherPrivate()
 
 }
 
-// attention: result list passed by reference!
-bool FolderWatcherPrivate::findFoldersBelow( const QDir& dir, QStringList& fullList )
-{
-    bool ok = true;
-    if( !(dir.exists() && dir.isReadable()) ) {
-        qDebug() << "Non existing path coming in: " << dir.absolutePath();
-        ok = false;
-    } else {
-        QStringList nameFilter;
-        nameFilter << QLatin1String("*");
-        QDir::Filters filter = QDir::Dirs | QDir::NoDotAndDotDot | QDir::NoSymLinks | QDir::Hidden;
-        const QStringList pathes = dir.entryList(nameFilter, filter);
-
-        QStringList::const_iterator constIterator;
-        for (constIterator = pathes.constBegin(); constIterator != pathes.constEnd();
-               ++constIterator) {
-            const QString fullPath(dir.path()+QLatin1String("/")+(*constIterator));
-            fullList.append(fullPath);
-            ok = findFoldersBelow(QDir(fullPath), fullList);
-        }
-    }
-
-    return ok;
-}
-
 void FolderWatcherPrivate::inotifyRegisterPath(const QString& path)
 {
     if( !path.isEmpty()) {
@@ -88,41 +64,47 @@ void FolderWatcherPrivate::inotifyRegisterPath(const QString& path)
 
 void FolderWatcherPrivate::slotAddFolderRecursive(const QString &path)
 {
-    int subdirs = 0;
     qDebug() << "(+) Watcher:" << path;
-
-    QDir inPath(path);
-    inotifyRegisterPath(inPath.absolutePath());
-
     const QStringList watchedFolders = _watches.values();
-
-    QStringList allSubfolders;
-    if( !findFoldersBelow(QDir(path), allSubfolders)) {
-        qDebug() << "Could not traverse all sub folders";
-    }
-    // qDebug() << "currently watching " << watchedFolders;
-    QStringListIterator subfoldersIt(allSubfolders);
-    while (subfoldersIt.hasNext()) {
-        QString subfolder = subfoldersIt.next();
-        // qDebug() << "  (**) subfolder: " << subfolder;
-        QDir folder (subfolder);
-        if (folder.exists() && !watchedFolders.contains(folder.absolutePath())) {
-            subdirs++;
-            if( _parent->pathIsIgnored(subfolder) ) {
-                qDebug() << "* Not adding" << folder.path();
-                continue;
-            }
-            inotifyRegisterPath(folder.absolutePath());
-        } else {
-            qDebug() << "    `-> discarded:" << folder.path();
-        }
-    }
-
+    int subdirs = addFolderRecursiveHelper(path, watchedFolders.toSet());
     if (subdirs >0) {
         qDebug() << "    `-> and" << subdirs << "subdirectories";
     }
 }
 
+int FolderWatcherPrivate::addFolderRecursiveHelper(const QString &path, const QSet<QString> &watchedFolders)
+{
+    QDir dir(path);
+    if( !(dir.exists() && dir.isReadable()) ) {
+        qDebug() << "Non existing path coming in: " << dir.absolutePath();
+        return 0;
+    }
+    int subdirs = 1;
+
+    inotifyRegisterPath(dir.absolutePath());
+
+    QDir::Filters filter = QDir::Dirs | QDir::NoDotAndDotDot | QDir::NoSymLinks | QDir::Hidden;
+    const QStringList pathes = dir.entryList(filter);
+    for (auto constIterator = pathes.constBegin(); constIterator != pathes.constEnd(); ++constIterator) {
+        const QString subfolder = path + QLatin1String("/") + (*constIterator);
+        QDir folder(subfolder);
+        if (folder.exists() && !watchedFolders.contains(subfolder)) {
+#ifndef OWNCLOUD_TEST // InotifyWatcherTest is not interested in ignored files and does not link against the folder
+            if( _parent->_folder->syncEngine().excludedFiles().isExcluded(
+                    subfolder, path, _parent->_folder->ignoreHiddenFiles())) {
+                qDebug() << "* Not adding" << folder.path();
+                continue;
+            }
+#endif
+            subdirs += addFolderRecursiveHelper(subfolder, watchedFolders);
+        } else {
+            qDebug() << "    `-> discarded:" << folder.path();
+        }
+    }
+    return subdirs;
+}
+
+
 void FolderWatcherPrivate::slotReceivedNotification(int fd)
 {
     int len;

src/gui/folderwatcher_linux.h

@@ -33,7 +33,6 @@ class FolderWatcherPrivate : public QObject
 {
     Q_OBJECT
 public:
-    FolderWatcherPrivate() { }
     FolderWatcherPrivate(FolderWatcher *p, const QString &path);
     ~FolderWatcherPrivate();
 
@@ -45,10 +44,9 @@ protected slots:
     void slotAddFolderRecursive(const QString &path);
 
 protected:
-    bool findFoldersBelow( const QDir& dir, QStringList& fullList );
+    int addFolderRecursiveHelper(const QString &path, const QSet<QString> &watchedFolders);
     void inotifyRegisterPath(const QString& path);
 
-private:
     FolderWatcher *_parent;
 
     QString _folder;

src/libsync/discoveryphase.cpp

@@ -14,6 +14,7 @@
 
 #include "discoveryphase.h"
 #include <csync_private.h>
+#include <csync_rename.h>
 #include <qdebug.h>
 
 #include <QUrl>
@@ -51,7 +52,7 @@ static bool findPathInList(const QStringList &list, const QString &path)
     return pathSlash.startsWith(*it);
 }
 
-bool DiscoveryJob::isInSelectiveSyncBlackList(const QString& path) const
+bool DiscoveryJob::isInSelectiveSyncBlackList(const char *path) const
 {
     if (_selectiveSyncBlackList.isEmpty()) {
         // If there is no black list, everything is allowed
@@ -59,13 +60,25 @@ bool DiscoveryJob::isInSelectiveSyncBlackList(const QString& path) const
     }
 
     // Block if it is in the black list
-    return findPathInList(_selectiveSyncBlackList, path);
+    if (findPathInList(_selectiveSyncBlackList, QString::fromUtf8(path))) {
+        return true;
+    }
 
+    // Also try to adjust the path if there was renames
+    if (csync_rename_count(_csync_ctx)) {
+        QScopedPointer<char, QScopedPointerPodDeleter> adjusted(
+            csync_rename_adjust_path_source(_csync_ctx, path));
+        if (strcmp(adjusted.data(), path) != 0) {
+            return findPathInList(_selectiveSyncBlackList, QString::fromUtf8(adjusted.data()));
+        }
+    }
+
+    return false;
 }
 
 int DiscoveryJob::isInSelectiveSyncBlackListCallback(void *data, const char *path)
 {
-    return static_cast<DiscoveryJob*>(data)->isInSelectiveSyncBlackList(QString::fromUtf8(path));
+    return static_cast<DiscoveryJob*>(data)->isInSelectiveSyncBlackList(path);
 }
 
 bool DiscoveryJob::checkSelectiveSyncNewFolder(const QString& path)

src/libsync/discoveryphase.h

@@ -174,7 +174,7 @@ class DiscoveryJob : public QObject {
      * return true if the given path should be ignored,
      * false if the path should be synced
      */
-    bool isInSelectiveSyncBlackList(const QString &path) const;
+    bool isInSelectiveSyncBlackList(const char* path) const;
     static int isInSelectiveSyncBlackListCallback(void *, const char *);
     bool checkSelectiveSyncNewFolder(const QString &path);
     static int checkSelectiveSyncNewFolderCallback(void*, const char*);

src/libsync/propagateremotemove.cpp

@@ -13,6 +13,7 @@
  */
 
 #include "propagateremotemove.h"
+#include "propagatorjobs.h"
 #include "owncloudpropagator_p.h"
 #include "account.h"
 #include "syncjournalfilerecord.h"
@@ -175,10 +176,45 @@ void PropagateRemoteMove::finalize()
         done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
         return;
     }
+
+    if (_item->_isDirectory) {
+        if (!adjustSelectiveSync(_propagator->_journal, _item->_file, _item->_renameTarget)) {
+            done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
+            return;
+        }
+    }
+
     _propagator->_journal->commit("Remote Rename");
     done(SyncFileItem::Success);
 }
 
+bool PropagateRemoteMove::adjustSelectiveSync(SyncJournalDb *journal, const QString &from_, const QString &to_)
+{
+    bool ok;
+    // We only care about preserving the blacklist.   The white list should anyway be empty.
+    // And the undecided list will be repopulated on the next sync, if there is anything too big.
+    QStringList list = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok);
+    if (!ok)
+        return false;
+
+    bool changed = false;
+    Q_ASSERT(!from_.endsWith(QLatin1String("/")));
+    Q_ASSERT(!to_.endsWith(QLatin1String("/")));
+    QString from = from_ + QLatin1String("/");
+    QString to = to_ + QLatin1String("/");
+
+    for (auto it = list.begin(); it != list.end(); ++it) {
+        if (it->startsWith(from)) {
+            *it = it->replace(0, from.size(), to);
+            changed = true;
+        }
+    }
+
+    if (changed) {
+        journal->setSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, list);
+    }
+    return true;
+}
 
 }
 

src/libsync/propagateremotemove.h

@@ -51,6 +51,12 @@ public:
     void start() Q_DECL_OVERRIDE;
     void abort() Q_DECL_OVERRIDE;
     JobParallelism parallelism() Q_DECL_OVERRIDE { return OCC::PropagatorJob::WaitForFinishedInParentDirectory; }
+
+    /**
+     * Rename the directory in the selective sync list
+     */
+    static bool adjustSelectiveSync(SyncJournalDb *journal, const QString &from, const QString &to);
+
 private slots:
     void slotMoveJobFinished();
     void finalize();

src/libsync/propagatorjobs.cpp

@@ -15,7 +15,7 @@
 
 #include "propagatorjobs.h"
 #include "owncloudpropagator_p.h"
-
+#include "propagateremotemove.h"
 #include "utility.h"
 #include "syncjournaldb.h"
 #include "syncjournalfilerecord.h"
@@ -231,6 +231,7 @@ void PropagateLocalRename::start()
     _propagator->_journal->deleteFileRecord(_item->_originalFile);
 
     // store the rename file name in the item.
+    const auto oldFile = _item->_file;
     _item->_file = _item->_renameTarget;
 
     SyncJournalFileRecord record(*_item, targetFile);
@@ -245,9 +246,14 @@ void PropagateLocalRename::start()
             done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
             return;
         }
+    } else {
+        if (!PropagateRemoteMove::adjustSelectiveSync(_propagator->_journal, oldFile, _item->_renameTarget)) {
+            done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
+            return;
+        }
     }
-    _propagator->_journal->commit("localRename");
 
+    _propagator->_journal->commit("localRename");
 
     done(SyncFileItem::Success);
 }

test/syncenginetestutils.h

@@ -28,6 +28,7 @@ inline QByteArray generateFileId() {
 
 class PathComponents : public QStringList {
 public:
+    PathComponents(const char *path) : PathComponents{QString::fromUtf8(path)} {}
     PathComponents(const QString &path) : QStringList{path.split('/', QString::SkipEmptyParts)} { }
     PathComponents(const QStringList &pathComponents) : QStringList{pathComponents} { }
 
@@ -90,8 +91,9 @@ public:
     void mkdir(const QString &relativePath) override {
         _rootDir.mkpath(relativePath);
     }
-    void rename(const QString &, const QString &) override {
-        Q_ASSERT(!"not implemented");
+    void rename(const QString &from, const QString &to) override {
+        QVERIFY(_rootDir.exists(from));
+        QVERIFY(_rootDir.rename(from, to));
     }
 };
 
@@ -457,6 +459,36 @@ public:
     qint64 readData(char *, qint64) override { return 0; }
 };
 
+class FakeMoveReply : public QNetworkReply
+{
+    Q_OBJECT
+public:
+    FakeMoveReply(FileInfo &remoteRootFileInfo, QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
+    : QNetworkReply{parent} {
+        setRequest(request);
+        setUrl(request.url());
+        setOperation(op);
+        open(QIODevice::ReadOnly);
+
+        Q_ASSERT(request.url().path().startsWith(sRootUrl.path()));
+        QString fileName = request.url().path().mid(sRootUrl.path().length());
+        QString destPath = request.rawHeader("Destination");
+        Q_ASSERT(destPath.startsWith(sRootUrl.path()));
+        QString dest = destPath.mid(sRootUrl.path().length());
+        remoteRootFileInfo.rename(fileName, dest);
+        QMetaObject::invokeMethod(this, "respond", Qt::QueuedConnection);
+    }
+
+    Q_INVOKABLE void respond() {
+        setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 201);
+        emit metaDataChanged();
+        emit finished();
+    }
+
+    void abort() override { }
+    qint64 readData(char *, qint64) override { return 0; }
+};
+
 class FakeGetReply : public QNetworkReply
 {
     Q_OBJECT
@@ -552,6 +584,8 @@ protected:
             return new FakeMkcolReply{_remoteRootFileInfo, op, request, this};
         else if (verb == QLatin1String("DELETE"))
             return new FakeDeleteReply{_remoteRootFileInfo, op, request, this};
+        else if (verb == QLatin1String("MOVE"))
+            return new FakeMoveReply{_remoteRootFileInfo, op, request, this};
         else {
             qDebug() << verb << outgoingData;
             Q_UNREACHABLE();

test/testinotifywatcher.cpp

@@ -11,18 +11,23 @@
 
 using namespace OCC;
 
-class TestInotifyWatcher: public FolderWatcherPrivate
+
+struct FriendlyFolderWatcherPrivate : FolderWatcherPrivate
+{
+    using FolderWatcherPrivate::FolderWatcherPrivate;
+    friend class TestInotifyWatcher;
+};
+
+
+class TestInotifyWatcher : public QObject
 {
     Q_OBJECT
-
-private:
-    QString _root;
-
 private slots:
-    void initTestCase() {
-        qsrand(QTime::currentTime().msec());
+    // Test the recursive path listing function lists everything
+    void testAddFolderRecursiveHelper() {
+        QTemporaryDir tmpDir;
 
-        _root = QDir::tempPath() + "/" + "test_" + QString::number(qrand());
+        QString _root = tmpDir.path();
         qDebug() << "creating test directory tree in " << _root;
         QDir rootDir(_root);
 
@@ -32,13 +37,13 @@ private slots:
         rootDir.mkpath(_root + "/a1/b3/c3");
         rootDir.mkpath(_root + "/a2/b3/c3");
 
-    }
+        FriendlyFolderWatcherPrivate watcher(0, _root);
+        QVERIFY(watcher._fd >= 0);
+        QCoreApplication::processEvents(); // Let the slotAddFolderRecursive slot run;
+        QStringList dirs = watcher._watches.values();
 
-    // Test the recursive path listing function findFoldersBelow
-    void testDirsBelowPath() {
-        QStringList dirs;
+        QVERIFY( dirs.indexOf(_root)>-1);
 
-        bool ok = findFoldersBelow(QDir(_root), dirs);
         QVERIFY( dirs.indexOf(_root + "/a1")>-1);
         QVERIFY( dirs.indexOf(_root + "/a1/b1")>-1);
         QVERIFY( dirs.indexOf(_root + "/a1/b1/c1")>-1);
@@ -53,20 +58,13 @@ private slots:
         QVERIFY( dirs.indexOf(_root + "/a1/b3")>-1);
         QVERIFY( dirs.indexOf(_root + "/a1/b3/c3")>-1);
 
-        QVERIFY( dirs.indexOf(_root + "/a2"));
-        QVERIFY( dirs.indexOf(_root + "/a2/b3"));
-        QVERIFY( dirs.indexOf(_root + "/a2/b3/c3"));
+        QVERIFY( dirs.contains(_root + "/a2"));
+        QVERIFY( dirs.contains(_root + "/a2/b3"));
+        QVERIFY( dirs.contains(_root + "/a2/b3/c3"));
 
-        QVERIFY2(dirs.count() == 11, "Directory count wrong.");
-
-        QVERIFY2(ok, "findFoldersBelow failed.");
+        QCOMPARE(dirs.count(), 12);
     }
 
-    void cleanupTestCase() {
-        if( _root.startsWith(QDir::tempPath() )) {
-           system( QString("rm -rf %1").arg(_root).toLocal8Bit() );
-        }
-    }
 };
 
 QTEST_APPLESS_MAIN(TestInotifyWatcher)

test/testsyncengine.cpp

@@ -7,6 +7,7 @@
 
 #include <QtTest>
 #include "syncenginetestutils.h"
+#include <syncengine.h>
 
 using namespace OCC;
 
@@ -137,8 +138,8 @@ private slots:
         fakeFolder.syncOnce();
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         auto oldState = fakeFolder.currentLocalState();
-        QVERIFY(oldState.find(PathComponents("folder/folderB/folderA/file.txt")));
-        QVERIFY(!oldState.find(PathComponents("folder/folderA/file.txt")));
+        QVERIFY(oldState.find("folder/folderB/folderA/file.txt"));
+        QVERIFY(!oldState.find("folder/folderA/file.txt"));
 
         // This sync should not remove the file
         fakeFolder.syncOnce();
@@ -146,6 +147,72 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), oldState);
 
     }
+
+    void testSelectiveSyncModevFolder() {
+        // issue #5224
+        FakeFolder fakeFolder{FileInfo{ QString(), {
+            FileInfo { QStringLiteral("parentFolder"), {
+                FileInfo{ QStringLiteral("subFolderA"), { { QStringLiteral("fileA.txt"), 400 } } },
+                FileInfo{ QStringLiteral("subFolderB"), { { QStringLiteral("fileB.txt"), 400 } } }
+            }
+        }}}};
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        auto expectedServerState = fakeFolder.currentRemoteState();
+
+        // Remove subFolderA with selectiveSync:
+        fakeFolder.syncEngine().journal()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList,
+                                                                {"parentFolder/subFolderA/"});
+        fakeFolder.syncEngine().journal()->avoidReadFromDbOnNextSync("parentFolder/subFolderA/");
+
+        fakeFolder.syncOnce();
+
+        {
+            // Nothing changed on the server
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            // The local state should not have subFolderA
+            auto remoteState = fakeFolder.currentRemoteState();
+            remoteState.remove("parentFolder/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+
+        // Rename parentFolder on the server
+        fakeFolder.remoteModifier().rename("parentFolder", "parentFolderRenamed");
+        expectedServerState = fakeFolder.currentRemoteState();
+        fakeFolder.syncOnce();
+
+        {
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            auto remoteState = fakeFolder.currentRemoteState();
+            // The subFolderA should still be there on the server.
+            QVERIFY(remoteState.find("parentFolderRenamed/subFolderA/fileA.txt"));
+            // But not on the client because of the selective sync
+            remoteState.remove("parentFolderRenamed/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+
+        // Rename it again, locally this time.
+        fakeFolder.localModifier().rename("parentFolderRenamed", "parentThirdName");
+        fakeFolder.syncOnce();
+
+        {
+            auto remoteState = fakeFolder.currentRemoteState();
+            // The subFolderA should still be there on the server.
+            QVERIFY(remoteState.find("parentThirdName/subFolderA/fileA.txt"));
+            // But not on the client because of the selective sync
+            remoteState.remove("parentThirdName/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+
+            expectedServerState = fakeFolder.currentRemoteState();
+            QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItem &, const PropagatorJob &)));
+            fakeFolder.syncOnce(); // This sync should do nothing
+            QCOMPARE(completeSpy.count(), 0);
+
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+    }
+
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)