VFS: Unbreak behavior for rename+hydrate #7001

Users can rename a file *and* add/remove the vfs suffix at the same time
leading to very complex sync actions. This patch doesn't add support for
them, but adds tests and makes sure these cases do not cause unintened
behavior.

The rename will be propagated, but the users's hydrate/dehydrate request
will be ignored.

3 files changed, 237 insertions, 49 deletions

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index 5c849e095..16b11cf11 100644
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -326,10 +326,12 @@ 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.
+    // remote will be rediscovered. This is just a fallback for a similar check
+    // in processFileAnalyzeRemoteInfo().
     if (_queryServer == ParentNotChanged
         && (dbEntry._type == ItemTypeVirtualFileDownload
-            || localEntry.type == ItemTypeVirtualFileDownload)) {
+            || localEntry.type == ItemTypeVirtualFileDownload)
+        && (localEntry.isValid() || _queryLocal == ParentNotChanged)) {
         item->_direction = SyncFileItem::Down;
         item->_instruction = CSYNC_INSTRUCTION_NEW;
         item->_type = ItemTypeVirtualFileDownload;
@@ -375,7 +377,10 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
             item->_direction = SyncFileItem::Down;
             item->_modtime = serverEntry.modtime;
             item->_size = serverEntry.size;
-        } else if (dbEntry._type == ItemTypeVirtualFileDownload || localEntry.type == ItemTypeVirtualFileDownload) {
+        } else if ((dbEntry._type == ItemTypeVirtualFileDownload || localEntry.type == ItemTypeVirtualFileDownload)
+            && (localEntry.isValid() || _queryLocal == ParentNotChanged)) {
+            // The above check for the localEntry existing is important. Otherwise it breaks
+            // the case where a file is moved and simultaneously tagged for download in the db.
             item->_direction = SyncFileItem::Down;
             item->_instruction = CSYNC_INSTRUCTION_NEW;
             item->_type = ItemTypeVirtualFileDownload;
@@ -794,38 +799,65 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         dbError();
         return;
     }
-    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
-               || localEntry.isDirectory || localEntry.isVirtualFile);
+    const auto originalPath = QString::fromUtf8(base._path);
+
+    // Function to gradually check conditions for accepting a move-candidate
+    auto moveCheck = [&]() {
+        if (!base.isValid()) {
+            qCInfo(lcDisco) << "Not a move, no item in db with inode" << localEntry.inode;
+            return false;
+        }
+        if (base.isDirectory() != item->isDirectory()) {
+            qCInfo(lcDisco) << "Not a move, types don't match" << base._type << item->_type << localEntry.type;
+            return false;
+        }
+        // Directories and virtual files don't need size/mtime equality
+        if (!localEntry.isDirectory && !base.isVirtualFile()
+            && (base._modtime != localEntry.modtime || base._fileSize != localEntry.size)) {
+            qCInfo(lcDisco) << "Not a move, mtime or size differs, "
+                            << "modtime:" << base._modtime << localEntry.modtime << ", "
+                            << "size:" << base._fileSize << localEntry.size;
+            return false;
+        }
 
-    auto originalPath = QString::fromUtf8(base._path);
-    if (isMove) {
         // The old file must have been deleted.
-        isMove = !QFile::exists(_discoveryData->_localDir + base._path)
-                // Exception: If the rename changes case only (like "foo" -> "Foo") the
-                // old filename might still point to the same file.
-                || (Utility::fsCasePreserving()
-                    && originalPath.compare(path._local, Qt::CaseInsensitive) == 0);
-    }
+        if (QFile::exists(_discoveryData->_localDir + base._path)
+            // Exception: If the rename changes case only (like "foo" -> "Foo") the
+            // old filename might still point to the same file.
+            && !(Utility::fsCasePreserving()
+                 && originalPath.compare(path._local, Qt::CaseInsensitive) == 0)) {
+            qCInfo(lcDisco) << "Not a move, base file still exists at" << originalPath;
+            return false;
+        }
 
-    // Verify the checksum where possible
-    if (isMove && !base._checksumHeader.isEmpty() && item->_type == ItemTypeFile) {
-        if (computeLocalChecksum(base._checksumHeader, _discoveryData->_localDir + path._original, item)) {
-            qCInfo(lcDisco) << "checking checksum of potential rename " << path._original << item->_checksumHeader << base._checksumHeader;
-            isMove = item->_checksumHeader == base._checksumHeader;
+        // Verify the checksum where possible
+        if (!base._checksumHeader.isEmpty() && item->_type == ItemTypeFile) {
+            if (computeLocalChecksum(base._checksumHeader, _discoveryData->_localDir + path._original, item)) {
+                qCInfo(lcDisco) << "checking checksum of potential rename " << path._original << item->_checksumHeader << base._checksumHeader;
+                if (item->_checksumHeader != base._checksumHeader) {
+                    qCInfo(lcDisco) << "Not a move, checksums differ";
+                    return false;
+                }
+            }
+        }
+
+        if (_discoveryData->isRenamed(originalPath)) {
+            qCInfo(lcDisco) << "Not a move, base path already renamed";
+            return false;
+        }
+
+        // Check local permission if we are allowed to put move the file here
+        // Technically we should use the one from the server, but we'll assume it is the same
+        if (!checkMovePermissions(base._remotePerm, originalPath, item->isDirectory())) {
+            qCInfo(lcDisco) << "Not a move, no permission to rename base file";
+            return false;
         }
-    }
-    if (isMove && _discoveryData->isRenamed(originalPath))
-        isMove = false;
 
-    //Check local permission if we are allowed to put move the file here
-    // Technically we should use the one from the server, but we'll assume it is the same
-    if (isMove && !checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()))
-        isMove = false;
+        return true;
+    };
 
     // Finally make it a NEW or a RENAME
