Placeholders: Fix migration issues

Some edgecases weren't covered and didn't have tests yet.

src/csync/csync_reconcile.cpp

@@ -176,9 +176,12 @@ static void _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx)
                 cur->instruction = CSYNC_INSTRUCTION_NEW;
                 break;
             }
-            if (cur->type == ItemTypePlaceholder && ctx->current == REMOTE_REPLICA) {
-                /* Do not remove on the server if the local placeholder is gone:
-                 * instead reestablish the local placeholder */
+            /* If the local placeholder is gone it should be reestablished.
+             * Unless the base file is seen in the local tree now. */
+            if (cur->type == ItemTypePlaceholder
+                && ctx->current == REMOTE_REPLICA
+                && cur->path.endsWith(ctx->placeholder_suffix)
+                && !other_tree->findFile(cur->path.left(cur->path.size() - ctx->placeholder_suffix.size()))) {
                 cur->instruction = CSYNC_INSTRUCTION_NEW;
                 break;
             }
@@ -451,6 +454,17 @@ static void _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx)
             if (cur->instruction == CSYNC_INSTRUCTION_EVAL)
                 cur->instruction = CSYNC_INSTRUCTION_NEW;
             break;
+        case CSYNC_INSTRUCTION_NONE:
+            // NONE/NONE on placeholders might become a REMOVE if the base file
+            // is found in the local tree.
+            if (cur->type == ItemTypePlaceholder
+                && other->instruction == CSYNC_INSTRUCTION_NONE
+                && ctx->current == LOCAL_REPLICA
+                && cur->path.endsWith(ctx->placeholder_suffix)
+                && ctx->local.files.findFile(cur->path.left(cur->path.size() - ctx->placeholder_suffix.size()))) {
+                cur->instruction = CSYNC_INSTRUCTION_REMOVE;
+            }
+            break;
         default:
             break;
         }

src/csync/csync_update.cpp

@@ -47,6 +47,7 @@
 #include "common/asserts.h"
 
 #include <QtCore/QTextCodec>
+#include <QtCore/QFile>
 
 // Needed for PRIu64 on MinGW in C++ mode.
 #define __STDC_FORMAT_MACROS
@@ -790,18 +791,10 @@ int csync_ftw(CSYNC *ctx, const char *uri, csync_walker_fn fn,
         && dirent->type == ItemTypeFile
         && filename.endsWith(ctx->placeholder_suffix)) {
         QByteArray db_uri = fullpath.mid(strlen(ctx->local.uri) + 1);
-        QByteArray base_uri = db_uri.left(db_uri.size() - ctx->placeholder_suffix.size());
-
-        // Don't fill the local tree with placeholder data if a real
-        // file was found. The remote tree will still have the placeholder
-        // file.
-        if (ctx->local.files.findFile(base_uri))
-            continue;
 
         if( ! fill_tree_from_db(ctx, db_uri.constData(), true) ) {
-          errno = ENOENT;
-          ctx->status_code = CSYNC_STATUS_OPENDIR_ERROR;
-          goto error;
+            qCWarning(lcUpdate) << "Placeholder without db entry for" << filename;
+            QFile::remove(fullpath);
         }
 
         continue;

test/testsyncplaceholders.cpp

@@ -141,6 +141,26 @@ private slots:
         QVERIFY(!dbRecord(fakeFolder, "A/a1.owncloud").isValid());
         QVERIFY(!dbRecord(fakeFolder, "A/a1m.owncloud").isValid());
         cleanup();
+
+        // Edge case: Local placeholder but no db entry for some reason
+        fakeFolder.remoteModifier().insert("A/a2", 64);
+        fakeFolder.remoteModifier().insert("A/a3", 64);
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(fakeFolder.currentLocalState().find("A/a2.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("A/a3.owncloud"));
+        cleanup();
+
+        fakeFolder.syncEngine().journal()->deleteFileRecord("A/a2.owncloud");
+        fakeFolder.syncEngine().journal()->deleteFileRecord("A/a3.owncloud");
+        fakeFolder.remoteModifier().remove("A/a3");
+        fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::FilesystemOnly);
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(fakeFolder.currentLocalState().find("A/a2.owncloud"));
+        QVERIFY(itemInstruction(completeSpy, "A/a2.owncloud", CSYNC_INSTRUCTION_NEW));
+        QVERIFY(dbRecord(fakeFolder, "A/a2.owncloud").isValid());
+        QVERIFY(!fakeFolder.currentLocalState().find("A/a3.owncloud"));
+        QVERIFY(!dbRecord(fakeFolder, "A/a3.owncloud").isValid());
+        cleanup();
     }
 
     void testPlaceholderConflict()
@@ -335,7 +355,7 @@ private slots:
     }
 
     // Check what might happen if an older sync client encounters placeholders
-    void testOldVersion()
+    void testOldVersion1()
     {
         FakeFolder fakeFolder{ FileInfo() };
         SyncOptions syncOptions;
@@ -378,6 +398,39 @@ private slots:
         QVERIFY(!fakeFolder.currentLocalState().find("A/a1.owncloud"));
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
+
+    // Older versions may leave db entries for foo and foo.owncloud
+    void testOldVersion2()
+    {
+        FakeFolder fakeFolder{ FileInfo() };
+
+        // Sync a file
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert("A/a1");
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        // Create the placeholder too
+        // In the wild, the new version would create the placeholder and the db entry
+        // while the old version would download the plain file.
+        fakeFolder.localModifier().insert("A/a1.owncloud");
+        auto &db = fakeFolder.syncJournal();
+        SyncJournalFileRecord rec;
+        db.getFileRecord(QByteArray("A/a1"), &rec);
+        rec._type = ItemTypePlaceholder;
+        rec._path = "A/a1.owncloud";
+        db.setFileRecord(rec);
+
+        SyncOptions syncOptions;
+        syncOptions._newFilesArePlaceholders = true;
+        fakeFolder.syncEngine().setSyncOptions(syncOptions);
+
+        // Check that a sync removes the placeholder and its db entry
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(!fakeFolder.currentLocalState().find("A/a1.owncloud"));
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QVERIFY(!dbRecord(fakeFolder, "A/a1.owncloud").isValid());
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncPlaceholders)