Discovery: Virtual file handling adjustments

- adjust virtual file path handing
- helpers for vfs suffix adding/removal
- helpers for isDirectory/isVirtual on SyncJournalRecords
- be clear about what PathTuple _local/_server mean

src/common/syncjournalfilerecord.h

@@ -52,6 +52,9 @@ public:
     QByteArray numericFileId() const;
     QDateTime modDateTime() const { return Utility::qDateTimeFromTime_t(_modtime); }
 
+    bool isDirectory() const { return _type == ItemTypeDirectory; }
+    bool isVirtualFile() const { return _type == ItemTypeVirtualFile || _type == ItemTypeVirtualFileDownload; }
+
     QByteArray _path;
     quint64 _inode = 0;
     qint64 _modtime = 0;

src/libsync/discovery.cpp

@@ -85,9 +85,9 @@ void ProcessDirectoryJob::process()
     for (auto &e : _localNormalQueryEntries) {
         // Remove the virtual file suffix
         auto name = e.name;
-        if (e.name.endsWith(_discoveryData->_syncOptions._virtualFileSuffix)) {
+        if (hasVirtualFileSuffix(name)) {
             e.isVirtualFile = true;
-            name.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
+            chopVirtualFileSuffix(name);
             if (localEntriesHash.contains(name))
                 continue; // If there is both a virtual file and a real file, we must keep the real file
         }
@@ -100,9 +100,8 @@ void ProcessDirectoryJob::process()
     auto pathU8 = _currentFolder._original.toUtf8();
     if (!_discoveryData->_statedb->listFilesInPath(pathU8, [&](const SyncJournalFileRecord &rec) {
             auto name = pathU8.isEmpty() ? rec._path : QString::fromUtf8(rec._path.mid(pathU8.size() + 1));
-            if (rec._type == ItemTypeVirtualFile || rec._type == ItemTypeVirtualFileDownload) {
+            if (rec.isVirtualFile())
-                name.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
+                chopVirtualFileSuffix(name);
-            }
             entriesNames.insert(name);
             dbEntriesHash[name] = rec;
         })) {
@@ -117,18 +116,25 @@ void ProcessDirectoryJob::process()
         auto localEntry = localEntriesHash.value(f);
         auto serverEntry = serverEntriesHash.value(f);
         SyncJournalFileRecord record = dbEntriesHash.value(f);
-        PathTuple path;
 
-        // If there's a local virtual file, the server path should not have the suffix
+        PathTuple path;
-        // but local/original/db should have it.
+        path = _currentFolder.addName(f);
-        if ((localEntry.isValid() && localEntry.isVirtualFile)
+
-            || (_queryLocal == ParentNotChanged && record.isValid() && record._type == ItemTypeVirtualFile)) {
+        // If the file is virtual in the db, adjust path._original
-            Q_ASSERT(!localEntry.isValid() || localEntry.name.endsWith(_discoveryData->_syncOptions._virtualFileSuffix));
+        if (record.isVirtualFile()) {
-            QString name = f + _discoveryData->_syncOptions._virtualFileSuffix;
+            ASSERT(hasVirtualFileSuffix(record._path));
-            path = _currentFolder.addName(name);
+            addVirtualFileSuffix(path._original);
-            path._server.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
+        } else if (localEntry.isVirtualFile) {
-        } else {
+            // We don't have a db entry - but it should be at this path
-            path = _currentFolder.addName(f);
+            addVirtualFileSuffix(path._original);
+        }
+
+        // If the file is virtual locally, adjust path._local
+        if (localEntry.isVirtualFile) {
+            ASSERT(hasVirtualFileSuffix(localEntry.name));
+            addVirtualFileSuffix(path._local);
+        } else if (record.isVirtualFile() && _queryLocal == ParentNotChanged) {
+            addVirtualFileSuffix(path._local);
         }
 
         // If the filename starts with a . we consider it a hidden file
@@ -276,6 +282,12 @@ void ProcessDirectoryJob::processFile(PathTuple path,
     item->_file = path._target;
     item->_originalFile = path._original;
 
+    // The item shall only have this type if the db request for the virtual download
+    // was successful (like: no conflicting remote remove etc). This decision is done
+    // either in processFileAnalyzeRemoteInfo() or further down here.
+    if (item->_type == ItemTypeVirtualFileDownload)
+        item->_type = ItemTypeVirtualFile;
+
     if (serverEntry.isValid()) {
         processFileAnalyzeRemoteInfo(item, path, localEntry, serverEntry, dbEntry);
         return;
@@ -283,11 +295,11 @@ void ProcessDirectoryJob::processFile(PathTuple path,
 
     // Downloading a virtual file is like a server action and can happen even if
     // server-side nothing has changed
+    // NOTE: Normally setting the VirtualFileDownload flag means that local and
+    // remote will be rediscovered. This is just a fallback.
     if (_queryServer == ParentNotChanged && dbEntry._type == ItemTypeVirtualFileDownload) {
         item->_direction = SyncFileItem::Down;
         item->_instruction = CSYNC_INSTRUCTION_NEW;
-        Q_ASSERT(item->_file.endsWith(_discoveryData->_syncOptions._virtualFileSuffix));
-        item->_file.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
         item->_type = ItemTypeVirtualFileDownload;
     }
 
@@ -326,7 +338,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
 
     // The file is known in the db already
     if (dbEntry.isValid()) {
-        if (serverEntry.isDirectory != (dbEntry._type == ItemTypeDirectory)) {
+        if (serverEntry.isDirectory != dbEntry.isDirectory()) {
             // If the type of the entity changed, it's like NEW, but
             // needs to delete the other entity first.
             item->_instruction = CSYNC_INSTRUCTION_TYPE_CHANGE;
@@ -337,15 +349,12 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
             item->_direction = SyncFileItem::Down;
             item->_instruction = CSYNC_INSTRUCTION_NEW;
             item->_type = ItemTypeVirtualFileDownload;
-            item->_file = path._target;
-            if (item->_file.endsWith(_discoveryData->_syncOptions._virtualFileSuffix))
-                item->_file.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
         } else if (dbEntry._etag != serverEntry.etag) {
             item->_direction = SyncFileItem::Down;
             item->_modtime = serverEntry.modtime;
             item->_size = serverEntry.size;
             if (serverEntry.isDirectory) {
-                ENFORCE(dbEntry._type == ItemTypeDirectory);
+                ENFORCE(dbEntry.isDirectory());
                 item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
             } else if (!localEntry.isValid() && _queryLocal != ParentNotChanged) {
                 // Deleted locally, changed on server
@@ -373,7 +382,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
     item->_modtime = serverEntry.modtime;
     item->_size = serverEntry.size;
 
-    auto postProcessServerNew = [item, this, path, serverEntry, localEntry, dbEntry] {
+    auto postProcessServerNew = [item, this, path, serverEntry, localEntry, dbEntry] () mutable {
         if (item->isDirectory()) {
             _pendingAsyncJobs++;
             _discoveryData->checkSelectiveSyncNewFolder(path._server, serverEntry.remotePerm,
@@ -387,9 +396,9 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
             return;
         }
         // Turn new remote files into virtual files if the option is enabled.
-        if (_discoveryData->_syncOptions._newFilesAreVirtual && item->_type == ItemTypeFile) {
+        if (!localEntry.isValid() && _discoveryData->_syncOptions._newFilesAreVirtual && item->_type == ItemTypeFile) {
             item->_type = ItemTypeVirtualFile;
-            item->_file.append(_discoveryData->_syncOptions._virtualFileSuffix);
+            addVirtualFileSuffix(path._original);
         }
         processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer);
     };
@@ -552,18 +561,26 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
     bool serverModified = item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_SYNC
         || item->_instruction == CSYNC_INSTRUCTION_RENAME || item->_instruction == CSYNC_INSTRUCTION_TYPE_CHANGE;
 
-    if ((dbEntry.isValid() && dbEntry._type == ItemTypeVirtualFile)
+    // Decay server modifications to UPDATE_METADATA if the local virtual exists
-        || (localEntry.isValid() && localEntry.isVirtualFile && item->_type != ItemTypeVirtualFileDownload)) {
+    bool hasLocalVirtual = localEntry.isVirtualFile || (_queryLocal == ParentNotChanged && dbEntry.isVirtualFile());
-        // Do not download virtual files
+    bool virtualFileDownload = item->_type == ItemTypeVirtualFileDownload;
-        if (serverModified || dbEntry._type != ItemTypeVirtualFile)
+    if (serverModified && !virtualFileDownload && hasLocalVirtual) {
-            item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
+        item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
         serverModified = false;
         item->_type = ItemTypeVirtualFile;
     }
+
+    if (dbEntry.isVirtualFile() && !virtualFileDownload)
+        item->_type = ItemTypeVirtualFile;
+
     _childModified |= serverModified;
 
     auto finalize = [&] {
         bool recurse = item->isDirectory() || localEntry.isDirectory || serverEntry.isDirectory;
+        // Even if we have a local directory: If the remote is a file that's propagated as a
+        // conflict we don't need to recurse into it. (local c1.owncloud, c1/ ; remote: c1)
+        if (item->_instruction == CSYNC_INSTRUCTION_CONFLICT && !item->isDirectory())
+            recurse = false;
         if (_queryLocal != NormalQuery && _queryServer != NormalQuery && !item->_isRestoration)
             recurse = false;
 
@@ -588,7 +605,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             item->_instruction = CSYNC_INSTRUCTION_NEW;
             item->_direction = SyncFileItem::Down;
             item->_type = ItemTypeVirtualFile;
-            item->_file.append(_discoveryData->_syncOptions._virtualFileSuffix);
         } else if (!serverModified) {
             // Removed locally: also remove on the server.
             if (_dirItem && _dirItem->_isRestoration && _dirItem->_instruction == CSYNC_INSTRUCTION_NEW) {
@@ -612,7 +628,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
     item->_inode = localEntry.inode;
 
     if (dbEntry.isValid()) {
-        bool typeChange = localEntry.isDirectory != (dbEntry._type == ItemTypeDirectory);
+        bool typeChange = localEntry.isDirectory != dbEntry.isDirectory();
         if (localEntry.isVirtualFile) {
             if (dbEntry._type == ItemTypeFile) {
                 // If we find what looks to be a spurious "abc.owncloud" the base file "abc"
@@ -620,7 +636,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
                 // deleted from the server.
                 if (dbEntry._modtime == localEntry.modtime && dbEntry._fileSize == localEntry.size) {
                     qCInfo(lcDisco) << "Base file was renamed to virtual file:" << item->_file;
-                    Q_ASSERT(item->_file.endsWith(_discoveryData->_syncOptions._virtualFileSuffix));
                     item->_direction = SyncFileItem::Down;
                     item->_instruction = CSYNC_INSTRUCTION_NEW;
                     item->_type = ItemTypeVirtualFile;
@@ -632,7 +647,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             }
         } else if (!typeChange && ((dbEntry._modtime == localEntry.modtime && dbEntry._fileSize == localEntry.size) || localEntry.isDirectory)) {
             // Local file unchanged.
-            ENFORCE(localEntry.isDirectory == (dbEntry._type == ItemTypeDirectory));
+            ENFORCE(localEntry.isDirectory == dbEntry.isDirectory());
             if (noServerEntry) {
                 item->_instruction = CSYNC_INSTRUCTION_REMOVE;
                 item->_direction = SyncFileItem::Down;
@@ -640,7 +655,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
                 item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
                 item->_direction = SyncFileItem::Down; // Does not matter
             }
-        } else if (serverModified || dbEntry._type == ItemTypeVirtualFile) {
+        } else if (serverModified || dbEntry.isVirtualFile()) {
             processFileConflict(item, path, localEntry, serverEntry, dbEntry);
         } else if (typeChange) {
             item->_instruction = CSYNC_INSTRUCTION_TYPE_CHANGE;
@@ -687,9 +702,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         // Somehow there is a missing DB entry while the virtual file already exists.
         // The instruction should already be set correctly.
         ASSERT(item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA);
-        ASSERT(item->_type == ItemTypeVirtualFile)
+        ASSERT(item->_type == ItemTypeVirtualFile);
-        ASSERT(item->_file.endsWith(_discoveryData->_syncOptions._virtualFileSuffix));
-        item->_file.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
         finalize();
         return;
     } else if (serverModified) {
@@ -732,7 +745,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
     bool isMove = base.isValid() && base._type == item->_type
         && ((base._modtime == localEntry.modtime && base._fileSize == localEntry.size)
                // Directories and virtual files don't need size/mtime equality
-               || item->_type == ItemTypeDirectory || localEntry.isVirtualFile);
+               || localEntry.isDirectory || localEntry.isVirtualFile);
 
     if (isMove) {
         //  The old file must have been deleted.
@@ -764,19 +777,19 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         auto processRename = [item, originalPath, base, this](PathTuple &path) {
             auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath);
             _discoveryData->_renamedItems.insert(originalPath, path._target);
+            item->_renameTarget = path._target;
+            path._server = adjustedOriginalPath;
+            item->_file = path._server;
+            path._original = originalPath;
+            item->_originalFile = path._original;
             item->_modtime = base._modtime;
             item->_inode = base._inode;
             item->_instruction = CSYNC_INSTRUCTION_RENAME;
             item->_direction = SyncFileItem::Up;
-            item->_renameTarget = path._target;
-            item->_file = adjustedOriginalPath;
-            item->_originalFile = originalPath;
             item->_fileId = base._fileId;
             item->_remotePerm = base._remotePerm;
             item->_etag = base._etag;
             item->_type = base._type;
-            path._original = originalPath;
-            path._server = adjustedOriginalPath;
             qCInfo(lcDisco) << "Rename detected (up) " << item->_file << " -> " << item->_renameTarget;
         };
         if (wasDeletedOnClient.first) {
@@ -786,8 +799,8 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             // We must query the server to know if the etag has not changed
             _pendingAsyncJobs++;
             QString serverOriginalPath = originalPath;
-            if (localEntry.isVirtualFile)
+            if (base.isVirtualFile())
-                serverOriginalPath.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
+                chopVirtualFileSuffix(serverOriginalPath);
             auto job = new RequestEtagJob(_discoveryData->_account, serverOriginalPath, this);
             connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result<QString> &etag) mutable {
                 if (!etag || (*etag != base._etag && !item->isDirectory()) || _discoveryData->_renamedItems.contains(originalPath)) {
@@ -823,12 +836,9 @@ void ProcessDirectoryJob::processFileConflict(const SyncFileItemPtr &item, Proce
         return;
     }
 
-    if (dbEntry._type == ItemTypeVirtualFile)
+    // A conflict with a virtual should lead to virtual file download
+    if (dbEntry.isVirtualFile() || localEntry.isVirtualFile)
         item->_type = ItemTypeVirtualFileDownload;
-    if (item->_file.endsWith(_discoveryData->_syncOptions._virtualFileSuffix)) {
-        item->_file.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
-        item->_type = ItemTypeVirtualFileDownload;
-    }
 
     // If there's no content hash, use heuristics
     if (serverEntry.checksumHeader.isEmpty()) {
@@ -890,6 +900,15 @@ void ProcessDirectoryJob::processFileFinalize(
     const SyncFileItemPtr &item, PathTuple path, bool recurse,
     QueryMode recurseQueryLocal, QueryMode recurseQueryServer)
 {
+    // Adjust target path for virtual-suffix files
+    if (item->_type == ItemTypeVirtualFile) {
+        addVirtualFileSuffix(path._target);
+        if (item->_instruction == CSYNC_INSTRUCTION_RENAME)
+            addVirtualFileSuffix(item->_renameTarget);
+        else
+            addVirtualFileSuffix(item->_file);
+    }
+
     if (path._original != path._target && (item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA || item->_instruction == CSYNC_INSTRUCTION_NONE)) {
         ASSERT(_dirItem && _dirItem->_instruction == CSYNC_INSTRUCTION_RENAME);
         // This is because otherwise subitems are not updated!  (ideally renaming a directory could
@@ -919,8 +938,6 @@ void ProcessDirectoryJob::processFileFinalize(
         if (item->_instruction == CSYNC_INSTRUCTION_REMOVE
             // For the purpose of rename deletion, restored deleted placeholder is as if it was deleted
             || (item->_type == ItemTypeVirtualFile && item->_instruction == CSYNC_INSTRUCTION_NEW)) {
-            if (item->_type == ItemTypeVirtualFile && !path._original.endsWith(_discoveryData->_syncOptions._virtualFileSuffix))
-                path._original.append(_discoveryData->_syncOptions._virtualFileSuffix);
             _discoveryData->_deletedItem[path._original] = item;
         }
         emit _discoveryData->itemDiscovered(item);
@@ -937,7 +954,7 @@ void ProcessDirectoryJob::processBlacklisted(const PathTuple &path, const OCC::L
     item->_file = path._target;
     item->_originalFile = path._original;
     item->_inode = localEntry.inode;
-    if (dbEntry.isValid() && ((dbEntry._modtime == localEntry.modtime && dbEntry._fileSize == localEntry.size) || (localEntry.isDirectory && dbEntry._type == ItemTypeDirectory))) {
+    if (dbEntry.isValid() && ((dbEntry._modtime == localEntry.modtime && dbEntry._fileSize == localEntry.size) || (localEntry.isDirectory && dbEntry.isDirectory()))) {
         item->_instruction = CSYNC_INSTRUCTION_REMOVE;
         item->_direction = SyncFileItem::Down;
     } else {
@@ -1138,6 +1155,24 @@ void ProcessDirectoryJob::dbError()
     emit finished();
 }
 
+void ProcessDirectoryJob::addVirtualFileSuffix(QString &str) const
+{
+    str.append(_discoveryData->_syncOptions._virtualFileSuffix);
+}
+
+bool ProcessDirectoryJob::hasVirtualFileSuffix(const QString &str) const
+{
+    return str.endsWith(_discoveryData->_syncOptions._virtualFileSuffix);
+}
+
+void ProcessDirectoryJob::chopVirtualFileSuffix(QString &str) const
+{
+    bool hasSuffix = hasVirtualFileSuffix(str);
+    ASSERT(hasSuffix);
+    if (hasSuffix)
+        str.chop(_discoveryData->_syncOptions._virtualFileSuffix.size());
+}
+
 DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery()
 {
     auto serverJob = new DiscoverySingleDirectoryJob(_discoveryData->_account,

src/libsync/discovery.h

@@ -78,13 +78,20 @@ private:
      *
      * 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.
+     *
+     * _server and _local paths will differ if there are renames, example:
+     *   remote renamed A/ to B/ and local renamed A/X to A/Y then
+     *     target:   B/Y/file
+     *     original: A/X/file
+     *     local:    A/Y/file
+     *     server:   B/X/file
      */
     struct PathTuple
     {
         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 _server; // Path on the server (before the sync)
-        QString _local; // Path locally
+        QString _local; // Path locally (before the sync)
         PathTuple addName(const QString &name) const
         {
             PathTuple result;
@@ -151,6 +158,10 @@ private:
     /** An DB operation failed */
     void dbError();
 
+    void addVirtualFileSuffix(QString &str) const;
+    bool hasVirtualFileSuffix(const QString &str) const;
+    void chopVirtualFileSuffix(QString &str) const;
+
     /** Start a remote discovery network job
      *
      * It fills _serverNormalQueryEntries and sets _serverQueryDone when done.