ensure we emit a rename command for renamed files

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>

src/libsync/discovery.cpp

@@ -41,7 +41,7 @@ Q_LOGGING_CATEGORY(lcDisco, "sync.discovery", QtInfoMsg)
 bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
     const std::map<QString, Entries> &entries, Entries &entry)
 {
-    const auto originalFileName = entry.localEntry.name;
+    const auto originalFileName = entry.localEntry.isValid() ? entry.localEntry.name : entry.serverEntry.name;
     const auto newFileName = originalFileName.trimmed();
 
     if (originalFileName == newFileName) {
@@ -61,7 +61,7 @@ bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
 
         if (!errorMessage.isEmpty()) {
             auto item = SyncFileItemPtr::create();
-            if (entry.localEntry.isDirectory) {
+            if ((entry.localEntry.isValid() && entry.localEntry.isDirectory) || (entry.serverEntry.isValid() && entry.serverEntry.isDirectory)) {
                 item->_type = CSyncEnums::ItemTypeDirectory;
             } else {
                 item->_type = CSyncEnums::ItemTypeFile;
@@ -76,7 +76,11 @@ bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
         }
     }
 
-    entry.localEntry.renameName = newFileName;
+    if (entry.localEntry.isValid()) {
+        entry.localEntry.renameName = newFileName;
+    } else {
+        entry.serverEntry.renameName = newFileName;
+    }
 
     return true;
 }
@@ -389,13 +393,6 @@ void ProcessDirectoryJob::processFile(PathTuple path,
     item->_originalFile = path._original;
     item->_previousSize = dbEntry._fileSize;
     item->_previousModtime = dbEntry._modtime;
-    if (!localEntry.renameName.isEmpty()) {
-        if (_dirItem) {
-            item->_renameTarget = _dirItem->_file + "/" + localEntry.renameName;
-        } else {
-            item->_renameTarget = localEntry.renameName;
-        }
-    }
 
     if (dbEntry._modtime == localEntry.modtime && dbEntry._type == ItemTypeVirtualFile && localEntry.type == ItemTypeFile) {
         item->_type = ItemTypeFile;
@@ -602,6 +599,22 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
     // Unknown in db: new file on the server
     Q_ASSERT(!dbEntry.isValid());
 
+    if (!serverEntry.renameName.isEmpty()) {
+        item->_renameTarget = _dirItem ? _dirItem->_file + "/" + serverEntry.renameName : serverEntry.renameName;
+        item->_originalFile = path._original;
+        item->_modtime = serverEntry.modtime;
+        item->_size = serverEntry.size;
+        item->_instruction = CSYNC_INSTRUCTION_RENAME;
+        item->_direction = SyncFileItem::Up;
+        item->_fileId = serverEntry.fileId;
+        item->_remotePerm = serverEntry.remotePerm;
+        item->_etag = serverEntry.etag;
+        item->_type = serverEntry.isDirectory ? CSyncEnums::ItemTypeDirectory : CSyncEnums::ItemTypeFile;
+
+        processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer);
+        return;
+    }
+
     item->_instruction = CSYNC_INSTRUCTION_NEW;
     item->_direction = SyncFileItem::Down;
     item->_modtime = serverEntry.modtime;
@@ -1005,6 +1018,42 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         return;
     }
 
+    if (!localEntry.renameName.isEmpty()) {
+        if (_dirItem) {
+            path._target = _dirItem->_file + "/" + localEntry.renameName;
+        } else {
+            path._target = localEntry.renameName;
+        }
+        OCC::SyncJournalFileRecord base;
+        if (!_discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &base)) {
+            dbError();
+            return;
+        }
+        const auto originalPath = base.path();
+        const auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down);
+        _discoveryData->_renamedItemsLocal.insert(originalPath, path._target);
+        item->_renameTarget = path._target;
+        path._server = adjustedOriginalPath;
+        if (_dirItem) {
+            item->_file = _dirItem->_file + "/" + localEntry.name;
+        } else {
+            item->_file = localEntry.name;
+        }
+        path._original = originalPath;
+        item->_originalFile = path._original;
+        item->_modtime = base._modtime;
+        item->_inode = base._inode;
+        item->_instruction = CSYNC_INSTRUCTION_RENAME;
+        item->_direction = SyncFileItem::Down;
+        item->_fileId = base._fileId;
+        item->_remotePerm = base._remotePerm;
+        item->_etag = base._etag;
+        item->_type = base._type;
+
+        finalize();
+        return;
+    }
+
     // New local file or rename
     item->_instruction = CSYNC_INSTRUCTION_NEW;
     item->_direction = SyncFileItem::Up;
@@ -1341,10 +1390,11 @@ void ProcessDirectoryJob::processFileFinalize(
     if (isVfsWithSuffix()) {
         if (item->_type == ItemTypeVirtualFile) {
             addVirtualFileSuffix(path._target);
-            if (item->_instruction == CSYNC_INSTRUCTION_RENAME)
+            if (item->_instruction == CSYNC_INSTRUCTION_RENAME) {
                 addVirtualFileSuffix(item->_renameTarget);
-            else
+            } else {
                 addVirtualFileSuffix(item->_file);
+            }
         }
         if (item->_type == ItemTypeVirtualFileDehydration
             && item->_instruction == CSYNC_INSTRUCTION_SYNC) {

src/libsync/discoveryphase.h

@@ -49,6 +49,7 @@ struct RemoteInfo
 {
     /** FileName of the entry (this does not contains any directory or path, just the plain name */
     QString name;
+    QString renameName;
     QByteArray etag;
     QByteArray fileId;
     QByteArray checksumHeader;

src/libsync/propagateremotemove.cpp

@@ -19,6 +19,7 @@
 #include "common/syncjournalfilerecord.h"
 #include "filesystem.h"
 #include "common/asserts.h"
+#include <QFileInfo>
 #include <QFile>
 #include <QStringList>
 #include <QDir>
@@ -79,7 +80,7 @@ void PropagateRemoteMove::start()
         return;
 
     QString origin = propagator()->adjustRenamedPath(_item->_file);
-    qCDebug(lcPropagateRemoteMove) << origin << _item->_renameTarget;
+    qCInfo(lcPropagateRemoteMove) << origin << _item->_renameTarget;
 
     QString targetFile(propagator()->fullLocalPath(_item->_renameTarget));
 
@@ -254,6 +255,14 @@ void PropagateRemoteMove::finalize()
             newItem._size = oldRecord._fileSize;
         }
     }
+
+    const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget);
+    if (!QFileInfo::exists(targetFile)) {
+        propagator()->_journal->commit("Remote Rename");
+        done(SyncFileItem::Success);
+        return;
+    }
+
     const auto result = propagator()->updateMetadata(newItem);
     if (!result) {
         done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()));

src/libsync/propagateupload.cpp

@@ -198,26 +198,6 @@ void PropagateUploadFileCommon::start()
     const auto slashPosition = path.lastIndexOf('/');
     const auto parentPath = slashPosition >= 0 ? path.left(slashPosition) : QString();
 
-
-    if (!_item->_renameTarget.isEmpty() && _item->_file != _item->_renameTarget) {
-        // Try to rename the file
-        const auto originalFilePathAbsolute = propagator()->fullLocalPath(_item->_file);
-        const auto newFilePathAbsolute = propagator()->fullLocalPath(_item->_renameTarget);
-        const auto renameSuccess = QFile::rename(originalFilePathAbsolute, newFilePathAbsolute);
-        if (!renameSuccess) {
-            done(SyncFileItem::NormalError, "File contains trailing spaces and couldn't be renamed");
-            return;
-        }
-        _item->_file = _item->_renameTarget;
-        _item->_modtime = FileSystem::getModTime(newFilePathAbsolute);
-        Q_ASSERT(_item->_modtime > 0);
-        if (_item->_modtime <= 0) {
-            qCWarning(lcPropagateUpload()) << "invalid modified time" << _item->_file << _item->_modtime;
-            slotOnErrorStartFolderUnlock(SyncFileItem::NormalError, tr("File %1 has invalid modified time. Do not upload to the server.").arg(QDir::toNativeSeparators(_item->_file)));
-            return;
-        }
-    }
-
     SyncJournalFileRecord parentRec;
     bool ok = propagator()->_journal->getFileRecord(parentPath, &parentRec);
     if (!ok) {

test/testlocaldiscovery.cpp

@@ -207,7 +207,7 @@ private slots:
 
     void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameAndUploadFile()
     {
-        FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };
+        FakeFolder fakeFolder{FileInfo{}};
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         const QString fileWithSpaces1(" foo");
         const QString fileWithSpaces2(" bar  ");
@@ -219,12 +219,33 @@ private slots:
         fakeFolder.localModifier().insert(fileWithSpaces1);
         fakeFolder.localModifier().insert(fileWithSpaces2);
         fakeFolder.localModifier().insert(fileWithSpaces3);
+        fakeFolder.localModifier().mkdir("A");
         fakeFolder.localModifier().insert(fileWithSpaces4);
         fakeFolder.localModifier().insert(fileWithSpaces5);
         fakeFolder.localModifier().insert(fileWithSpaces6);
 
         QVERIFY(fakeFolder.syncOnce());
 
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces1));
+
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces2));
+
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces3));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces6));
+
+        QVERIFY(fakeFolder.syncOnce());
+
         QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces1.trimmed()));
         QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces1));
         QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces1.trimmed()));
