clean transition when files with spaces exist already

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

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index ded16cc98adfce4309343fce50ef342dd288e146..80d57201bac82cd5cf76e9ba80c281cd091c4f73 100644 (file)
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -52,10 +52,10 @@ bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
     if (entriesIter != entries.end()) {
         QString errorMessage;
         const auto newFileNameEntry = entriesIter->second;
-        if (newFileNameEntry.serverEntry.isValid()) {
+        if (entry.serverEntry.isValid() && newFileNameEntry.serverEntry.isValid()) {
             errorMessage = tr("File contains trailing spaces and could not be renamed, because a file with the same name already exists on the server.");
         }
-        if (newFileNameEntry.localEntry.isValid()) {
+        if (entry.localEntry.isValid() && newFileNameEntry.localEntry.isValid()) {
             errorMessage = tr("File contains trailing spaces and could not be renamed, because a file with the same name already exists locally.");
         }
 
@@ -71,7 +71,7 @@ bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
             item->_instruction = CSYNC_INSTRUCTION_ERROR;
             item->_status = SyncFileItem::NormalError;
             item->_errorString = errorMessage;
-            emit _discoveryData->itemDiscovered(item);
+            processFileFinalize(item, path, false, ParentNotChanged, ParentNotChanged);
             return false;
         }
     }
@@ -852,6 +852,39 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);
     };
 
+    auto handleInvalidSpaceRename = [&] (SyncFileItem::Direction direction) {
+        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 = direction;
+        item->_fileId = base._fileId;
+        item->_remotePerm = base._remotePerm;
+        item->_etag = base._etag;
+        item->_type = base._type;
+    };
+
     if (!localEntry.isValid()) {
         if (_queryLocal == ParentNotChanged && dbEntry.isValid()) {
             // Not modified locally (ParentNotChanged)
@@ -933,6 +966,8 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
                     || _discoveryData->_syncOptions._vfs->needsMetadataUpdate(*item))) {
                 item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
                 item->_direction = SyncFileItem::Down;
+            } else if (!localEntry.renameName.isEmpty()) {
+                handleInvalidSpaceRename(SyncFileItem::Up);
             }
         } else if (!typeChange && isVfsWithSuffix()
             && dbEntry.isVirtualFile() && !localEntry.isVirtualFile
@@ -1019,37 +1054,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
     }
 
     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;
-
+        handleInvalidSpaceRename(SyncFileItem::Down);
         finalize();
         return;
     }

diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp
index 5f5cdd06e1a640c6d265149ef6fe5c4fd6e3a4a1..d6b1bd29f1479021127a433e28f79c747c6fba43 100644 (file)
--- a/src/libsync/propagateremotemove.cpp
+++ b/src/libsync/propagateremotemove.cpp
@@ -238,10 +238,14 @@ void PropagateRemoteMove::finalize()
     auto &vfs = propagator()->syncOptions()._vfs;
     auto pinState = vfs->pinState(_item->_originalFile);
 
-    // Delete old db data.
-    propagator()->_journal->deleteFileRecord(_item->_originalFile);
-    if (!vfs->setPinState(_item->_originalFile, PinState::Inherited)) {
-        qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << _item->_originalFile << "to inherited";
+    const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget);
+
+    if (QFileInfo::exists(targetFile)) {
+        // Delete old db data.
+        propagator()->_journal->deleteFileRecord(_item->_originalFile);
+        if (!vfs->setPinState(_item->_originalFile, PinState::Inherited)) {
+            qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << _item->_originalFile << "to inherited";
+        }
     }
 
     SyncFileItem newItem(*_item);
@@ -256,7 +260,6 @@ void PropagateRemoteMove::finalize()
         }
     }
 
-    const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget);
     if (!QFileInfo::exists(targetFile)) {
         propagator()->_journal->commit("Remote Rename");
         done(SyncFileItem::Success);

diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp
index 90499bfc92ada10ce335efef22fe871f492cfcb7..fedec81648e425dffc123d881ceafd1e1b56840f 100644 (file)
--- a/test/testlocaldiscovery.cpp
+++ b/test/testlocaldiscovery.cpp
@@ -360,6 +360,32 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces));
         QVERIFY(fakeFolder.currentLocalState().find(fileTrimmed));
     }
+
+    void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedExists_renameFile()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces1(" foo");
+        const QString fileWithSpaces2(" bar  ");
+        const QString fileWithSpaces3("bla ");
+
+        fakeFolder.localModifier().insert(fileWithSpaces1);
+        fakeFolder.localModifier().insert(fileWithSpaces2);
+        fakeFolder.localModifier().insert(fileWithSpaces3);
+        fakeFolder.remoteModifier().insert(fileWithSpaces1);
+        fakeFolder.remoteModifier().insert(fileWithSpaces2);
+        fakeFolder.remoteModifier().insert(fileWithSpaces3);
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        auto expectedState = fakeFolder.currentLocalState();
+        qDebug() << expectedState;
+        QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestLocalDiscovery)