Conflicts: Change tags to be more user friendly #6365

From "_conflict-user-yyyymmdd-hhmmss"
to   " (conflicted copy user yyyy-mm-dd hhmmss)"

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();
 
 

src/common/syncjournalfilerecord.h

@@ -115,18 +115,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.
      */

src/common/utility.cpp

@@ -559,19 +559,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;
@@ -586,13 +590,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)
@@ -600,19 +619,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 end = conflictName.indexOf('.', start);
-        if (end == -1)
-            end = conflictName.size();
-        return conflictName.left(start) + conflictName.mid(end);
+    auto startOld = conflictName.lastIndexOf("_conflict-");
+
+    // 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)

test/testchunkingng.cpp

@@ -289,7 +289,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');

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));
     }
 };

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<QString>("input");
         QTest::addColumn<QString>("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();
 

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;
         }