Reconcile: Rename maps are consistent with update phase #6212

For duplicate file ids the update phase and reconcile phase determined
the rename mappings independently. If they disagreed (due to different
order of processing), complicated misbehavior would result.

This patch fixes it by letting reconcile try to use the mapping that the
update phase has computed first.

src/csync/csync.cpp

@@ -211,14 +211,14 @@ static int _csync_treewalk_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
 
     if (other_file_it == other_tree->cend()) {
         /* Check the renamed path as well. */
-        QByteArray renamed_path = csync_rename_adjust_path(ctx, cur->path);
+        QByteArray renamed_path = csync_rename_adjust_parent_path(ctx, cur->path);
         if (renamed_path != cur->path)
             other_file_it = other_tree->find(renamed_path);
     }
 
     if (other_file_it == other_tree->cend()) {
         /* Check the source path as well. */
-        QByteArray renamed_path = csync_rename_adjust_path_source(ctx, cur->path);
+        QByteArray renamed_path = csync_rename_adjust_parent_path_source(ctx, cur->path);
         if (renamed_path != cur->path)
             other_file_it = other_tree->find(renamed_path);
     }

src/csync/csync_reconcile.cpp

@@ -113,7 +113,7 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
 
     if (!other) {
         /* Check the renamed path as well. */
-        other = other_tree->findFile(csync_rename_adjust_path(ctx, cur->path));
+        other = other_tree->findFile(csync_rename_adjust_parent_path(ctx, cur->path));
     }
     if (!other) {
         /* Check if it is ignored */
@@ -147,24 +147,25 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
             cur->instruction = CSYNC_INSTRUCTION_NEW;
 
             bool processedRename = false;
-            auto renameCandidateProcessing = [&](const OCC::SyncJournalFileRecord &base) {
+            auto renameCandidateProcessing = [&](const QByteArray &basePath) {
                 if (processedRename)
                     return;
-                if (!base.isValid())
+                if (basePath.isEmpty())
                     return;
 
                 /* First, check that the file is NOT in our tree (another file with the same name was added) */
-                if (our_tree->findFile(base._path)) {
-                    qCDebug(lcReconcile, "Origin found in our tree : %s", base._path.constData());
+                if (our_tree->findFile(basePath)) {
+                    other = nullptr;
+                    qCDebug(lcReconcile, "Origin found in our tree : %s", basePath.constData());
                 } else {
                     /* Find the potential rename source file in the other tree.
                     * If the renamed file could not be found in the opposite tree, that is because it
                     * is not longer existing there, maybe because it was renamed or deleted.
                     * The journal is cleaned up later after propagation.
                     */
-                    other = other_tree->findFile(base._path);
+                    other = other_tree->findFile(basePath);
                     qCDebug(lcReconcile, "Rename origin in other tree (%s) %s",
-                        base._path.constData(), other ? "found" : "not found");
+                        basePath.constData(), other ? "found" : "not found");
                 }
 
                 if(!other) {
@@ -197,7 +198,7 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
                     cur->instruction = CSYNC_INSTRUCTION_NONE;
                     // We have consumed 'other': exit this loop to not consume another one.
                     processedRename = true;
-                } else if (our_tree->findFile(csync_rename_adjust_path(ctx, other->path)) == cur) {
+                } else if (our_tree->findFile(csync_rename_adjust_parent_path(ctx, other->path)) == cur) {
                     // If we're here, that means that the other side's reconcile will be able
                     // to work against cur: The filename itself didn't change, only a parent
                     // directory was renamed! In that case it's safe to ignore the rename
@@ -225,12 +226,34 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
                 qCDebug(lcReconcile, "Finding rename origin through inode %" PRIu64 "",
                     cur->inode);
                 ctx->statedb->getFileRecordByInode(cur->inode, &base);
-                renameCandidateProcessing(base);
+                renameCandidateProcessing(base._path);
             } else {
                 ASSERT(ctx->current == REMOTE_REPLICA);
-                qCDebug(lcReconcile, "Finding rename origin through file ID %s",
-                    cur->file_id.constData());
-                ctx->statedb->getFileRecordsByFileId(cur->file_id, renameCandidateProcessing);
+
+                // The update phase has already mapped out all dir->dir renames, check the
+                // path that is consistent with that first. Otherwise update mappings and
+                // reconcile mappings might disagree, leading to odd situations down the
+                // line.
+                auto basePath = csync_rename_adjust_full_path_source(ctx, cur->path);
+                if (basePath != cur->path) {
+                    qCDebug(lcReconcile, "Trying rename origin by csync_rename mapping %s",
+                        basePath.constData());
+                    // We go through getFileRecordsByFileId to ensure the basePath
+                    // computed in this way also has the expected fileid.
+                    ctx->statedb->getFileRecordsByFileId(cur->file_id,
+                        [&](const OCC::SyncJournalFileRecord &base) {
+                            if (base._path == basePath)
+                                renameCandidateProcessing(basePath);
+                        });
+                }
+
+                // Also feed all the other files with the same fileid if necessary
+                if (!processedRename) {
+                    qCDebug(lcReconcile, "Finding rename origin through file ID %s",
+                        cur->file_id.constData());
+                    ctx->statedb->getFileRecordsByFileId(cur->file_id,
+                        [&](const OCC::SyncJournalFileRecord &base) { renameCandidateProcessing(base._path); });
+                }
             }
 
             break;

src/csync/csync_rename.cpp

@@ -36,7 +36,7 @@ void csync_rename_record(CSYNC* ctx, const QByteArray &from, const QByteArray &t
     ctx->renames.folder_renamed_from[to] = from;
 }
 
-QByteArray csync_rename_adjust_path(CSYNC* ctx, const QByteArray &path)
+QByteArray csync_rename_adjust_parent_path(CSYNC *ctx, const QByteArray &path)
 {
     if (ctx->renames.folder_renamed_to.empty())
         return path;
@@ -50,11 +50,25 @@ QByteArray csync_rename_adjust_path(CSYNC* ctx, const QByteArray &path)
     return path;
 }
 
-QByteArray csync_rename_adjust_path_source(CSYNC* ctx, const QByteArray &path)
+QByteArray csync_rename_adjust_parent_path_source(CSYNC *ctx, const QByteArray &path)
 {
     if (ctx->renames.folder_renamed_from.empty())
         return path;
-    for (auto p = _parentDir(path); !p.isEmpty(); p = _parentDir(p)) {
+    for (ByteArrayRef p = _parentDir(path); !p.isEmpty(); p = _parentDir(p)) {
+        auto it = ctx->renames.folder_renamed_from.find(p);
+        if (it != ctx->renames.folder_renamed_from.end()) {
+            QByteArray rep = it->second + path.mid(p.length());
+            return rep;
+        }
+    }
+    return path;
+}
+
+QByteArray csync_rename_adjust_full_path_source(CSYNC *ctx, const QByteArray &path)
+{
+    if (ctx->renames.folder_renamed_from.empty())
+        return path;
+    for (ByteArrayRef p = path; !p.isEmpty(); p = _parentDir(p)) {
         auto it = ctx->renames.folder_renamed_from.find(p);
         if (it != ctx->renames.folder_renamed_from.end()) {
             QByteArray rep = it->second + path.mid(p.length());

src/csync/csync_rename.h

@@ -22,10 +22,19 @@
 
 #include "csync.h"
 
-/* Return the final destination path of a given patch in case of renames */
-QByteArray OCSYNC_EXPORT csync_rename_adjust_path(CSYNC *ctx, const QByteArray &path);
+/* Return the final destination path of a given patch in case of renames
+ *
+ * Does only map the parent directories. If the directory "A" is renamed to
+ * "B" then this function will not map "A" to "B". Only "A/foo" -> "B/foo".
+*/
+QByteArray OCSYNC_EXPORT csync_rename_adjust_parent_path(CSYNC *ctx, const QByteArray &path);
+
 /* Return the source of a given path in case of renames */
-QByteArray OCSYNC_EXPORT csync_rename_adjust_path_source(CSYNC *ctx, const QByteArray &path);
+QByteArray OCSYNC_EXPORT csync_rename_adjust_parent_path_source(CSYNC *ctx, const QByteArray &path);
+
+/* like the parent_path variant, but applying to the full path */
+QByteArray OCSYNC_EXPORT csync_rename_adjust_full_path_source(CSYNC *ctx, const QByteArray &path);
+
 void OCSYNC_EXPORT csync_rename_record(CSYNC *ctx, const QByteArray &from, const QByteArray &to);
 /*  Return the amount of renamed item recorded */
 bool OCSYNC_EXPORT csync_rename_count(CSYNC *ctx);

src/libsync/discoveryphase.cpp

@@ -75,7 +75,7 @@ bool DiscoveryJob::isInSelectiveSyncBlackList(const QByteArray &path) const
 
     // Also try to adjust the path if there was renames
     if (csync_rename_count(_csync_ctx)) {
-        QByteArray adjusted = csync_rename_adjust_path_source(_csync_ctx, path);
+        QByteArray adjusted = csync_rename_adjust_parent_path_source(_csync_ctx, path);
         if (adjusted != path) {
             return findPathInList(_selectiveSyncBlackList, QString::fromUtf8(adjusted));
         }

test/testsyncmove.cpp

@@ -231,12 +231,25 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), remoteInfo);
     }
 
+    void testDuplicateFileId_data()
+    {
+        QTest::addColumn<QString>("prefix");
+
+        // There have been bugs related to how the original
+        // folder and the folder with the duplicate tree are
+        // ordered. Test both cases here.
+        QTest::newRow("first ordering") << "O"; // "O" > "A"
+        QTest::newRow("second ordering") << "0"; // "0" < "A"
+    }
+
     // If the same folder is shared in two different ways with the same
     // user, the target user will see duplicate file ids. We need to make
     // sure the move detection and sync still do the right thing in that
     // case.
     void testDuplicateFileId()
     {
+        QFETCH(QString, prefix);
+
         FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
         auto &remote = fakeFolder.remoteModifier();
 
@@ -245,8 +258,8 @@ private slots:
         remote.mkdir("A/Q");
 
         // Duplicate every entry in A under O/A
-        remote.mkdir("O");
-        remote.children["O"].addChild(remote.children["A"]);
+        remote.mkdir(prefix);
+        remote.children[prefix].addChild(remote.children["A"]);
 
         // This already checks that the rename detection doesn't get
         // horribly confused if we add new files that have the same
@@ -263,28 +276,28 @@ private slots:
 
         // Try a remote file move
         remote.rename("A/a1", "A/W/a1m");
-        remote.rename("O/A/a1", "O/A/W/a1m");
+        remote.rename(prefix + "/A/a1", prefix + "/A/W/a1m");
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(nGET, 0);
 
         // And a remote directory move
         remote.rename("A/W", "A/Q/W");
-        remote.rename("O/A/W", "O/A/Q/W");
+        remote.rename(prefix + "/A/W", prefix + "/A/Q/W");
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(nGET, 0);
 
         // Partial file removal (in practice, A/a2 may be moved to O/a2, but we don't care)
-        remote.rename("O/A/a2", "O/a2");
+        remote.rename(prefix + "/A/a2", prefix + "/a2");
         remote.remove("A/a2");
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(nGET, 0);
 
         // Local change plus remote move at the same time
-        fakeFolder.localModifier().appendByte("O/a2");
-        remote.rename("O/a2", "O/a3");
+        fakeFolder.localModifier().appendByte(prefix + "/a2");
+        remote.rename(prefix + "/a2", prefix + "/a3");
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         QCOMPARE(nGET, 1);