From 584ffe6307413e09aa64d9b006013477040d5548 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 7 Jul 2023 19:11:59 +0800 Subject: [PATCH 1/8] Unify else and if in SyncEngine::slotItemDiscovered for filesystem setmodtime check Signed-off-by: Claudio Cambra --- src/libsync/syncengine.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 7a13f7133f30b..e9f2fb9ec729f 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -394,13 +394,11 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) emit itemCompleted(item, ErrorCategory::GenericError); return; } - } else { - if (!FileSystem::setModTime(filePath, item->_modtime)) { - item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_errorString = tr("Could not update file metadata: %1").arg(filePath); - emit itemCompleted(item, ErrorCategory::GenericError); - return; - } + } else if (!FileSystem::setModTime(filePath, item->_modtime)) { + item->_instruction = CSYNC_INSTRUCTION_ERROR; + item->_errorString = tr("Could not update file metadata: %1").arg(filePath); + emit itemCompleted(item, ErrorCategory::GenericError); + return; } // Updating the db happens on success From 3fd8e2037b22c93c004e8a53fc33ee112265116d Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 7 Jul 2023 19:14:09 +0800 Subject: [PATCH 2/8] Return bool from FileSystem::setFileReadOnlyWeak depending on whether permission change was actually made or not Signed-off-by: Claudio Cambra --- src/common/filesystembase.cpp | 6 +++--- src/common/filesystembase.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 26d6b9f3121a7..54a4832a732a6 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -114,17 +114,17 @@ void FileSystem::setFolderMinimumPermissions(const QString &filename) #endif } - -void FileSystem::setFileReadOnlyWeak(const QString &filename, bool readonly) +bool FileSystem::setFileReadOnlyWeak(const QString &filename, bool readonly) { QFile file(filename); QFile::Permissions permissions = file.permissions(); if (!readonly && (permissions & QFile::WriteOwner)) { - return; // already writable enough + return false; // already writable enough } setFileReadOnly(filename, readonly); + return true; } bool FileSystem::rename(const QString &originFileName, diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index bc0b592c23aff..fd0572804dcb2 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -65,7 +65,7 @@ namespace FileSystem { * This means that it will preserve explicitly set rw-r--r-- permissions even * when the umask is 0002. (setFileReadOnly() would adjust to rw-rw-r--) */ - void OCSYNC_EXPORT setFileReadOnlyWeak(const QString &filename, bool readonly); + bool OCSYNC_EXPORT setFileReadOnlyWeak(const QString &filename, bool readonly); /** * @brief Try to set permissions so that other users on the local machine can not From 4e5f9e5f08569f1934d80daeb82713b490802583 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 7 Jul 2023 19:15:03 +0800 Subject: [PATCH 3/8] Only change modification time if any modifications were actually made on downsynced file Signed-off-by: Claudio Cambra --- src/libsync/syncengine.cpp | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index e9f2fb9ec729f..e228d604abe88 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -357,6 +357,7 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) // mini-jobs later on, we just update metadata right now. if (item->_direction == SyncFileItem::Down) { + auto modificationHappened = false; QString filePath = _localPath + item->_file; // If the 'W' remote permission changed, update the local filesystem @@ -365,8 +366,9 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) && prev.isValid() && prev._remotePerm.hasPermission(RemotePermissions::CanWrite) != item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) { const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite); - FileSystem::setFileReadOnlyWeak(filePath, isReadOnly); + modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly); } + auto rec = item->toSyncJournalFileRecordWithInode(filePath); if (rec._checksumHeader.isEmpty()) rec._checksumHeader = prev._checksumHeader; @@ -382,23 +384,26 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) emit itemCompleted(item, ErrorCategory::GenericError); return; } + modificationHappened = true; } // Update on-disk virtual file metadata - if (item->_type == ItemTypeVirtualFile) { - auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId); - if (!r) { - item->_status = SyncFileItem::Status::NormalError; + if (modificationHappened) { + if (item->_type == ItemTypeVirtualFile) { + auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId); + if (!r) { + item->_status = SyncFileItem::Status::NormalError; + item->_instruction = CSYNC_INSTRUCTION_ERROR; + item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error()); + emit itemCompleted(item, ErrorCategory::GenericError); + return; + } + } else if (!FileSystem::setModTime(filePath, item->_modtime)) { item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error()); + item->_errorString = tr("Could not update file metadata: %1").arg(filePath); emit itemCompleted(item, ErrorCategory::GenericError); return; } - } else if (!FileSystem::setModTime(filePath, item->_modtime)) { - item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_errorString = tr("Could not update file metadata: %1").arg(filePath); - emit itemCompleted(item, ErrorCategory::GenericError); - return; } // Updating the db happens on success From 568f9e50bbac4c16eda1753f94c537d63ee64850 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Fri, 7 Jul 2023 19:15:54 +0800 Subject: [PATCH 4/8] Only attempt conversion to placeholder on new file if VFS enabled in SyncEngine::slotItemDiscovered Signed-off-by: Claudio Cambra --- src/libsync/syncengine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index e228d604abe88..c36748934ca43 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -375,7 +375,7 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) rec._serverHasIgnoredFiles |= prev._serverHasIgnoredFiles; // Ensure it's a placeholder file on disk - if (item->_type == ItemTypeFile) { + if (item->_type == ItemTypeFile && _syncOptions._vfs->mode() != Vfs::Off) { const auto result = _syncOptions._vfs->convertToPlaceholder(filePath, *item); if (!result) { item->_status = SyncFileItem::Status::NormalError; From d8390b2bc3092c63cea0ff5d49a365ba5cbba9ca Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Mon, 10 Jul 2023 13:00:28 +0800 Subject: [PATCH 5/8] Ensure file modification time is set if item modtime differs from journal modtime Signed-off-by: Claudio Cambra --- src/libsync/syncengine.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index c36748934ca43..0836126ca4cdb 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -388,17 +388,17 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) } // Update on-disk virtual file metadata - if (modificationHappened) { - if (item->_type == ItemTypeVirtualFile) { - auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId); - if (!r) { - item->_status = SyncFileItem::Status::NormalError; - item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error()); - emit itemCompleted(item, ErrorCategory::GenericError); - return; - } - } else if (!FileSystem::setModTime(filePath, item->_modtime)) { + if (modificationHappened && item->_type == ItemTypeVirtualFile) { + auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId); + if (!r) { + item->_status = SyncFileItem::Status::NormalError; + item->_instruction = CSYNC_INSTRUCTION_ERROR; + item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error()); + emit itemCompleted(item, ErrorCategory::GenericError); + return; + } + } else if (modificationHappened || prev._modtime != item->_modtime) { + if (!FileSystem::setModTime(filePath, item->_modtime)) { item->_instruction = CSYNC_INSTRUCTION_ERROR; item->_errorString = tr("Could not update file metadata: %1").arg(filePath); emit itemCompleted(item, ErrorCategory::GenericError); From e94407ac4661b398833756b5cebf31ff59fa77b0 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Tue, 11 Jul 2023 15:37:29 +0800 Subject: [PATCH 6/8] Add find method to DiskFileModifier class in SyncEngineTestUtils Signed-off-by: Claudio Cambra --- test/syncenginetestutils.cpp | 6 ++++++ test/syncenginetestutils.h | 2 ++ 2 files changed, 8 insertions(+) diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp index 15c7f5a489245..b095e5a10ca0b 100644 --- a/test/syncenginetestutils.cpp +++ b/test/syncenginetestutils.cpp @@ -112,6 +112,12 @@ void DiskFileModifier::setE2EE([[maybe_unused]] const QString &relativePath, [[m { } +QFile DiskFileModifier::find(const QString &relativePath) const +{ + const auto path = _rootDir.filePath(relativePath); + return QFile(path); +} + FileInfo FileInfo::A12_B12_C12_S12() { FileInfo fi { QString {}, { diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index 4f9c552ba40b0..3a299f3ac927e 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -108,6 +108,8 @@ class DiskFileModifier : public FileModifier void setModTime(const QString &relativePath, const QDateTime &modTime) override; void modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout) override; void setE2EE(const QString &relativepath, const bool enabled) override; + + [[nodiscard]] QFile find(const QString &relativePath) const; }; class FileInfo : public FileModifier From 09d7db48058b5db411ef158c022a04943c3be7ca Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Thu, 13 Jul 2023 21:04:07 +0800 Subject: [PATCH 7/8] Add test for unmodified local files not getting mtimes changed upon upload to server Signed-off-by: Claudio Cambra --- test/testsyncengine.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index ec5aebeb24892..734f80af0889c 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -11,6 +11,7 @@ #include "propagatorjobs.h" #include "caseclashconflictsolver.h" +#include #include using namespace OCC; @@ -920,6 +921,29 @@ private slots: QCOMPARE(QFileInfo(fakeFolder.localPath() + "foo").lastModified(), datetime); } + // A local file should not be modified after upload to server if nothing has changed. + void testLocalFileInitialMtime() + { + constexpr auto fooFolder = "foo/"; + constexpr auto barFile = "foo/bar"; + + FakeFolder fakeFolder{FileInfo{}}; + fakeFolder.localModifier().mkdir(fooFolder); + fakeFolder.localModifier().insert(barFile); + + const auto localDiskFileModifier = dynamic_cast(fakeFolder.localModifier()); + + const auto localFile = localDiskFileModifier.find(barFile); + const auto localFileInfo = QFileInfo(localFile); + const auto expectedMtime = localFileInfo.metadataChangeTime(); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + const auto currentMtime = localFileInfo.metadataChangeTime(); + QCOMPARE(currentMtime, expectedMtime); + } + /** * Checks whether subsequent large uploads are skipped after a 507 error */ From 832f0f8764b756d56b391e673fe60e878fbcc663 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Mon, 7 Aug 2023 08:24:43 +0800 Subject: [PATCH 8/8] Set modificationHappened to also depend on file size change Signed-off-by: Claudio Cambra --- src/libsync/syncengine.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 0836126ca4cdb..f583bb2ad60f8 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -357,7 +357,7 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) // mini-jobs later on, we just update metadata right now. if (item->_direction == SyncFileItem::Down) { - auto modificationHappened = false; + auto modificationHappened = false; // Decides whether or not we modify file metadata QString filePath = _localPath + item->_file; // If the 'W' remote permission changed, update the local filesystem @@ -369,6 +369,8 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly); } + modificationHappened |= item->_size != prev._fileSize; + auto rec = item->toSyncJournalFileRecordWithInode(filePath); if (rec._checksumHeader.isEmpty()) rec._checksumHeader = prev._checksumHeader;