From 32145342c8ba4ce25a7a321d7c62199d34d11770 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 52985fd4454a..313f0597e852 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 9811d35a96c1815674e1deb9e44deed5baafd2ed 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 89c642692193..ef9ba1efd677 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 bc0b592c23af..fd0572804dcb 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 fe829e2693843e34854f6d26aa2c76ab35739650 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 313f0597e852..33156e909a32 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 961033efc181202a21aa92b45f3d0c8e0cfa0c51 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 33156e909a32..56af0770f628 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 4681d27de553958c7927d84b7ba8bfb4a0a3eb53 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 56af0770f628..a999c8a0edbf 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 67e6546e1ad4583313547c7718fc9323a3a4b08c 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 82aa701664d1..88ece82c1e7e 100644 --- a/test/syncenginetestutils.cpp +++ b/test/syncenginetestutils.cpp @@ -113,6 +113,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 4f9c552ba40b..3a299f3ac927 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 1cd537f928f2dc0fb7448f066edf7fb974f01905 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 9516a05570dc..f4c4c023664e 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -12,6 +12,7 @@ #include "propagatorjobs.h" #include "syncengine.h" +#include #include using namespace OCC; @@ -921,6 +922,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 812e696d53ef14c326e2312d7007ff40a3d23b3b 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 a999c8a0edbf..656689a550e6 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;