diff options
| -rw-r--r-- | CHANGELOG.md | 22 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 77 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 40 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 143 |
4 files changed, 273 insertions, 9 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index d17b46c..8ddbe50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,28 @@ point at which they are stable. ## [Unreleased] +### Fixed + +- Moving a message between folders now gives its file a fresh Maildir name. + 0.26.0 carried the old name across, including mbsync's `,U=<n>` UID infix, + which belongs to the folder the file came from. Moving a message out and + back reinserted a UID the server had since reassigned, and mbsync refused + the folder with `Maildir error: duplicate UID`. If you saw that error, see + Upgrading below. + +### Upgrading + +If a sync reported `Maildir error: duplicate UID <n> in <folder>` after +deleting or restoring mail with 0.26.0, that folder holds two files claiming +one UID. No mail is lost; mbsync simply refuses to sync the folder until it is +resolved. Stop any running sync, then strip the `,U=<n>` infix from the newer +of each pair and reindex: + + notmuch new + +mbsync re-derives the UID on the next sync. The code no longer creates this +state. + ### Added - The message pane's right-click menu offers **Select all**. Chromium's own menu diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 2c03226..66b408c 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -20,9 +20,12 @@ #include <notmuch.h> +#include <QCoreApplication> +#include <QDateTime> #include <QDir> #include <QDirIterator> #include <QFileInfo> +#include <QHostInfo> #include <QSet> #include <cstdlib> @@ -662,6 +665,63 @@ void NotmuchWorker::applyTags(const TagChange &change) emit tagsApplied(change); } +namespace { + +/// A fresh Maildir filename for a message being moved between folders, +/// preserving only its `:2,<flags>` suffix. +/// +/// mbsync's manual is explicit about why this exists, under "the more +/// efficient default UID mapping scheme": "it is important that the MUA +/// renames files when moving them between Maildir folders", and "the general +/// expectation is that a completely new filename is generated as if the +/// message was new". +/// +/// The `,U=<n>` infix mbsync writes is its per-folder IMAP UID. Carrying it +/// into another folder makes it a claim about a folder the file is no longer +/// in; moving a message out and back then reinserts a UID the server has +/// since reassigned, and mbsync refuses the folder with `Maildir error: +/// duplicate UID`. Measured on real mail, four collisions in one folder from +/// a single move-and-restore. +/// +/// The FLAGS are kept, deliberately, and that is not a contradiction of +/// "as if the message was new". They record seen, flagged and replied, and +/// `maildir.synchronize_flags` is true, so notmuch reads them back as tags: +/// dropping them would mark every deleted message unread and lose Important +/// on the way to the trash. Only the unique part is regenerated. +QString freshMaildirName(const QString &oldName) +{ + // The `:2,` suffix, when there is one. `info` is everything from the + // separator on, so an empty-flag `:2,` is preserved as faithfully as + // `:2,FS`. + QString info; + const int sep = oldName.indexOf(QStringLiteral(":2,")); + if (sep >= 0) + info = oldName.mid(sep); + + // The conventional left-to-right unique part: time, a per-process counter, + // the pid, the host. The counter is what makes two messages moved in the + // same second distinct, which a timestamp alone does not guarantee. + static quint64 counter = 0; + const qint64 now = QDateTime::currentSecsSinceEpoch(); + const QString host = QHostInfo::localHostName().isEmpty() + ? QStringLiteral("localhost") + : QHostInfo::localHostName(); + + return QStringLiteral("%1.M%2P%3Q%4.%5%6") + .arg(now) + .arg(QDateTime::currentMSecsSinceEpoch() % 1000) + .arg(QCoreApplication::applicationPid()) + .arg(++counter) + // A `/` or a `:` in a hostname would break the path or the flag + // separator. Neither is legal in a hostname, so this is belt and + // braces rather than a known case. + .arg(QString(host).replace(QLatin1Char('/'), QLatin1Char('_')) + .replace(QLatin1Char(':'), QLatin1Char('_'))) + .arg(info); +} + +} // namespace + void NotmuchWorker::moveMessages(const QStringList &messageIds, const QString &destFolder) { @@ -728,15 +788,24 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, continue; } - const QString to = destDir + QLatin1Char('/') + QFileInfo(from).fileName(); - if (from == to) { - // Already where it was asked to go. Reported as moved, since the - // caller's request is satisfied. + // Already where it was asked to go, compared on the DIRECTORY rather + // than on the full path. It used to compare paths, which worked only + // because the filename was carried across unchanged; with a fresh name + // that test can never be true, so a message already in the destination + // would be renamed on every move for no reason, and every rename is a + // new filename mbsync has to reconcile. + if (QFileInfo(from).absolutePath() == QFileInfo(destDir).absoluteFilePath()) { moved.append(id); origins.insert(id, origin); continue; } + // A FRESH name, never the old one. See freshMaildirName(): carrying + // the `,U=` infix across a folder boundary is what produced + // `Maildir error: duplicate UID` on real mail. + const QString to = destDir + QLatin1Char('/') + + freshMaildirName(QFileInfo(from).fileName()); + if (!QFile::rename(from, to)) { emit errorOccurred(QStringLiteral("Cannot move %1 to %2") .arg(QFileInfo(from).fileName(), destFolder)); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 79e137a..0deaec3 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -8907,15 +8907,51 @@ static bool notmuchTag(const QString &configPath, const QStringList &args) return process.waitForFinished(15000) && process.exitCode() == 0; } +/// Whether `dir` holds the message whose id the fixture wrote as `stem`. +/// +/// Matches on the MESSAGE-ID INSIDE each file, never on the filename. It used +/// to compare filenames, which worked only while a move carried the name +/// across unchanged. It no longer does: mbsync requires an MUA to rename a +/// file when it moves it between folders, so `NotmuchWorker::moveMessages()` +/// generates a fresh name and this helper could never find a moved message +/// again. Every one of these assertions failed at once, each reporting "the +/// file is not there" about a file that was. +/// +/// `stem` stays in the fixture's `<local>.example.org` form so the fifty-odd +/// call sites did not have to change; it is turned back into +/// `<local@example.org>` here. static bool folderHasMessageFile(const QString &dir, const QString &stem) { QDir directory(dir); if (!directory.exists()) return false; + + // `del1.example.org` is the fixture's rendering of `del1@example.org`: + // it replaces the `@` to make a filename-safe stem. Only the LAST dot + // before the domain is the substituted one, so the split is on the first + // dot, which is where the local part ends for every id these tests use. + const int dot = stem.indexOf(QLatin1Char('.')); + if (dot < 0) + return false; + const QString messageId = QStringLiteral("<%1@%2>") + .arg(stem.left(dot), stem.mid(dot + 1)); + const QStringList entries = directory.entryList(QDir::Files); for (const QString &entry : entries) { - if (entry == stem || entry.startsWith(stem + QLatin1Char(':'))) - return true; + QFile file(directory.filePath(entry)); + if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) + continue; + // The header block only: a quoted id in a body must not count. + while (!file.atEnd()) { + const QByteArray line = file.readLine(); + if (line.trimmed().isEmpty()) + break; + if (line.startsWith("Message-ID:") || line.startsWith("Message-Id:")) { + if (QString::fromUtf8(line).contains(messageId)) + return true; + break; + } + } } return false; } diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index bf1c23c..b5da31a 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -90,6 +90,9 @@ private slots: void moveMessagesReindexesAtTheNewPath(); void moveMessagesKeepsTheMessagesTags(); void moveMessagesReportsOnlyWhatMoved(); + void moveMessagesGivesTheFileAFreshMaildirName(); + void moveMessagesKeepsTheMaildirFlags(); + void twoMessagesMovedTogetherGetDistinctNames(); private: /// Adds one read message in `folder` and reindexes, for the move tests. @@ -1160,9 +1163,19 @@ void TestNotmuchWorker::moveMessagesRelocatesTheFile() // cur/, never new/: a file in new/ is re-announced as fresh mail by every // reader of the Maildir. - const QString expected = m_fixture.maildirPath() + QStringLiteral("/trash/cur/") - + QFileInfo(before).fileName(); - QVERIFY2(QFile::exists(expected), qPrintable(expected)); + // + // Asserted on the DIRECTORY, not on the full path. The filename is + // deliberately regenerated by the move (see + // moveMessagesGivesTheFileAFreshMaildirName), so an assertion naming the + // old filename here encoded the very bug that item fixes: it required the + // name to be carried across, which is what produced duplicate mbsync UIDs + // on real mail. + const QString expectedDir = + m_fixture.maildirPath() + QStringLiteral("/trash/cur"); + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QCOMPARE(QFileInfo(after).absolutePath(), expectedDir); + QVERIFY2(QFile::exists(after), qPrintable(after)); QVERIFY(!QFile::exists(before)); } @@ -1217,6 +1230,130 @@ void TestNotmuchWorker::moveMessagesKeepsTheMessagesTags() .arg(after.join(QLatin1Char(' '))))); } +void TestNotmuchWorker::moveMessagesGivesTheFileAFreshMaildirName() +{ + // mbsync's manual, under "the more efficient default UID mapping scheme": + // "it is important that the MUA renames files when moving them between + // Maildir folders", and "the general expectation is that a completely new + // filename is generated as if the message was new". + // + // qtmaildir is that MUA and did not rename. The `,U=<n>` infix is mbsync's + // per-folder IMAP UID, so carrying it into another folder makes it a claim + // about a folder the file is no longer in. Moving a message out and back + // then reinserts a UID the server has since reassigned, and mbsync reports + // `Maildir error: duplicate UID`. + // + // Measured on the user's real Maildir on 2026-08-19: four collisions in one + // folder, eight distinct messages, from a move-out and restore made with + // 0.26.0. + const QString id = QStringLiteral("move5@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + // A realistic mbsync name, which the fixture does not produce on its own: + // <unique>,U=<n>:2,<flags>. + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + const QString uidName = QFileInfo(before).absolutePath() + + QStringLiteral("/move5.example.org,U=42:2,S"); + QVERIFY2(QFile::rename(before, uidName), "could not stage a ,U= filename"); + QVERIFY(m_fixture.index()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ id }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + QVERIFY(QFile::exists(after)); + + const QString name = QFileInfo(after).fileName(); + // The point of the item: no UID infix survives the move. + QVERIFY2(!name.contains(QStringLiteral(",U=")), + qPrintable(QStringLiteral("the moved file kept a UID infix: %1") + .arg(name))); + // And it is a genuinely new name rather than the old one with the infix + // cut out, which is what mbsync calls the expectation. + QVERIFY2(name != QStringLiteral("move5.example.org:2,S"), + qPrintable(QStringLiteral("the name was only stripped, not " + "regenerated: %1").arg(name))); +} + +void TestNotmuchWorker::moveMessagesKeepsTheMaildirFlags() +{ + // A fresh name must NOT mean fresh state. The `:2,<flags>` suffix carries + // seen, flagged and replied, and notmuch's maildir.synchronize_flags is + // true on the user's setup, so dropping it would mark read mail unread and + // lose Important on every message the user deletes. + // + // Asserted on the FLAGS rather than on the whole name, since the unique + // part is expected to change and the flags are expected not to. + const QString id = QStringLiteral("move6@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), id), + qPrintable(m_fixture.error())); + + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + // Seen and Flagged, so a suffix that is dropped or truncated shows up. + const QString staged = QFileInfo(before).absolutePath() + + QStringLiteral("/move6.example.org,U=7:2,FS"); + QVERIFY2(QFile::rename(before, staged), "could not stage a flagged filename"); + QVERIFY(m_fixture.index()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ id }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString after = fileOf(id); + QVERIFY2(!after.isEmpty(), "the message is not in the database after the move"); + const QString name = QFileInfo(after).fileName(); + + QVERIFY2(name.endsWith(QStringLiteral(":2,FS")), + qPrintable(QStringLiteral("the move lost the maildir flags: %1") + .arg(name))); + QVERIFY(!name.contains(QStringLiteral(",U="))); +} + +void TestNotmuchWorker::twoMessagesMovedTogetherGetDistinctNames() +{ + // The generated name must be unique, since a collision is the entire class + // of bug this change exists to remove: two files landing on one name means + // one message silently overwrites the other. + // + // Two messages in ONE batch, which is the case a timestamp alone does not + // cover: both are moved in the same second, so only the per-process counter + // separates them. A generator using time and pid alone passes every other + // test here and fails this one. + const QString first = QStringLiteral("move7@example.org"); + const QString second = QStringLiteral("move8@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), first), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("inbox"), second), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.moveMessages({ first, second }, QStringLiteral("trash")); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + const QString a = fileOf(first); + const QString b = fileOf(second); + QVERIFY2(!a.isEmpty() && !b.isEmpty(), + "a message is missing from the database after the move"); + + // Distinct names... + QVERIFY2(QFileInfo(a).fileName() != QFileInfo(b).fileName(), + qPrintable(QStringLiteral("both messages were named %1") + .arg(QFileInfo(a).fileName()))); + // ...and both files really are on disk, which is what a collision would + // have destroyed. The name check alone would pass against one file that + // overwrote the other if the database still named two paths. + QVERIFY(QFile::exists(a)); + QVERIFY(QFile::exists(b)); +} + void TestNotmuchWorker::moveMessagesReportsOnlyWhatMoved() { // A stale id must not abort the batch, and must not be reported as moved |