diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h index 2ba00dc458..56ce2aac45 100644 --- a/src/common/syncjournaldb.h +++ b/src/common/syncjournaldb.h @@ -222,13 +222,13 @@ public: /// Store a new or updated record in the database void setConflictRecord(const ConflictRecord &record); - /// Retrieve a conflict record by path of the _conflict- file + /// Retrieve a conflict record by path of the file with the conflict tag ConflictRecord conflictRecord(const QByteArray &path); - /// Delete a conflict record by path of the _conflict- file + /// Delete a conflict record by path of the file with the conflict tag void deleteConflictRecord(const QByteArray &path); - /// Return all paths of _conflict- files with records in the db + /// Return all paths of files with a conflict tag in the name and records in the db QByteArrayList conflictRecordPaths(); diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h index b09006365b..43259c1e6c 100644 --- a/src/common/syncjournalfilerecord.h +++ b/src/common/syncjournalfilerecord.h @@ -114,18 +114,15 @@ public: /** Represents a conflict in the conflicts table. * - * In the following the "conflict file" is the file with the "_conflict-" - * tag and the base file is the file that its a conflict for. So if - * a/foo.txt is the base file, its conflict file could be - * a/foo_conflict-1234.txt. + * In the following the "conflict file" is the file that has the conflict + * tag in the filename, and the base file is the file that it's a conflict for. + * So if "a/foo.txt" is the base file, its conflict file could be + * "a/foo (conflicted copy 1234).txt". */ class OCSYNC_EXPORT ConflictRecord { public: - /** Path to the _conflict- file - * - * So if a/foo.txt has a conflict, this path would point to - * a/foo_conflict-1234.txt. + /** Path to the file with the conflict tag in the name * * The path is sync-folder relative. */ diff --git a/src/common/utility.cpp b/src/common/utility.cpp index 70e7a5ae70..d2a46bb7c7 100644 --- a/src/common/utility.cpp +++ b/src/common/utility.cpp @@ -548,19 +548,23 @@ QString Utility::makeConflictFileName( const QString &fn, const QDateTime &dt, const QString &user) { QString conflictFileName(fn); - // Add _conflict-XXXX before the extension. + // Add conflict tag before the extension. int dotLocation = conflictFileName.lastIndexOf('.'); // If no extension, add it at the end (take care of cases like foo/.hidden or foo.bar/file) if (dotLocation <= conflictFileName.lastIndexOf('/') + 1) { dotLocation = conflictFileName.size(); } - QString conflictMarker = QStringLiteral("_conflict-"); + QString conflictMarker = QStringLiteral(" (conflicted copy "); if (!user.isEmpty()) { - conflictMarker.append(sanitizeForFileName(user)); - conflictMarker.append('-'); + // Don't allow parens in the user name, to ensure + // we can find the beginning and end of the conflict tag. + const auto userName = sanitizeForFileName(user).replace('(', '_').replace(')', '_'); + conflictMarker.append(userName); + conflictMarker.append(' '); } - conflictMarker.append(dt.toString("yyyyMMdd-hhmmss")); + conflictMarker.append(dt.toString("yyyy-MM-dd hhmmss")); + conflictMarker.append(')'); conflictFileName.insert(dotLocation, conflictMarker); return conflictFileName; @@ -575,13 +579,28 @@ bool Utility::isConflictFile(const char *name) bname = name; } - return std::strstr(bname, "_conflict-"); + // Old pattern + if (std::strstr(bname, "_conflict-")) + return true; + + // New pattern + if (std::strstr(bname, "(conflicted copy")) + return true; + + return false; } bool Utility::isConflictFile(const QString &name) { auto bname = name.midRef(name.lastIndexOf('/') + 1); - return bname.contains("_conflict-", Utility::fsCasePreserving() ? Qt::CaseInsensitive : Qt::CaseSensitive); + + if (bname.contains(QStringLiteral("_conflict-"))) + return true; + + if (bname.contains(QStringLiteral("(conflicted copy"))) + return true; + + return false; } QByteArray Utility::conflictFileBaseName(const QByteArray &conflictName) @@ -589,19 +608,29 @@ QByteArray Utility::conflictFileBaseName(const QByteArray &conflictName) // This function must be able to deal with conflict files for conflict files. // To do this, we scan backwards, for the outermost conflict marker and // strip only that to generate the conflict file base name. - int from = conflictName.size(); - while (from != -1) { - auto start = conflictName.lastIndexOf("_conflict-", from); - if (start == -1) - return ""; - from = start - 1; + auto startOld = conflictName.lastIndexOf("_conflict-"); - auto end = conflictName.indexOf('.', start); - if (end == -1) - end = conflictName.size(); - return conflictName.left(start) + conflictName.mid(end); + // A single space before "(conflicted copy" is considered part of the tag + auto startNew = conflictName.lastIndexOf("(conflicted copy"); + if (startNew > 0 && conflictName[startNew - 1] == ' ') + startNew -= 1; + + // The rightmost tag is relevant + auto tagStart = qMax(startOld, startNew); + if (tagStart == -1) + return ""; + + // Find the end of the tag + auto tagEnd = conflictName.size(); + auto dot = conflictName.lastIndexOf('.'); // dot could be part of user name for new tag! + if (dot > tagStart) + tagEnd = dot; + if (tagStart == startNew) { + auto paren = conflictName.indexOf(')', tagStart); + if (paren != -1) + tagEnd = paren + 1; } - return ""; + return conflictName.left(tagStart) + conflictName.mid(tagEnd); } QString Utility::sanitizeForFileName(const QString &name) diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp index ce78805371..36f8dd84d1 100644 --- a/test/testchunkingng.cpp +++ b/test/testchunkingng.cpp @@ -294,7 +294,7 @@ private slots: // There is a conflict file with our version auto &stateAChildren = localState.find("A")->children; auto it = std::find_if(stateAChildren.cbegin(), stateAChildren.cend(), [&](const FileInfo &fi) { - return fi.name.startsWith("a0_conflict"); + return fi.name.startsWith("a0 (conflicted copy"); }); QVERIFY(it != stateAChildren.cend()); QCOMPARE(it->contentChar, 'B'); diff --git a/test/testexcludedfiles.cpp b/test/testexcludedfiles.cpp index 5043768ecd..11d0a32821 100644 --- a/test/testexcludedfiles.cpp +++ b/test/testexcludedfiles.cpp @@ -37,6 +37,7 @@ private slots: QVERIFY(!excluded.isExcluded("/a/.b", "/a", keepHidden)); QVERIFY(excluded.isExcluded("/a/.Trashes", "/a", keepHidden)); QVERIFY(excluded.isExcluded("/a/foo_conflict-bar", "/a", keepHidden)); + QVERIFY(excluded.isExcluded("/a/foo (conflicted copy bar)", "/a", keepHidden)); QVERIFY(excluded.isExcluded("/a/.b", "/a", excludeHidden)); } }; diff --git a/test/testsyncconflict.cpp b/test/testsyncconflict.cpp index 4bc297a360..8703ccc412 100644 --- a/test/testsyncconflict.cpp +++ b/test/testsyncconflict.cpp @@ -42,7 +42,7 @@ QStringList findConflicts(const FileInfo &dir) { QStringList conflicts; for (const auto &item : dir.children) { - if (item.name.contains("conflict")) { + if (item.name.contains("(conflicted copy")) { conflicts.append(item.path()); } } @@ -56,7 +56,7 @@ bool expectAndWipeConflict(FileModifier &local, FileInfo state, const QString pa if (!base) return false; for (const auto &item : base->children) { - if (item.name.startsWith(pathComponents.fileName()) && item.name.contains("_conflict")) { + if (item.name.startsWith(pathComponents.fileName()) && item.name.contains("(conflicted copy")) { local.remove(item.path()); return true; } @@ -128,7 +128,7 @@ private slots: QCOMPARE(Utility::conflictFileBaseName(conflictMap[a1FileId].toUtf8()), QByteArray("A/a1")); // Check that the conflict file contains the username - QVERIFY(conflictMap[a1FileId].contains(QString("-%1-").arg(fakeFolder.syncEngine().account()->davDisplayName()))); + QVERIFY(conflictMap[a1FileId].contains(QString("(conflicted copy %1 ").arg(fakeFolder.syncEngine().account()->davDisplayName()))); QCOMPARE(remote.find(conflictMap[a1FileId])->contentChar, 'L'); QCOMPARE(remote.find("A/a1")->contentChar, 'R'); @@ -160,7 +160,7 @@ private slots: // file didn't finish in the same sync run that the conflict was created. // To do that we need to create a mock conflict record. auto a1FileId = fakeFolder.remoteModifier().find("A/a1")->fileId; - QString conflictName = QLatin1String("A/a1_conflict-me-1234"); + QString conflictName = QLatin1String("A/a1 (conflicted copy me 1234)"); fakeFolder.localModifier().insert(conflictName, 64, 'L'); ConflictRecord conflictRecord; conflictRecord.path = conflictName.toUtf8(); @@ -210,10 +210,10 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); // With no headers from the server - fakeFolder.remoteModifier().insert("A/a1_conflict-1234"); + fakeFolder.remoteModifier().insert("A/a1 (conflicted copy 1234)"); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); - auto conflictRecord = fakeFolder.syncJournal().conflictRecord("A/a1_conflict-1234"); + auto conflictRecord = fakeFolder.syncJournal().conflictRecord("A/a1 (conflicted copy 1234)"); QVERIFY(conflictRecord.isValid()); QCOMPARE(conflictRecord.baseFileId, fakeFolder.remoteModifier().find("A/a1")->fileId); @@ -312,40 +312,72 @@ private slots: QTest::addColumn("input"); QTest::addColumn("output"); - QTest::newRow("") + QTest::newRow("nomatch1") << "a/b/foo" << ""; - QTest::newRow("") + QTest::newRow("nomatch2") << "a/b/foo.txt" << ""; - QTest::newRow("") + QTest::newRow("nomatch3") << "a/b/foo_conflict" << ""; - QTest::newRow("") + QTest::newRow("nomatch4") << "a/b/foo_conflict.txt" << ""; - QTest::newRow("") + QTest::newRow("match1") << "a/b/foo_conflict-123.txt" << "a/b/foo.txt"; - QTest::newRow("") + QTest::newRow("match2") << "a/b/foo_conflict-foo-123.txt" << "a/b/foo.txt"; - QTest::newRow("") + QTest::newRow("match3") << "a/b/foo_conflict-123" << "a/b/foo"; - QTest::newRow("") + QTest::newRow("match4") << "a/b/foo_conflict-foo-123" << "a/b/foo"; + // new style + QTest::newRow("newmatch1") + << "a/b/foo (conflicted copy 123).txt" + << "a/b/foo.txt"; + QTest::newRow("newmatch2") + << "a/b/foo (conflicted copy foo 123).txt" + << "a/b/foo.txt"; + + QTest::newRow("newmatch3") + << "a/b/foo (conflicted copy 123)" + << "a/b/foo"; + QTest::newRow("newmatch4") + << "a/b/foo (conflicted copy foo 123)" + << "a/b/foo"; + + QTest::newRow("newmatch5") + << "a/b/foo (conflicted copy foo 123) bla" + << "a/b/foo bla"; + + QTest::newRow("newmatch6") + << "a/b/foo (conflicted copy foo.bar 123)" + << "a/b/foo"; + // double conflict files - QTest::newRow("") + QTest::newRow("double1") << "a/b/foo_conflict-123_conflict-456.txt" << "a/b/foo_conflict-123.txt"; - QTest::newRow("") + QTest::newRow("double2") << "a/b/foo_conflict-foo-123_conflict-bar-456.txt" << "a/b/foo_conflict-foo-123.txt"; + QTest::newRow("double3") + << "a/b/foo (conflicted copy 123) (conflicted copy 456).txt" + << "a/b/foo (conflicted copy 123).txt"; + QTest::newRow("double4") + << "a/b/foo (conflicted copy 123)_conflict-456.txt" + << "a/b/foo (conflicted copy 123).txt"; + QTest::newRow("double5") + << "a/b/foo_conflict-123 (conflicted copy 456).txt" + << "a/b/foo_conflict-123.txt"; } void testConflictFileBaseName() @@ -509,8 +541,8 @@ private slots: auto conflicts = findConflicts(fakeFolder.currentLocalState()); std::sort(conflicts.begin(), conflicts.end()); QVERIFY(conflicts.size() == 2); - QVERIFY(conflicts[0].contains("A_conflict")); - QVERIFY(conflicts[1].contains("B_conflict")); + QVERIFY(conflicts[0].contains("A (conflicted copy")); + QVERIFY(conflicts[1].contains("B (conflicted copy")); for (auto conflict : conflicts) QDir(fakeFolder.localPath() + conflict).removeRecursively(); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -548,7 +580,7 @@ private slots: // inside of them! auto conflicts = findConflicts(fakeFolder.currentLocalState()); QVERIFY(conflicts.size() == 1); - QVERIFY(conflicts[0].contains("A_conflict")); + QVERIFY(conflicts[0].contains("A (conflicted copy")); for (auto conflict : conflicts) QDir(fakeFolder.localPath() + conflict).removeRecursively(); diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 67d3c22bea..e51ea114b2 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -42,7 +42,7 @@ QStringList findConflicts(const FileInfo &dir) { QStringList conflicts; for (const auto &item : dir.children) { - if (item.name.contains("conflict")) { + if (item.name.contains("(conflicted copy")) { conflicts.append(item.path()); } } @@ -56,7 +56,7 @@ bool expectAndWipeConflict(FileModifier &local, FileInfo state, const QString pa if (!base) return false; for (const auto &item : base->children) { - if (item.name.startsWith(pathComponents.fileName()) && item.name.contains("_conflict")) { + if (item.name.startsWith(pathComponents.fileName()) && item.name.contains("(conflicted copy")) { local.remove(item.path()); return true; }