From 6b467acf38508df979852c5b60de044d72d93f58 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Thu, 28 Feb 2019 14:33:37 +0100 Subject: [PATCH] Download: Remove useless code and add a test From issue #7015, the code is wrong because the path is the file system path and not the path on the DB. But since this is a conflict, this means the reconcile will still want to download the file from the server next sync, so we need not to worry about this case --- src/libsync/owncloudpropagator.h | 2 +- src/libsync/propagatedownload.cpp | 13 ------- src/libsync/syncengine.h | 2 + test/testdownload.cpp | 61 +++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index cf84e2447ba..3005a847b97 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -357,7 +357,7 @@ class PropagateIgnoreJob : public PropagateItemJob } }; -class OwncloudPropagator : public QObject +class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject { Q_OBJECT public: diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 81e18beaaa1..60c43ef88fd 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1004,19 +1004,6 @@ void PropagateDownloadFile::downloadFinished() // The fileChanged() check is done above to generate better error messages. if (!FileSystem::uncheckedRenameReplace(_tmpFile.fileName(), fn, &error)) { qCWarning(lcPropagateDownload) << QString("Rename failed: %1 => %2").arg(_tmpFile.fileName()).arg(fn); - - // If we moved away the original file due to a conflict but can't - // put the downloaded file in its place, we are in a bad spot: - // If we do nothing the next sync run will assume the user deleted - // the file! - // To avoid that, the file is removed from the metadata table entirely - // which makes it look like we're just about to initially download - // it. - if (isConflict) { - propagator()->_journal->deleteFileRecord(fn); - propagator()->_journal->commit("download finished"); - } - // If the file is locked, we want to retry this sync when it // becomes available again, otherwise try again directly if (FileSystem::isFileLocked(fn)) { diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index 83fa92fa4f8..264146474f2 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -134,6 +134,8 @@ class OWNCLOUDSYNC_EXPORT SyncEngine : public QObject */ static void wipeVirtualFiles(const QString &localPath, SyncJournalDb &journal, Vfs &vfs); + auto getPropagator() { return _propagator; } // for the test + signals: // During update, before reconcile void rootEtag(QString); diff --git a/test/testdownload.cpp b/test/testdownload.cpp index fab2de91768..d8ce6773efb 100644 --- a/test/testdownload.cpp +++ b/test/testdownload.cpp @@ -8,6 +8,7 @@ #include #include "syncenginetestutils.h" #include +#include using namespace OCC; @@ -145,6 +146,66 @@ private slots: QCOMPARE(getItem(completeSpy, "A/broken")->_status, SyncFileItem::FatalError); QVERIFY(getItem(completeSpy, "A/broken")->_errorString.contains("System in maintenance mode")); } + + void testMoveFailsInAConflict() { +#ifdef Q_OS_WIN + QSKIP("Not run on windows because permission on directory does not do what is expected"); +#endif + // Test for https://github.com/owncloud/client/issues/7015 + // We want to test the case in which the renaming of the original to the conflict file succeeds, + // but renaming the temporary file fails. + // This tests uses the fact that a "touchedFile" notification will be sent at the right moment. + // Note that there will be first a notification on the file and the conflict file before. + + FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; + fakeFolder.syncEngine().setIgnoreHiddenFiles(true); + fakeFolder.remoteModifier().setContents("A/a1", 'A'); + fakeFolder.localModifier().setContents("A/a1", 'B'); + + bool propConnected = false; + QString conflictFile; + auto transProgress = connect(&fakeFolder.syncEngine(), &SyncEngine::transmissionProgress, + [&](const ProgressInfo &pi) { + auto propagator = fakeFolder.syncEngine().getPropagator(); + if (pi.status() != ProgressInfo::Propagation || propConnected || !propagator) + return; + propConnected = true; + connect(propagator.data(), &OwncloudPropagator::touchedFile, [&](const QString &s) { + if (s.contains("conflicted copy")) { + QCOMPARE(conflictFile, QString()); + conflictFile = s; + return; + } + if (!conflictFile.isEmpty()) { + // Check that the temporary file is still there + QCOMPARE(QDir(fakeFolder.localPath() + "A/").entryList({"*.~*"}, QDir::Files | QDir::Hidden).count(), 1); + // Set the permission to read only on the folder, so the rename of the temporary file will fail + QFile(fakeFolder.localPath() + "A/").setPermissions(QFile::Permissions(0x5555)); + } + }); + }); + + QVERIFY(!fakeFolder.syncOnce()); // The sync must fail because the rename failed + QVERIFY(!conflictFile.isEmpty()); + + // restore permissions + QFile(fakeFolder.localPath() + "A/").setPermissions(QFile::Permissions(0x7777)); + + QObject::disconnect(transProgress); + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &, QIODevice *) -> QNetworkReply * { + if (op == QNetworkAccessManager::GetOperation) + QTest::qFail("There shouldn't be any download", __FILE__, __LINE__); + return nullptr; + }); + QVERIFY(fakeFolder.syncOnce()); + + // The a1 file is still tere and have the right content + QVERIFY(fakeFolder.currentRemoteState().find("A/a1")); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a1")->contentChar, 'A'); + + QVERIFY(QFile::remove(conflictFile)); // So the comparison succeeds; + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } }; QTEST_GUILESS_MAIN(TestDownload)