Merge pull request #5872 from nextcloud/work/do-not-mod-new-files-unn…

…ecessarily Do not modify discovered files on disk if not necessary
nextcloud · Aug 7, 2023 · f529dd6 · f529dd6
2 parents 0a86967 + 812e696
commit f529dd6
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 8 deletions.
diff --git 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
@@ -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

diff --git 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; // Decides whether or not we modify file metadata
             QString filePath = _localPath + item->_file;
 
             // If the 'W' remote permission changed, update the local filesystem
@@ -365,15 +366,18 @@ 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);
             }
+
+            modificationHappened |= item->_size != prev._fileSize;
+
             auto rec = item->toSyncJournalFileRecordWithInode(filePath);
             if (rec._checksumHeader.isEmpty())
                 rec._checksumHeader = prev._checksumHeader;
             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;
@@ -382,10 +386,11 @@ 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) {
+            if (modificationHappened && item->_type == ItemTypeVirtualFile) {
                 auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId);
                 if (!r) {
                     item->_status = SyncFileItem::Status::NormalError;
@@ -394,7 +399,7 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item)
                     emit itemCompleted(item, ErrorCategory::GenericError);
                     return;
                 }
-            } else {
+            } 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);

diff --git 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
@@ -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

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -12,6 +12,7 @@
 #include "propagatorjobs.h"
 #include "syncengine.h"
 
+#include <QFile>
 #include <QtTest>
 
 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<DiskFileModifier &>(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
      */