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.

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index a84dd94c05e09e8968646ca498ac581f9ca7a194..6f4810aefc98e2711e982fbd4f00dab26af21e31 100644 (file)
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -325,10 +325,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;
@@ -374,7 +376,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;
@@ -802,38 +807,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);
@@ -854,6 +886,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 9373ae7c3c1c9239540f32cbb68c9d3686fe7e2f..952668fa8b9b738116da797efc2a10cb792e3d8a 100644 (file)
--- 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 730f879cf33f7fb737e2af29d4028fbab2657a82..6096c5c924351b2f130dc7dfa81ad412e41438c7 100644 (file)
--- a/test/testsyncvirtualfiles.cpp
+++ b/test/testsyncvirtualfiles.cpp
@@ -553,7 +553,8 @@ private slots:
 
         // If a file is renamed to <name>.nextcloud, it becomes virtual
         fakeFolder.localModifier().rename("A/a1", "A/a1.nextcloud");
-        // If a file is renamed to <random>.nextcloud, the file sticks around (to preserve user data)
+        // If a file is renamed to <random>.nextcloud, the rename propagates but the
+        // file isn't made virtual the first sync run.
         fakeFolder.localModifier().rename("A/a2", "A/rand.nextcloud");
         // dangling virtual files are removed
         fakeFolder.localModifier().insert("A/dangling.nextcloud", 1, ' ');
@@ -567,13 +568,13 @@ private slots:
 
         QVERIFY(!fakeFolder.currentLocalState().find("A/a2"));
         QVERIFY(!fakeFolder.currentLocalState().find("A/a2.nextcloud"));
-        QVERIFY(fakeFolder.currentLocalState().find("A/rand.nextcloud"));
+        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.nextcloud").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.nextcloud"));
-
         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.nextcloud"));
         QVERIFY(fakeFolder.currentLocalState().find("file2.nextcloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("file3.nextcloud"));
         cleanup();
 
         fakeFolder.localModifier().rename("file1.nextcloud", "renamed1.nextcloud");
         fakeFolder.localModifier().rename("file2.nextcloud", "renamed2.nextcloud");
         triggerDownload(fakeFolder, "file2");
+        triggerDownload(fakeFolder, "file3");
         QVERIFY(fakeFolder.syncOnce());
 
         QVERIFY(!fakeFolder.currentLocalState().find("file1.nextcloud"));
@@ -610,12 +614,109 @@ private slots:
         QVERIFY(dbRecord(fakeFolder, "renamed1.nextcloud").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.nextcloud"));
-        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.nextcloud"));
+        QVERIFY(fakeFolder.currentRemoteState().find("renamed2"));
+        QVERIFY(itemInstruction(completeSpy, "renamed2.nextcloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "renamed2.nextcloud")._type == ItemTypeVirtualFile);
+
+        QVERIFY(itemInstruction(completeSpy, "file3", CSYNC_INSTRUCTION_NEW));
+        cleanup();
+
+        // Test rename while adding/removing vfs suffix
+        fakeFolder.localModifier().rename("renamed1.nextcloud", "R1");
+        // Contents of file2 could also change at the same time...
+        fakeFolder.localModifier().rename("file3", "R3.nextcloud");
+        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.nextcloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case4"));
+        QVERIFY(fakeFolder.currentLocalState().find("case5.nextcloud"));
+        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.nextcloud", "case3-rename");
+
+        // Case 4: foo -> bar.oc (db unchanged)
+        fakeFolder.localModifier().rename("case4", "case4-rename.nextcloud");
+
+        // Case 5: foo -> bar (db dehydrate)
+        fakeFolder.localModifier().rename("case5.nextcloud", "case5-rename.nextcloud");
+        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.nextcloud"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case3-rename"));
+        QVERIFY(fakeFolder.currentLocalState().find("case3-rename.nextcloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case3"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case3-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case3-rename.nextcloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case3-rename.nextcloud")._type == ItemTypeVirtualFile);
+
+        // Case 4: the rename went though, dehydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case4"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case4.nextcloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case4-rename"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case4-rename.nextcloud"));
+        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.nextcloud"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case5-rename"));
+        QVERIFY(fakeFolder.currentLocalState().find("case5-rename.nextcloud"));
+        QVERIFY(!fakeFolder.currentRemoteState().find("case5"));
+        QVERIFY(fakeFolder.currentRemoteState().find("case5-rename"));
+        QVERIFY(itemInstruction(completeSpy, "case5-rename.nextcloud", CSYNC_INSTRUCTION_RENAME));
+        QVERIFY(dbRecord(fakeFolder, "case5-rename.nextcloud")._type == ItemTypeVirtualFile);
+
+        // Case 6: the rename went though, dehydration is forgotten
+        QVERIFY(!fakeFolder.currentLocalState().find("case6"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case6.nextcloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("case6-rename"));
+        QVERIFY(!fakeFolder.currentLocalState().find("case6-rename.nextcloud"));
+        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