@@ -256,6 +277,55 @@ private slots:
         QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces6));
     }
 
+    void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameFile()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces4("A/ foo");
+        const QString fileWithSpaces5("A/ bar  ");
+        const QString fileWithSpaces6("A/bla ");
+
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert(fileWithSpaces4);
+        fakeFolder.remoteModifier().insert(fileWithSpaces5);
+        fakeFolder.remoteModifier().insert(fileWithSpaces6);
+
+        qDebug() << fakeFolder.currentRemoteState();
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        qDebug() << fakeFolder.currentRemoteState();
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces6));
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces4));
+        QVERIFY(fakeFolder.currentLocalState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces5));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces6));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces6));
+
+        auto expectedState = fakeFolder.currentLocalState();
+        QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
+    }
+
     void testCreateFileWithTrailingSpaces_localTrimmedDoesExist_dontRenameAndUploadFile()
     {
         FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };

test/testsyncvirtualfiles.cpp

@@ -763,6 +763,123 @@ private slots:
         QVERIFY(dbRecord(fakeFolder, "case6-rename")._type == ItemTypeFile);
     }
 
+    void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameAndUploadFile()
+    {
+        FakeFolder fakeFolder{ FileInfo() };
+        setupVfs(fakeFolder);
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        const QString fileWithSpaces1(" foo");
+        const QString fileWithSpaces2(" bar  ");
+        const QString fileWithSpaces3("bla ");
+        const QString fileWithSpaces4("A/ foo");
+        const QString fileWithSpaces5("A/ bar  ");
+        const QString fileWithSpaces6("A/bla ");
+
+        fakeFolder.localModifier().insert(fileWithSpaces1);
+        fakeFolder.localModifier().insert(fileWithSpaces2);
+        fakeFolder.localModifier().insert(fileWithSpaces3);
+        fakeFolder.localModifier().mkdir("A");
+        fakeFolder.localModifier().insert(fileWithSpaces4);
+        fakeFolder.localModifier().insert(fileWithSpaces5);
+        fakeFolder.localModifier().insert(fileWithSpaces6);
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces1));
+
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces2));
+
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces3));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentLocalState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces6));
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces1));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces1));
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces2));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces2));
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces3));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces3));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces4));
+        QVERIFY(fakeFolder.currentLocalState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces5));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces6));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces6));
+    }
+
+    void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameFile()
+    {
+        FakeFolder fakeFolder{ FileInfo() };
+        setupVfs(fakeFolder);
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces4("A/ foo");
+        const QString fileWithSpaces5("A/ bar  ");
+        const QString fileWithSpaces6("A/bla ");
+
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert(fileWithSpaces4);
+        fakeFolder.remoteModifier().insert(fileWithSpaces5);
+        fakeFolder.remoteModifier().insert(fileWithSpaces6);
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces4));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces5));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces6));
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/foo"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces4));
+        QVERIFY(fakeFolder.currentLocalState().find("A/foo" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find(QString{fileWithSpaces4 + DVSUFFIX}));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bar"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces5));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bar" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find(QString{fileWithSpaces5 + DVSUFFIX}));
+
+        QVERIFY(fakeFolder.currentRemoteState().find("A/bla"));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces6));
+        QVERIFY(fakeFolder.currentLocalState().find("A/bla" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find(QString{fileWithSpaces6 + DVSUFFIX}));
+    }
+
     // Dehydration via sync works
     void testSyncDehydration()
     {