aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/composecontext.cpp23
-rw-r--r--src/maildirname.cpp61
-rw-r--r--src/maildirname.h24
-rw-r--r--src/mainwindow.cpp24
-rw-r--r--tests/test_maildirname.cpp96
-rw-r--r--tests/test_mainwindow.cpp78
6 files changed, 300 insertions, 6 deletions
diff --git a/src/composecontext.cpp b/src/composecontext.cpp
index d0406fc..7233330 100644
--- a/src/composecontext.cpp
+++ b/src/composecontext.cpp
@@ -24,6 +24,7 @@
#include "composecontext.h"
#include "config.h"
+#include "maildirname.h"
#include "mimeparser.h"
#include <QDir>
@@ -491,16 +492,32 @@ ComposeContext ComposeContextBuilder::forDraft(const Config &config,
{
ComposeContext context;
+ // Item 163. The caller's path comes from the model, captured when the
+ // query ran, and mbsync renames an uploaded draft to add its `,U=<uid>`
+ // infix. Resolving first is what stops a rename from refusing the reopen:
+ // the refusal happens BEFORE any composer exists, so the user composes
+ // again into a FRESH window whose autosave has no previous path to unlink,
+ // and the draft is silently forked into two files with two Message-IDs,
+ // both of which reach the server.
+ //
+ // Returns the path unchanged when nothing was renamed, and empty when the
+ // file is genuinely gone, which still fails below exactly as before.
+ const QString resolved = MaildirName::resolveRenamed(path);
+
MimeParser parser;
- const ParsedMessage draft = parser.parse(path);
+ const ParsedMessage draft = parser.parse(resolved);
if (!draft.ok)
return context; // Kind::New and empty: the caller reports the failure.
context.kind = ComposeContext::Kind::Draft;
- context.originalPath = path;
+ context.originalPath = resolved;
// The file this composer OWNS. Without it the first autosave writes a
// second draft and leaves this one behind, so one message becomes two.
- context.draftPath = path;
+ //
+ // The RESOLVED path, never the caller's: seeding the stale one would let
+ // the reopen succeed and the unlink still miss, which is the same fork
+ // arriving one step later.
+ context.draftPath = resolved;
const auto addresses = [](const QString &header) {
QStringList out;
diff --git a/src/maildirname.cpp b/src/maildirname.cpp
index 6263aec..9f827fc 100644
--- a/src/maildirname.cpp
+++ b/src/maildirname.cpp
@@ -20,6 +20,8 @@
#include <QCoreApplication>
#include <QDateTime>
+#include <QDir>
+#include <QFileInfo>
#include <QHostInfo>
namespace MaildirName {
@@ -77,4 +79,63 @@ QString fresh(const QString &oldName)
.arg(info);
}
+QString resolveRenamed(const QString &path)
+{
+ if (path.isEmpty())
+ return QString();
+
+ // The ordinary case, and the overwhelmingly common one: nothing was
+ // renamed. One stat, then out.
+ if (QFileInfo::exists(path))
+ return path;
+
+ const QFileInfo info(path);
+ const QString name = info.fileName();
+
+ // The unique part mbsync preserves. `<stem>:2,D` becomes
+ // `<stem>,U=5:2,D`, so the stem ends at whichever of `,` or `:` comes
+ // first. A name carrying neither is all stem.
+ int cut = name.size();
+ for (const QChar separator : { QLatin1Char(','), QLatin1Char(':') }) {
+ const int at = name.indexOf(separator);
+ if (at >= 0 && at < cut)
+ cut = at;
+ }
+ const QString stem = name.left(cut);
+ if (stem.isEmpty())
+ return QString();
+
+ // One directory, never a recursive walk: a rename keeps the file where it
+ // was, and a file that changed FOLDERS is a different question that only
+ // the message id can answer (see NotmuchWorker::moveMessages(), item 162).
+ const QDir dir(info.absolutePath());
+ if (!dir.exists())
+ return QString();
+
+ QString found;
+ const QFileInfoList entries =
+ dir.entryInfoList(QDir::Files | QDir::NoDotAndDotDot);
+ for (const QFileInfo &entry : entries) {
+ const QString candidate = entry.fileName();
+ // Anchored on the stem AND on what follows it, so `...Q2` cannot match
+ // `...Q23`: the next character must begin the infix or the flags.
+ if (!candidate.startsWith(stem))
+ continue;
+ const QString rest = candidate.mid(stem.size());
+ if (!rest.isEmpty() && !rest.startsWith(QLatin1Char(','))
+ && !rest.startsWith(QLatin1Char(':'))) {
+ continue;
+ }
+
+ // Two files sharing a stem cannot happen in a correct Maildir. Refuse
+ // rather than guess: the caller reports "gone", which is honest, where
+ // a guess could open, move or delete the wrong message.
+ if (!found.isEmpty())
+ return QString();
+ found = entry.absoluteFilePath();
+ }
+
+ return found;
+}
+
} // namespace MaildirName
diff --git a/src/maildirname.h b/src/maildirname.h
index f24bc71..255517d 100644
--- a/src/maildirname.h
+++ b/src/maildirname.h
@@ -38,4 +38,28 @@ namespace MaildirName {
/// what a newly composed draft is.
QString fresh(const QString &oldName);
+/// The file \p path names, or the renamed file that replaced it.
+///
+/// Item 163. mbsync renames an uploaded file to add its `,U=<uid>` infix, and
+/// anything holding the previous name (the model's `MessageRef::filePath`, a
+/// draft's `ComposeContext::draftPath`) then points at a path that no longer
+/// exists. Returns \p path unchanged when it is still there, so the ordinary
+/// case costs one stat and nothing else.
+///
+/// Matched on the UNIQUE STEM, the part before the first `,` or `:`, which
+/// mbsync preserves: `<stem>:2,D` becomes `<stem>,U=5:2,D`. That is what makes
+/// this safe to do by filename at all. The search is confined to the file's
+/// own directory and never recurses, and an ambiguous match (more than one
+/// candidate, which a correct Maildir cannot produce) yields nothing rather
+/// than guessing.
+///
+/// Empty when there is no such file, which every caller must treat as the
+/// genuine "it is gone" it is: recovering silently from a real deletion would
+/// turn a reportable defect into a wrong answer.
+///
+/// This resolves a RENAME, not a MOVE. A file that changed folders is a
+/// different question and belongs to whoever knows the message id;
+/// `NotmuchWorker::moveMessages()` re-resolves that way for item 162.
+QString resolveRenamed(const QString &path);
+
} // namespace MaildirName
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index af3b817..5845922 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -18,6 +18,8 @@
#include "mainwindow.h"
+#include "maildirname.h"
+
#include <QAction>
#include <QApplication>
#include <QCloseEvent>
@@ -1043,8 +1045,13 @@ void MainWindow::openComposerFor(const MessageRef &ref,
return;
}
+ // Item 163, the same stale path the pane and the draft reopen hit. Here it
+ // refuses a Reply or a Forward outright, so the user cannot answer a
+ // message that is sitting on disk and readable.
+ const QString originalPath = MaildirName::resolveRenamed(ref.filePath);
+
MimeParser parser;
- const ParsedMessage original = parser.parse(ref.filePath);
+ const ParsedMessage original = parser.parse(originalPath);
if (!original.ok) {
showTransientStatus(tr("That message could not be read"));
return;
@@ -1052,7 +1059,7 @@ void MainWindow::openComposerFor(const MessageRef &ref,
ComposeContext context;
context.kind = kind;
- context.originalPath = ref.filePath;
+ context.originalPath = originalPath;
const bool replyAll = kind == ComposeContext::Kind::ReplyAll;
const bool forwarding = kind == ComposeContext::Kind::Forward;
@@ -3849,11 +3856,22 @@ void MainWindow::renderMessages(const QVector<MessageRef> &messages)
const MessageRef &ref = messages.at(i);
ThreadRenderItem item;
- item.message = parser.parse(ref.filePath);
+ // Item 163. The model's path was captured when the query ran, and
+ // mbsync renames an uploaded file to add its `,U=<uid>` infix, so a row
+ // loaded before that sync names a file that no longer exists. The pane
+ // then reported the message unreadable while nothing was wrong with it.
+ // Unchanged when nothing was renamed; empty when the file is genuinely
+ // gone, which still reports below.
+ const QString path = MaildirName::resolveRenamed(ref.filePath);
+ item.message = parser.parse(path);
if (!item.message.ok) {
// One unreadable message must not lose the rest of the thread, so
// it becomes an inline note rather than replacing the whole pane.
+ //
+ // Named by the path the model HOLDS, not by the resolved one: when
+ // resolution failed there is no resolved path, and the stale name
+ // is what the user can act on.
item.message = {};
item.message.ok = true;
item.message.from = tr("(unreadable message)");
diff --git a/tests/test_maildirname.cpp b/tests/test_maildirname.cpp
index dcc8fab..a8e4c01 100644
--- a/tests/test_maildirname.cpp
+++ b/tests/test_maildirname.cpp
@@ -18,7 +18,10 @@
#include "maildirname.h"
+#include <QDir>
+#include <QFile>
#include <QSet>
+#include <QTemporaryDir>
#include <QTest>
class TestMaildirName : public QObject
@@ -31,6 +34,11 @@ private slots:
void anEmptyFlagSuffixIsPreserved();
void aNameWithNoSuffixGetsNone();
void theUidInfixIsNotCarriedAcross();
+ void resolveRenamedReturnsAPathThatStillExists();
+ void resolveRenamedFindsTheFileMbsyncRenamed();
+ void resolveRenamedIsEmptyWhenTheFileIsReallyGone();
+ void resolveRenamedDoesNotMatchADifferentMessage();
+ void resolveRenamedRefusesAnAmbiguousMatch();
};
// Two messages written in the same second must not collide, which a
@@ -89,5 +97,93 @@ void TestMaildirName::theUidInfixIsNotCarriedAcross()
.arg(name)));
}
+namespace {
+
+/// One empty file, so a test can assert on which PATH is chosen rather than on
+/// content. resolveRenamed() answers a filesystem question and never opens the
+/// file.
+bool touch(const QString &path)
+{
+ QFile file(path);
+ if (!file.open(QIODevice::WriteOnly))
+ return false;
+ file.close();
+ return true;
+}
+
+} // namespace
+
+void TestMaildirName::resolveRenamedReturnsAPathThatStillExists()
+{
+ // The ordinary case, and the one that must stay cheap: nothing was
+ // renamed, so the answer is the question.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString path = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(path));
+
+ QCOMPARE(MaildirName::resolveRenamed(path), path);
+}
+
+void TestMaildirName::resolveRenamedFindsTheFileMbsyncRenamed()
+{
+ // Item 163. mbsync uploads the file and inserts its `,U=<uid>` infix
+ // before the flag suffix, leaving the unique stem alone.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ const QString renamed =
+ dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=5:2,D"));
+ QVERIFY(touch(renamed));
+ QVERIFY2(!QFile::exists(stale), "the stale path must not exist");
+
+ QCOMPARE(MaildirName::resolveRenamed(stale), renamed);
+}
+
+void TestMaildirName::resolveRenamedIsEmptyWhenTheFileIsReallyGone()
+{
+ // The bounded half. A deleted file must NOT be recovered from, or a
+ // reportable defect becomes a wrong answer.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString gone = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(!QFile::exists(gone));
+
+ QVERIFY(MaildirName::resolveRenamed(gone).isEmpty());
+}
+
+void TestMaildirName::resolveRenamedDoesNotMatchADifferentMessage()
+{
+ // A neighbouring file in the same folder is not this message. Matching on
+ // anything looser than the whole stem would return it, and the caller
+ // would then open, display or MOVE the wrong mail.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q3.host,U=5:2,D"))));
+ QVERIFY(touch(dir.filePath(QStringLiteral("9999999999.M111Q1.host,U=6:2,D"))));
+
+ QVERIFY(MaildirName::resolveRenamed(stale).isEmpty());
+}
+
+void TestMaildirName::resolveRenamedRefusesAnAmbiguousMatch()
+{
+ // Two files sharing one stem cannot happen in a correct Maildir, so this
+ // is a "the world is not what I assumed" case. Guessing between them could
+ // move or delete the wrong file, and the caller reports honestly instead.
+ QTemporaryDir dir;
+ QVERIFY(dir.isValid());
+
+ const QString stale = dir.filePath(QStringLiteral("1787647354.M369Q2.host:2,D"));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=5:2,D"))));
+ QVERIFY(touch(dir.filePath(QStringLiteral("1787647354.M369Q2.host,U=6:2,S"))));
+
+ QVERIFY(MaildirName::resolveRenamed(stale).isEmpty());
+}
+
QTEST_MAIN(TestMaildirName)
#include "test_maildirname.moc"
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index f935cac..21c3661 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -490,6 +490,7 @@ private slots:
void doubleClickingADraftOpensTheComposer();
void aResumedDraftReplacesItsFileRatherThanAddingOne();
void aResumedDraftKeepsItsBlindRecipients();
+ void aDraftRenamedByASyncStillReopensAndReplacesItsFile();
void theComposerSplitsItsToolbarByScope();
void ccAndBccHideBehindADisclosure();
void ccAndBccAreRevealedWhenTheyCarryAValue();
@@ -12861,6 +12862,83 @@ void TestMainWindow::aResumedDraftReplacesItsFileRatherThanAddingOne()
"now exists twice");
}
+void TestMainWindow::aDraftRenamedByASyncStillReopensAndReplacesItsFile()
+{
+ // Item 163, the composer site, and the one that costs data rather than
+ // display. mbsync uploads a draft and renames it to add its `,U=<uid>`
+ // infix; the model's path was captured when the query ran, so the reopen
+ // is handed a name that no longer exists.
+ //
+ // The refusal happens BEFORE any composer exists, so the user composes
+ // again into a FRESH window whose autosave has no previous path to unlink.
+ // The old revision survives, each save mints a new Message-ID, and both
+ // files reach the server. Asserted as the file COUNT, which is the shape
+ // the fork actually takes.
+ ComposeFixture fixture;
+ QVERIFY(fixture.build());
+
+ OutgoingMessage message;
+ message.accountKey = QStringLiteral("acct");
+ message.to = { QStringLiteral("someone@example.org") };
+ message.subject = QStringLiteral("Written before a sync");
+ message.markdownBody = QStringLiteral("The first half.");
+
+ const QString folder = fixture.mailRoot() + QStringLiteral("/acct/Drafts");
+ const QString path = writeDraftFile(folder, message,
+ fixture.config().account(
+ QStringLiteral("acct")));
+ QVERIFY(!path.isEmpty());
+
+ // mbsync's rename: same directory, same unique stem, `,U=<uid>` inserted
+ // before the flag suffix. Nothing reindexes, so the caller below still
+ // holds the pre-rename name, which is the whole precondition.
+ const QFileInfo before(path);
+ const QString base = before.fileName();
+ const int suffix = base.indexOf(QStringLiteral(":2,"));
+ QVERIFY2(suffix > 0, "the draft fixture has no maildir flag suffix");
+ const QString renamed = before.absolutePath() + QLatin1Char('/')
+ + base.left(suffix) + QStringLiteral(",U=7")
+ + base.mid(suffix);
+ QVERIFY2(QFile::rename(path, renamed), "could not stage the sync rename");
+
+ // The guard that proves this test can fail: without it, a fixture that
+ // quietly left the original in place would pass against the bug.
+ QVERIFY2(!QFile::exists(path), "the stale path should no longer exist");
+
+ const auto draftCount = [&folder]() {
+ return QDir(folder + QStringLiteral("/cur"))
+ .entryList(QDir::Files).size();
+ };
+ QCOMPARE(draftCount(), 1);
+
+ // The STALE path, exactly as openComposerFor() passes MessageRef::filePath.
+ const ComposeContext context =
+ ComposeContextBuilder::forDraft(fixture.config(), path);
+ QVERIFY2(context.kind == ComposeContext::Kind::Draft,
+ "the reopen was refused, so the user would compose a second draft");
+ // Resolved, not the caller's: seeding the stale path would let the reopen
+ // succeed and the unlink still miss, forking the draft one step later.
+ QCOMPARE(context.draftPath, renamed);
+
+ ComposeWindow window(context, fixture.config(), fixture.mailRoot());
+ auto *body = window.findChild<QPlainTextEdit *>(QStringLiteral("body"));
+ QVERIFY(body);
+ body->setPlainText(QStringLiteral("The second half."));
+
+ auto *timer = window.findChild<QTimer *>(QStringLiteral("autosave"));
+ QVERIFY2(timer, "the composer has no autosave timer");
+ QVERIFY2(timer->isActive(), "editing the body did not arm the autosave");
+ timer->setInterval(0);
+ QTRY_VERIFY_WITH_TIMEOUT(!timer->isActive(), 5000);
+
+ // Still ONE draft: the autosave replaced the renamed file rather than
+ // leaving it behind beside a new one.
+ QCOMPARE(draftCount(), 1);
+ QVERIFY2(!QFile::exists(renamed),
+ "the renamed draft survived the autosave, so the draft was forked "
+ "into two files and both would reach the server");
+}
+
void TestMainWindow::aResumedDraftKeepsItsBlindRecipients()
{
// MessageBuilder writes Bcc into the draft file deliberately, and says