From 831f3430ab7f7294b7de4b279991ca313e2efe80 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 11 Sep 2024 16:54:31 +0200 Subject: [PATCH 1/3] add automated tests for complex move scenario we rename the top folder and we rename a not direct child that is also moved to another folder below the original renamed top folder so a/b/c a/b/d/file will be e/b/c/f/file with a -> e d-> f Signed-off-by: Matthieu Gallien --- test/testsyncmove.cpp | 92 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 7dda06235400c..1266fdef4b0ee 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -1253,6 +1253,98 @@ private slots: QCOMPARE(counter.nMOVE, 2); QCOMPARE(counter.nMKCOL, 0); } + + void testMultipleRenameFromServer() + { + FakeFolder fakeFolder{{}}; + + fakeFolder.remoteModifier().mkdir("root"); + fakeFolder.remoteModifier().mkdir("root/stable"); + fakeFolder.remoteModifier().mkdir("root/stable/folder to move"); + fakeFolder.remoteModifier().insert("root/stable/folder to move/index.txt"); + fakeFolder.remoteModifier().mkdir("root/stable/move"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.remoteModifier().rename("root", "root test"); + fakeFolder.remoteModifier().rename("root test/stable/folder to move", "root test/stable/folder"); + fakeFolder.remoteModifier().rename("root test/stable/folder", "root test/stable/move/folder"); + OperationCounter counter; + fakeFolder.setServerOverride(counter.functor()); + + connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToPropagate, [&](SyncFileItemVector &items) { + SyncFileItemPtr root, stable, folder, file, move; + for (auto &item : items) { + qDebug() << item->_file; + if (item->_file == "root") { + root = item; + } else if (item->_file == "root test/stable") { + stable = item; + } else if (item->_file == "root test/stable/move") { + move = item; + } else if (item->_file == "root/stable/folder to move") { + folder = item; + } else if (item->_file == "root test/stable/move/folder/index.txt") { + file = item; + } + } + + QVERIFY(root); + QCOMPARE(root->_instruction, CSYNC_INSTRUCTION_RENAME); + QCOMPARE(root->_direction, SyncFileItem::Down); + + QVERIFY(stable); + QCOMPARE(stable->_instruction, CSYNC_INSTRUCTION_RENAME); + QCOMPARE(stable->_direction, SyncFileItem::Down); + + QVERIFY(move); + QCOMPARE(move->_instruction, CSYNC_INSTRUCTION_RENAME); + QCOMPARE(move->_direction, SyncFileItem::Down); + + QVERIFY(folder); + QCOMPARE(folder->_instruction, CSYNC_INSTRUCTION_RENAME); + QCOMPARE(folder->_direction, SyncFileItem::Down); + + QVERIFY(file); + QCOMPARE(file->_instruction, CSYNC_INSTRUCTION_RENAME); + QCOMPARE(file->_direction, SyncFileItem::Down); + }); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(counter.nDELETE, 0); + QCOMPARE(counter.nGET, 0); + QCOMPARE(counter.nPUT, 0); + QCOMPARE(counter.nMOVE, 0); + QCOMPARE(counter.nMKCOL, 0); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } + + void testMultipleRenameFromLocal() + { + FakeFolder fakeFolder{{}}; + + fakeFolder.remoteModifier().mkdir("root"); + fakeFolder.remoteModifier().mkdir("root/stable"); + fakeFolder.remoteModifier().mkdir("root/stable/folder to move"); + fakeFolder.remoteModifier().insert("root/stable/folder to move/index.txt"); + fakeFolder.remoteModifier().mkdir("root/stable/move"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.localModifier().rename("root", "root test"); + fakeFolder.localModifier().rename("root test/stable/folder to move", "root test/stable/folder"); + fakeFolder.localModifier().rename("root test/stable/folder", "root test/stable/move/folder"); + OperationCounter counter; + fakeFolder.setServerOverride(counter.functor()); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(counter.nDELETE, 0); + QCOMPARE(counter.nGET, 0); + QCOMPARE(counter.nPUT, 0); + QCOMPARE(counter.nMOVE, 2); + QCOMPARE(counter.nMKCOL, 0); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } }; QTEST_GUILESS_MAIN(TestSyncMove) From a0982ca6d4fbf5cfd3addba79d5f2be1abcbf040 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 11 Sep 2024 17:14:11 +0200 Subject: [PATCH 2/3] avoid wrong tracking of renamed folders during discovery ensure server rename operations are tracked as server rename operations ensure local rename are tracked as local rename operations Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 468d1fae467c2..ada2e8a0023e6 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1727,9 +1727,11 @@ void ProcessDirectoryJob::processFileFinalize( ASSERT(_dirItem && _dirItem->_instruction == CSYNC_INSTRUCTION_RENAME); // This is because otherwise subitems are not updated! (ideally renaming a directory could // update the database for all items! See PropagateDirectory::slotSubJobsFinished) - const auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(path._original, SyncFileItem::Down); - Q_UNUSED(adjustedOriginalPath) - _discoveryData->_renamedItemsLocal.insert(path._original, path._target); + if (_dirItem->_direction == SyncFileItem::Direction::Down) { + _discoveryData->_renamedItemsRemote.insert(path._original, path._target); + } else { + _discoveryData->_renamedItemsLocal.insert(path._original, path._target); + } item->_instruction = CSYNC_INSTRUCTION_RENAME; item->_renameTarget = path._target; item->_direction = _dirItem->_direction; From 2f48389ed071e64a1d6c9ac22171aeb02cceea2e Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 11 Sep 2024 17:16:08 +0200 Subject: [PATCH 3/3] improve some log that are produced during discovery would be usefull to track bugs during rename operations Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index ada2e8a0023e6..a938088cc803a 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -64,7 +64,7 @@ ProcessDirectoryJob::ProcessDirectoryJob(const PathTuple &path, const SyncFileIt , _discoveryData(parent->_discoveryData) , _currentFolder(path) { - qCDebug(lcDisco) << path._server << queryServer << path._local << queryLocal << lastSyncTimestamp; + qCDebug(lcDisco) << "PREPARING" << _currentFolder._server << _queryServer << _currentFolder._local << _queryLocal; computePinState(parent->_pinState); } @@ -77,6 +77,7 @@ ProcessDirectoryJob::ProcessDirectoryJob(DiscoveryPhase *data, PinState basePinS , _discoveryData(data) , _currentFolder(path) { + qCDebug(lcDisco) << "PREPARING" << _currentFolder._server << _queryServer << _currentFolder._local << _queryLocal; computePinState(basePinState); } @@ -532,6 +533,7 @@ void ProcessDirectoryJob::processFile(PathTuple path, << " | checksum: " << dbEntry._checksumHeader << "//" << serverEntry.checksumHeader << " | perm: " << dbEntry._remotePerm << "//" << serverEntry.remotePerm << " | fileid: " << dbEntry._fileId << "//" << serverEntry.fileId + << " | inode: " << dbEntry._inode << "/" << localEntry.inode << "/" << " | type: " << dbEntry._type << "/" << localEntry.type << "/" << (serverEntry.isDirectory ? ItemTypeDirectory : ItemTypeFile) << " | e2ee: " << dbEntry.isE2eEncrypted() << "/" << serverEntry.isE2eEncrypted() << " | e2eeMangledName: " << dbEntry.e2eMangledName() << "/" << serverEntry.e2eMangledName @@ -1738,13 +1740,12 @@ void ProcessDirectoryJob::processFileFinalize( } { - const auto discoveredItemLog = QStringLiteral("%1 %2 %3 %4").arg(item->_file).arg(item->_instruction).arg(item->_direction).arg(item->_type); const auto isImportantInstruction = item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_IGNORE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA; if (isImportantInstruction) { - qCInfo(lcDisco) << discoveredItemLog; + qCInfo(lcDisco) << "discovered" << item->_file << item->_instruction << item->_direction << item->_type; } else { - qCDebug(lcDisco) << discoveredItemLog; + qCDebug(lcDisco) << "discovered" << item->_file << item->_instruction << item->_direction << item->_type; } }