Merge pull request #7334 from nextcloud/bugfix/useFileIdToTrackRename…

…dItems Bugfix/use file id to track renamed items
nextcloud · Oct 17, 2024 · b59c7b4 · b59c7b4
2 parents fe47ca0 + 05b6db5
commit b59c7b4
Show file tree

Hide file tree

Showing 5 changed files with 9 additions and 55 deletions.
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -912,9 +912,8 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(const SyncFileItemPtr &it
             done = true;
             return;
         }
-        if (!serverEntry.isDirectory && base._etag != serverEntry.etag) {
-            /* File with different etag, don't do a rename, but download the file again */
-            qCInfo(lcDisco, "file etag different, not a rename");
+        if (!serverEntry.isDirectory && (base._modtime != serverEntry.modtime || base._fileSize != serverEntry.size)) {
+            qCInfo(lcDisco, "file metadata different, forcing a download of the new file");
             done = true;
             return;
         }
@@ -1595,7 +1594,7 @@ void ProcessDirectoryJob::processFileConflict(const SyncFileItemPtr &item, Proce
     // If there's no content hash, use heuristics
     if (serverEntry.checksumHeader.isEmpty()) {
         // If the size or mtime is different, it's definitely a conflict.
-        bool isConflict = (serverEntry.size != localEntry.size) || (serverEntry.modtime != localEntry.modtime);
+        bool isConflict = (serverEntry.size != localEntry.size) || (serverEntry.modtime != localEntry.modtime) || (dbEntry.isValid() && dbEntry._modtime != localEntry.modtime && serverEntry.modtime == localEntry.modtime);
 
         // It could be a conflict even if size and mtime match!
         //

diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp
@@ -178,6 +178,7 @@ void FileInfo::setContents(const QString &relativePath, char contentChar)
     FileInfo *file = findInvalidatingEtags(relativePath);
     Q_ASSERT(file);
     file->contentChar = contentChar;
+    file->lastModified = QDateTime::currentDateTimeUtc();
 }
 
 void FileInfo::appendByte(const QString &relativePath)

diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp
@@ -328,35 +328,22 @@ private slots:
         fakeFolder.localModifier().insert("A/a0", size);
         QVERIFY(!fakeFolder.syncOnce()); // error: abort!
 
-        // Now the next sync gets a NEW/NEW conflict and since there's no checksum
-        // it just becomes a UPDATE_METADATA
-        auto checkEtagUpdated = [&](SyncFileItemVector &items) {
-            QCOMPARE(items.size(), 1);
-            QCOMPARE(items[0]->_file, QLatin1String("A"));
-            SyncJournalFileRecord record;
-            QVERIFY(fakeFolder.syncJournal().getFileRecord(QByteArray("A/a0"), &record));
-            QCOMPARE(record._etag, fakeFolder.remoteModifier().find("A/a0")->etag);
-        };
-        auto connection = connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToPropagate, checkEtagUpdated);
         QVERIFY(fakeFolder.syncOnce());
-        disconnect(connection);
         QCOMPARE(nGET, 0);
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-
         // Test 2: modified file upload aborted
+        nGET = 0;
         fakeFolder.localModifier().appendByte("A/a0");
         QVERIFY(!fakeFolder.syncOnce()); // error: abort!
 
         // An EVAL/EVAL conflict is also UPDATE_METADATA when there's no checksums
-        connection = connect(&fakeFolder.syncEngine(), &SyncEngine::aboutToPropagate, checkEtagUpdated);
         QVERIFY(fakeFolder.syncOnce());
-        disconnect(connection);
-        QCOMPARE(nGET, 0);
+        QCOMPARE(nGET, 1);
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-
         // Test 3: modified file upload aborted, with good checksums
+        nGET = 0;
         fakeFolder.localModifier().appendByte("A/a0");
         QVERIFY(!fakeFolder.syncOnce()); // error: abort!
 
@@ -365,12 +352,11 @@ private slots:
         fakeFolder.remoteModifier().find("A/a0")->checksums = moveChecksumHeader;
 
         QVERIFY(fakeFolder.syncOnce());
-        disconnect(connection);
         QCOMPARE(nGET, 0); // no new download, just a metadata update!
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
 
-
         // Test 4: New file, that gets deleted locally before the next sync
+        nGET = 0;
         fakeFolder.localModifier().insert("A/a3", size);
         QVERIFY(!fakeFolder.syncOnce()); // error: abort!
         fakeFolder.localModifier().remove("A/a3");

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -432,7 +432,7 @@ private slots:
 
         QTest::newRow("Same mtime, but no server checksum -> ignored in reconcile")
             << true << QByteArray()
-            << 0;
+            << 1;
 
         QTest::newRow("Same mtime, weak server checksum differ -> downloaded")
             << true << QByteArray("Adler32:bad")

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
@@ -741,38 +741,6 @@ private slots:
         }
     }
 
-    // https://github.com/owncloud/client/issues/6629#issuecomment-402450691
-    // When a file is moved and the server mtime was not in sync, the local mtime should be kept
-    void testMoveAndMTimeChange()
-    {
-        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
-        OperationCounter counter;
-        fakeFolder.setServerOverride(counter.functor());
-
-        // Changing the mtime on the server (without invalidating the etag)
-        fakeFolder.remoteModifier().find("A/a1")->lastModified = QDateTime::currentDateTimeUtc().addSecs(-50000);
-        fakeFolder.remoteModifier().find("A/a2")->lastModified = QDateTime::currentDateTimeUtc().addSecs(-40000);
-
-        // Move a few files
-        fakeFolder.remoteModifier().rename("A/a1", "A/a1_server_renamed");
-        fakeFolder.localModifier().rename("A/a2", "A/a2_local_renamed");
-
-        QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(counter.nGET, 0);
-        QCOMPARE(counter.nPUT, 0);
-        QCOMPARE(counter.nMOVE, 1);
-        QCOMPARE(counter.nDELETE, 0);
-
-        // Another sync should do nothing
-        QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(counter.nGET, 0);
-        QCOMPARE(counter.nPUT, 0);
-        QCOMPARE(counter.nMOVE, 1);
-        QCOMPARE(counter.nDELETE, 0);
-
-        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-    }
-
     // Test for https://github.com/owncloud/client/issues/6694
     void testInvertFolderHierarchy()
     {