-    if (!isMove) {
+    if (!moveCheck()) {
        postProcessLocalNew();
     } else {
         auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath);
@@ -846,6 +878,15 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             item->_remotePerm = base._remotePerm;
             item->_etag = base._etag;
             item->_type = base._type;
+
+            // Discard any download/dehydrate tags on the base file.
+            // They could be preserved and honored in a follow-up sync,
+            // but it complicates handling a lot and will happen rarely.
+            if (item->_type == ItemTypeVirtualFileDownload)
+                item->_type = ItemTypeVirtualFile;
+            if (item->_type == ItemTypeVirtualFileDehydration)
+                item->_type = ItemTypeFile;
+
             qCInfo(lcDisco) << "Rename detected (up) " << item->_file << " -> " << item->_renameTarget;
         };
         if (wasDeletedOnClient.first) {
diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp
index 44a7f9cb7..7cda141b5 100644
--- a/src/libsync/propagateremotemove.cpp
+++ b/src/libsync/propagateremotemove.cpp
@@ -89,21 +89,67 @@ void PropagateRemoteMove::start()
         return;
     }
 
-    QString source = propagator()->_remoteFolder + origin;
-    QString destination = QDir::cleanPath(propagator()->account()->davUrl().path() + propagator()->_remoteFolder + _item->_renameTarget);
+    QString remoteSource = propagator()->_remoteFolder + origin;
+    QString remoteDestination = QDir::cleanPath(propagator()->account()->davUrl().path() + propagator()->_remoteFolder + _item->_renameTarget);
+
     auto &vfs = propagator()->syncOptions()._vfs;
