From 1fb4c22adf2475fd9652e299484041d15c759404 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Sun, 23 Dec 2018 11:30:02 +0100 Subject: [PATCH] Move: add more test and fix move within moves --- src/libsync/discovery.cpp | 18 +++++++-------- src/libsync/discoveryphase.cpp | 5 ++-- src/libsync/discoveryphase.h | 7 ++++-- test/testsyncmove.cpp | 42 ++++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 9aadc4307..3c74e6a08 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -282,7 +282,7 @@ void ProcessDirectoryJob::processFile(PathTuple path, << " | fileid: " << dbEntry._fileId << "//" << serverEntry.fileId << " | inode: " << dbEntry._inode << "/" << localEntry.inode << "/"; - if (_discoveryData->_renamedItems.contains(path._original)) { + if (_discoveryData->isRenamed(path._original)) { qCDebug(lcDisco) << "Ignoring renamed"; return; // Ignore this. } @@ -489,7 +489,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( // Now we know there is a sane rename candidate. QString originalPath = QString::fromUtf8(base._path); - if (_discoveryData->_renamedItems.contains(originalPath)) { + if (_discoveryData->isRenamed(originalPath)) { qCInfo(lcDisco, "folder already has a rename entry, skipping"); return; } @@ -530,8 +530,8 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( bool wasDeletedOnServer = _discoveryData->findAndCancelDeletedJob(originalPath).first; auto postProcessRename = [this, item, base, originalPath](PathTuple &path) { - auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath); - _discoveryData->_renamedItems.insert(originalPath, path._target); + auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Up); + _discoveryData->_renamedItemsRemote.insert(originalPath, path._target); item->_modtime = base._modtime; item->_inode = base._inode; item->_instruction = CSYNC_INSTRUCTION_RENAME; @@ -556,7 +556,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs); if (etag || etag.error().code != 404 || // Somehow another item claimed this original path, consider as if it existed - _discoveryData->_renamedItems.contains(originalPath)) { + _discoveryData->isRenamed(originalPath)) { // If the file exist or if there is another error, consider it is a new file. postProcessServerNew(); return; @@ -818,7 +818,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( } } auto originalPath = QString::fromUtf8(base._path); - if (isMove && _discoveryData->_renamedItems.contains(originalPath)) + if (isMove && _discoveryData->isRenamed(originalPath)) isMove = false; //Check local permission if we are allowed to put move the file here @@ -833,8 +833,8 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); auto processRename = [item, originalPath, base, this](PathTuple &path) { - auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath); - _discoveryData->_renamedItems.insert(originalPath, path._target); + auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down); + _discoveryData->_renamedItemsLocal.insert(originalPath, path._target); item->_renameTarget = path._target; path._server = adjustedOriginalPath; item->_file = path._server; @@ -861,7 +861,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( chopVirtualFileSuffix(serverOriginalPath); auto job = new RequestEtagJob(_discoveryData->_account, serverOriginalPath, this); connect(job, &RequestEtagJob::finishedWithResult, this, [=](const HttpResult &etag) mutable { - if (!etag || (*etag != base._etag && !item->isDirectory()) || _discoveryData->_renamedItems.contains(originalPath)) { + if (!etag || (*etag != base._etag && !item->isDirectory()) || _discoveryData->isRenamed(originalPath)) { qCInfo(lcDisco) << "Can't rename because the etag has changed or the directory is gone" << originalPath; // Can't be a rename, leave it as a new. postProcessLocalNew(); diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index f8ea51d29..bb80a9a53 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -132,11 +132,10 @@ void DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePerm propfindJob->start(); } - /* Given a path on the remote, give the path as it is when the rename is done */ -QString DiscoveryPhase::adjustRenamedPath(const QString &original) const +QString DiscoveryPhase::adjustRenamedPath(const QString &original, SyncFileItem::Direction d) const { - return OCC::adjustRenamedPath(_renamedItems, original); + return OCC::adjustRenamedPath(d == SyncFileItem::Down ? _renamedItemsRemote : _renamedItemsLocal, original); } QString adjustRenamedPath(const QMap renamedItems, const QString original) diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index cebc4181e..8e90fbb75 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -132,7 +132,10 @@ class DiscoveryPhase : public QObject friend class ProcessDirectoryJob; QMap _deletedItem; QMap _queuedDeletedDirectories; - QMap _renamedItems; // map source -> destinations + // map source -> destinations + QMap _renamedItemsRemote; + QMap _renamedItemsLocal; + bool isRenamed(const QString &p) { return _renamedItemsLocal.contains(p) || _renamedItemsRemote.contains(p); } int _currentlyActiveJobs = 0; // both must contain a sorted list @@ -153,7 +156,7 @@ class DiscoveryPhase : public QObject * Note that it only considers parent directory renames. So if A/B got renamed to C/D, * checking A/B/file would yield C/D/file, but checking A/B would yield A/B. */ - QString adjustRenamedPath(const QString &original) const; + QString adjustRenamedPath(const QString &original, SyncFileItem::Direction) const; /** * Check if there is already a job to delete that item. diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index adfc74255..dee9ac0b6 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -670,6 +670,48 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), expectedState); QCOMPARE(fakeFolder.currentRemoteState(), expectedState); } + + void testDeepHierarchy_data() + { + QTest::addColumn("local"); + QTest::newRow("remote") << false; + QTest::newRow("local") << true; + } + + void testDeepHierarchy() + { + QFETCH(bool, local); + FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() }; + auto &modifier = local ? fakeFolder.localModifier() : fakeFolder.remoteModifier(); + + modifier.mkdir("FolA"); + modifier.mkdir("FolA/FolB"); + modifier.mkdir("FolA/FolB/FolC"); + modifier.mkdir("FolA/FolB/FolC/FolD"); + modifier.mkdir("FolA/FolB/FolC/FolD/FolE"); + modifier.insert("FolA/FileA.txt"); + modifier.insert("FolA/FolB/FileB.txt"); + modifier.insert("FolA/FolB/FolC/FileC.txt"); + modifier.insert("FolA/FolB/FolC/FolD/FileD.txt"); + modifier.insert("FolA/FolB/FolC/FolD/FolE/FileE.txt"); + QVERIFY(fakeFolder.syncOnce()); + + modifier.insert("FolA/FileA2.txt"); + modifier.insert("FolA/FolB/FileB2.txt"); + modifier.insert("FolA/FolB/FolC/FileC2.txt"); + modifier.insert("FolA/FolB/FolC/FolD/FileD2.txt"); + modifier.insert("FolA/FolB/FolC/FolD/FolE/FileE2.txt"); + modifier.rename("FolA", "FolA_Renamed"); + modifier.rename("FolA_Renamed/FolB", "FolB_Renamed"); + modifier.rename("FolB_Renamed/FolC", "FolA"); + modifier.rename("FolA/FolD", "FolA/FolD_Renamed"); + modifier.mkdir("FolB_Renamed/New"); + modifier.rename("FolA/FolD_Renamed/FolE", "FolB_Renamed/New/FolE"); + auto expected = local ? fakeFolder.currentLocalState() : fakeFolder.currentRemoteState(); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), expected); + QCOMPARE(fakeFolder.currentRemoteState(), expected); + } }; QTEST_GUILESS_MAIN(TestSyncMove)