-    if (vfs->mode() == Vfs::WithSuffix
-        && (_item->_type == ItemTypeVirtualFile || _item->_type == ItemTypeVirtualFileDownload)) {
+    auto itype = _item->_type;
+    ASSERT(itype != ItemTypeVirtualFileDownload && itype != ItemTypeVirtualFileDehydration);
+    if (vfs->mode() == Vfs::WithSuffix && itype != ItemTypeDirectory) {
         const auto suffix = vfs->fileSuffix();
-        ASSERT(source.endsWith(suffix) && destination.endsWith(suffix));
-        if (source.endsWith(suffix) && destination.endsWith(suffix)) {
-            source.chop(suffix.size());
-            destination.chop(suffix.size());
+        bool sourceHadSuffix = remoteSource.endsWith(suffix);
+        bool destinationHadSuffix = remoteDestination.endsWith(suffix);
+
+        // Remote source and destination definitely shouldn't have the suffix
+        if (sourceHadSuffix)
+            remoteSource.chop(suffix.size());
+        if (destinationHadSuffix)
+            remoteDestination.chop(suffix.size());
+
+        QString folderTarget = _item->_renameTarget;
+
+        // Users can rename the file *and at the same time* add or remove the vfs
+        // suffix. That's a complicated case where a remote rename plus a local hydration
+        // change is requested. We don't currently deal with that. Instead, the rename
+        // is propagated and the local vfs suffix change is reverted.
+        // The discovery would still set up _renameTarget without the changed
+        // suffix, since that's what must be propagated to the remote but the local
+        // file may have a different name. folderTargetAlt will contain this potential
+        // name.
+        QString folderTargetAlt = folderTarget;
+        if (itype == ItemTypeFile) {
+            ASSERT(!sourceHadSuffix && !destinationHadSuffix);
+
+            // If foo -> bar.owncloud, the rename target will be "bar"
+            folderTargetAlt = folderTarget + suffix;
+
+        } else if (itype == ItemTypeVirtualFile) {
+            ASSERT(sourceHadSuffix && destinationHadSuffix);
+
+            // If foo.owncloud -> bar, the rename target will be "bar.owncloud"
+            folderTargetAlt.chop(suffix.size());
+        }
+
+        QString localTarget = propagator()->getFilePath(folderTarget);
+        QString localTargetAlt = propagator()->getFilePath(folderTargetAlt);
+
+        // If the expected target doesn't exist but a file with different hydration
+        // state does, rename the local file to bring it in line with what the discovery
+        // has set up.
+        if (!FileSystem::fileExists(localTarget) && FileSystem::fileExists(localTargetAlt)) {
+            QString error;
+            if (!FileSystem::uncheckedRenameReplace(localTargetAlt, localTarget, &error)) {
+                done(SyncFileItem::NormalError, tr("Could not rename %1 to %2, error: %3")
+                     .arg(folderTargetAlt, folderTarget, error));
+                return;
+            }
+            qCInfo(lcPropagateRemoteMove) << "Suffix vfs required local rename of"
+                                          << folderTargetAlt << "to" << folderTarget;
         }
     }
-    qCDebug(lcPropagateRemoteMove) << source << destination;
+    qCDebug(lcPropagateRemoteMove) << remoteSource << remoteDestination;
 
-    _job = new MoveJob(propagator()->account(), source, destination, this);
+    _job = new MoveJob(propagator()->account(), remoteSource, remoteDestination, this);
     connect(_job.data(), &MoveJob::finishedSignal, this, &PropagateRemoteMove::slotMoveJobFinished);
     propagator()->_activeJobList.append(this);
     _job->start();
@@ -162,9 +208,9 @@ void PropagateRemoteMove::finalize()
     propagator()->_journal->deleteFileRecord(_item->_originalFile);
 
     SyncFileItem newItem(*_item);
+    newItem._type = _item->_type;
     if (oldRecord.isValid()) {
         newItem._checksumHeader = oldRecord._checksumHeader;
-        newItem._type = oldRecord._type;
         if (newItem._size != oldRecord._fileSize) {
             qCWarning(lcPropagateRemoteMove) << "File sizes differ on server vs sync journal: " << newItem._size << oldRecord._fileSize;
 
diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp
index 5539b8f5b..6b99e21bb 100644
--- a/test/testsyncvirtualfiles.cpp
+++ b/test/testsyncvirtualfiles.cpp
@@ -553,7 +553,8 @@ private slots:
 
         // If a file is renamed to <name>.owncloud, it becomes virtual
         fakeFolder.localModifier().rename("A/a1", "A/a1.owncloud");
-        // If a file is renamed to <random>.owncloud, the file sticks around (to preserve user data)
+        // If a file is renamed to <random>.owncloud, the rename propagates but the
+        // file isn't made virtual the first sync run.
         fakeFolder.localModifier().rename("A/a2", "A/rand.owncloud");
         // dangling virtual files are removed
         fakeFolder.localModifier().insert("A/dangling.owncloud", 1, ' ');
@@ -567,13 +568,13 @@ private slots:
 
         QVERIFY(!fakeFolder.currentLocalState().find("A/a2"));
         QVERIFY(!fakeFolder.currentLocalState().find("A/a2.owncloud"));
-        QVERIFY(fakeFolder.currentLocalState().find("A/rand.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("A/rand"));
         QVERIFY(!fakeFolder.currentRemoteState().find("A/a2"));
-        QVERIFY(itemInstruction(completeSpy, "A/a2", CSYNC_INSTRUCTION_REMOVE));
-        QVERIFY(!dbRecord(fakeFolder, "A/rand.owncloud").isValid());
+        QVERIFY(fakeFolder.currentRemoteState().find("A/rand"));
+        QVERIFY(itemInstruction(completeSpy, "A/rand", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "A/rand")._type == ItemTypeFile);
 
         QVERIFY(!fakeFolder.currentLocalState().find("A/dangling.owncloud"));
-
         cleanup();
     }
 
@@ -591,15 +592,18 @@ private slots:
 
         fakeFolder.remoteModifier().insert("file1", 128, 'C');
         fakeFolder.remoteModifier().insert("file2", 256, 'C');
+        fakeFolder.remoteModifier().insert("file3", 256, 'C');
         QVERIFY(fakeFolder.syncOnce());
 
         QVERIFY(fakeFolder.currentLocalState().find("file1.owncloud"));
         QVERIFY(fakeFolder.currentLocalState().find("file2.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("file3.owncloud"));
         cleanup();
 
         fakeFolder.localModifier().rename("file1.owncloud", "renamed1.owncloud");
         fakeFolder.localModifier().rename("file2.owncloud", "renamed2.owncloud");
         triggerDownload(fakeFolder, "file2");
+        triggerDownload(fakeFolder, "file3");
         QVERIFY(fakeFolder.syncOnce());
 
         QVERIFY(!fakeFolder.currentLocalState().find("file1.owncloud"));
@@ -610,12 +614,109 @@ private slots:
         QVERIFY(dbRecord(fakeFolder, "renamed1.owncloud").isValid());
 
         // file2 has a conflict between the download request and the rename:
-        // currently the download wins
+        // the rename wins, the download is ignored
+        QVERIFY(!fakeFolder.currentLocalState().find("file2"));
         QVERIFY(!fakeFolder.currentLocalState().find("file2.owncloud"));
-        QVERIFY(fakeFolder.currentLocalState().find("file2"));
-        QVERIFY(fakeFolder.currentRemoteState().find("file2"));
-        QVERIFY(itemInstruction(completeSpy, "file2", CSYNC_INSTRUCTION_NEW));
-        QVERIFY(dbRecord(fakeFolder, "file2").isValid());
+        QVERIFY(fakeFolder.currentLocalState().find("renamed2.owncloud"));
+        QVERIFY(fakeFolder.currentRemoteState().find("renamed2"));
+        QVERIFY(itemInstruction(completeSpy, "renamed2.owncloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "renamed2.owncloud")._type == ItemTypeVirtualFile);
+
+        QVERIFY(itemInstruction(completeSpy, "file3", CSYNC_INSTRUCTION_NEW));
+        cleanup();
+
+        // Test rename while adding/removing vfs suffix
+        fakeFolder.localModifier().rename("renamed1.owncloud", "R1");
+        // Contents of file2 could also change at the same time...
+        fakeFolder.localModifier().rename("file3", "R3.owncloud");
+        QVERIFY(fakeFolder.syncOnce());
+        cleanup();
+    }
+
+    void testRenameVirtual2()
+    {
+        FakeFolder fakeFolder{ FileInfo() };
+        setupVfs(fakeFolder);
+        QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
+        auto cleanup = [&]() {
+            completeSpy.clear();
+        };
+        cleanup();
+
+        fakeFolder.remoteModifier().insert("case3", 128, 'C');
+        fakeFolder.remoteModifier().insert("case4", 256, 'C');
+        fakeFolder.remoteModifier().insert("case5", 256, 'C');
+        fakeFolder.remoteModifier().insert("case6", 256, 'C');
+        QVERIFY(fakeFolder.syncOnce());
+
+        triggerDownload(fakeFolder, "case4");
+        triggerDownload(fakeFolder, "case6");
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentLocalState().find("case3.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case4"));
+        QVERIFY(fakeFolder.currentLocalState().find("case5.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case6"));
+        cleanup();
+
+        // Case 1: foo -> bar (tested elsewhere)
+        // Case 2: foo.oc -> bar.oc (tested elsewhere)
+
+        // Case 3: foo.oc -> bar (db unchanged)
+        fakeFolder.localModifier().rename("case3.owncloud", "case3-rename");
+
+        // Case 4: foo -> bar.oc (db unchanged)
+        fakeFolder.localModifier().rename("case4", "case4-rename.owncloud");
+
+        // Case 5: foo -> bar (db dehydrate)
+        fakeFolder.localModifier().rename("case5.owncloud", "case5-rename.owncloud");
+        triggerDownload(fakeFolder, "case5");
+
+        // Case 6: foo.oc -> bar.oc (db hydrate)
+        fakeFolder.localModifier().rename("case6", "case6-rename");
+        markForDehydration(fakeFolder, "case6");
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        // Case 3: the rename went though, hydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case3"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case3.owncloud"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case3-rename"));
+        QVERIFY(fakeFolder.currentLocalState().find("case3-rename.owncloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case3"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case3-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case3-rename.owncloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case3-rename.owncloud")._type == ItemTypeVirtualFile);
+
+        // Case 4: the rename went though, dehydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case4"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case4.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case4-rename"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case4-rename.owncloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case4"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case4-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case4-rename", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case4-rename")._type == ItemTypeFile);
+
+        // Case 5: the rename went though, hydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case5"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case5.owncloud"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case5-rename"));
+        QVERIFY(fakeFolder.currentLocalState().find("case5-rename.owncloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case5"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case5-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case5-rename.owncloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case5-rename.owncloud")._type == ItemTypeVirtualFile);
+
+        // Case 6: the rename went though, dehydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case6"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case6.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case6-rename"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case6-rename.owncloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case6"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case6-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case6-rename", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case6-rename")._type == ItemTypeFile);
     }
 
     // Dehydration via sync works