diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/config.cpp | 8 | ||||
| -rw-r--r-- | src/config.h | 6 | ||||
| -rw-r--r-- | src/mailsync.cpp | 90 | ||||
| -rw-r--r-- | src/mailsync.h | 80 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 77 | ||||
| -rw-r--r-- | src/mainwindow.h | 25 |
6 files changed, 276 insertions, 10 deletions
diff --git a/src/config.cpp b/src/config.cpp index 8b784ba..0f0caf1 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -475,6 +475,14 @@ void Config::load(const QString &path) if (m_syncLog.isEmpty()) m_syncLog = MailSync::defaultLogPath(); + // The status file the script writes (item 174), beside the log and unvalidated + // for the same reason: a fresh install has none until the first sync runs, + // and a missing one reads as SyncState::Unknown when the time comes. + m_syncStatus = + settings.value(QStringLiteral("sync/status")).toString().trimmed(); + if (m_syncStatus.isEmpty()) + m_syncStatus = MailSync::defaultStatusPath(); + // Account groups are written as [account.work], [account.personal], etc. // A dot, not a slash, separates the "account" namespace from the key: // QSettings' INI backend treats "/" as its own hierarchical group diff --git a/src/config.h b/src/config.h index 26fc1c1..1ab923d 100644 --- a/src/config.h +++ b/src/config.h @@ -381,6 +381,11 @@ public: /// clear on a cron sync, which is exactly the defect this exists to fix. QString syncLog() const { return m_syncLog; } + /// The status file assets/mailsync.sh writes, which is what the + /// application READS to learn what a run it did not start actually did + /// (item 174). The log beside it is for a human. + QString syncStatus() const { return m_syncStatus; } + /// Optional alternate notmuch config file. Empty means "let notmuch decide". QString notmuchConfig() const { return m_notmuchConfig; } @@ -548,6 +553,7 @@ private: QString m_syncCommand; QString m_syncLog; + QString m_syncStatus; int m_toolbarIconSize = 24; QString m_notmuchConfig; QString m_dateFormat; diff --git a/src/mailsync.cpp b/src/mailsync.cpp index 10e5ef7..caf61ef 100644 --- a/src/mailsync.cpp +++ b/src/mailsync.cpp @@ -21,6 +21,9 @@ #include <QCoreApplication> #include <QDir> #include <QFile> +#include <QJsonArray> +#include <QJsonDocument> +#include <QJsonObject> #include <QRegularExpression> namespace { @@ -251,6 +254,93 @@ QString MailSync::defaultLogPath() return QDir::homePath() + QStringLiteral("/.local/state/mailsync.log"); } +QString MailSync::defaultStatusPath() +{ + // Hardcoded to match assets/mailsync.sh for the same reason defaultLogPath + // is: the script builds it from $HOME, and QStandardPaths would derive a + // path the script never writes. + // + // Under the application's own state directory rather than beside + // mailsync.log, and the split is deliberate: the log belongs to the script + // and a human reads it, while this file is the interface between the two + // programs. + return QDir::homePath() + + QStringLiteral("/.local/state/qtmaildir/syncstatus.json"); +} + +SyncStatus MailSync::readStatus(const QString &statusPath) +{ + // Every failure below returns this untouched, so Unknown is the default + // rather than something each branch has to remember to set. + SyncStatus status; + + if (statusPath.isEmpty()) + return status; + + QFile file(statusPath); + if (!file.open(QIODevice::ReadOnly)) + return status; + + // The whole file: it holds one run and is a few hundred bytes. The cap is + // against a path that is not the file we think it is, since a reader on the + // UI thread must not swallow something enormous by mistake. + constexpr qint64 kMaxBytes = 64 * 1024; + if (file.size() > kMaxBytes) + return status; + + QJsonParseError error{}; + const QJsonDocument doc = QJsonDocument::fromJson(file.readAll(), &error); + if (error.error != QJsonParseError::NoError || !doc.isObject()) + return status; + + const QJsonObject object = doc.object(); + + // Refused rather than guessed at, the rule the rules file already follows: + // a later version may mean something different by these same field names, + // and acting on it would be worse than observing nothing. The script writes + // 1 and both sides bump together. + if (object.value(QStringLiteral("version")).toInt() != 1) + return status; + + const QString state = object.value(QStringLiteral("state")).toString(); + if (state == QLatin1String("ok")) + status.state = SyncState::Ok; + else if (state == QLatin1String("failed")) + status.state = SyncState::Failed; + else if (state == QLatin1String("skipped")) + status.state = SyncState::Skipped; + else + return status; // An unrecognised state is not a fourth kind of run. + + const QJsonArray channels = + object.value(QStringLiteral("channels")).toArray(); + for (const QJsonValue &value : channels) { + const QString channel = value.toString(); + if (channel.isEmpty()) + continue; + // "-a" is the script's word for "every channel", not the name of one. + // Kept as a flag so a caller cannot match it against configured + // channels, find nothing, and clear nothing on the run that carried + // everything. + if (channel == QLatin1String("-a")) + status.everyChannel = true; + else + status.channels.append(channel); + } + + status.mbsyncStatus = + object.value(QStringLiteral("mbsync_status")).toInt(-1); + status.notmuchStatus = + object.value(QStringLiteral("notmuch_status")).toInt(-1); + + status.started = QDateTime::fromString( + object.value(QStringLiteral("started")).toString(), Qt::ISODate); + status.ended = QDateTime::fromString( + object.value(QStringLiteral("ended")).toString(), Qt::ISODate); + + return status; +} + SyncOutcome MailSync::lastRunOutcome(const QString &logPath) { if (logPath.isEmpty()) diff --git a/src/mailsync.h b/src/mailsync.h index a826f43..d614dbc 100644 --- a/src/mailsync.h +++ b/src/mailsync.h @@ -18,6 +18,7 @@ #pragma once +#include <QDateTime> #include <QObject> #include <QProcess> #include <QString> @@ -76,6 +77,64 @@ enum class SyncOutcome { Failed, }; +/// What a finished run was, from the status file (item 174). +/// +/// Three states rather than SyncOutcome's two, and the third is the point: +/// a run that SKIPPED because another held the lock is neither a success nor a +/// failure, and having no way to say so is why item 125 left the spinner +/// running for ever. +enum class SyncState { + Unknown, + Ok, + Failed, + Skipped, +}; + +/// One finished run of the sync script, as the script itself reported it. +/// +/// This exists because the application used to INFER a finished run, from an +/// inode in /proc/locks and from grepping the log for its RUN END banner. That +/// made a human-readable line into wire format, and it could not answer the +/// question the pending count actually needs answered: which channels did this +/// run carry? The local sync path has always narrowed its clear to the accounts +/// it carried; the external path could not, and cleared everything. +/// +/// Written by assets/mailsync.sh, which is the only producer. The two agree by +/// TEST rather than by shared code, exactly as the two readers of rules.json +/// do: assets/test_mailsync.py pins the writer, test_mailsync.cpp pins the +/// reader, and one test runs the real script and reads what it wrote. +struct SyncStatus +{ + SyncState state = SyncState::Unknown; + + /// The channels the run synced. Empty when `everyChannel` is true. + QStringList channels; + + /// The run covered every account, which the script reports as "-a". + /// + /// Carried as a flag rather than left as the literal string in `channels`, + /// because "-a" is not a channel name: a caller matching it against + /// configured channels finds nothing and clears nothing, on exactly the run + /// that carried everything. + bool everyChannel = false; + + /// Reported separately as well as folded into `state`, because they mean + /// different things: a failed mbsync means the edits never reached the + /// server, while a failed notmuch means they did and only the local index + /// is behind. -1 for a run where neither program ran. + int mbsyncStatus = -1; + int notmuchStatus = -1; + + QDateTime started; + QDateTime ended; + + /// True only for a run that completed with both programs succeeding. + /// Nothing else may clear the pending count, per the rule the local path + /// states: clearing on a failure asserts the edits reached the mail store + /// when the sync is exactly what failed to put them there. + bool carriedEdits() const { return state == SyncState::Ok; } +}; + /// Runs the configured external sync command. /// /// qtmaildir deliberately does not implement sync itself. The existing script @@ -125,6 +184,27 @@ public: /// Anything unreadable, absent or unmarked is Unknown. static SyncOutcome lastRunOutcome(const QString &logPath); + /// Reads the status file assets/mailsync.sh writes (item 174). + /// + /// Preferred over lastRunOutcome(), which stays as the fallback for a file + /// that is missing or unreadable: that is what a first run after upgrading + /// looks like, and deleting a working mechanism in the same change that + /// adds its replacement leaves two broken things instead of one. + /// + /// Anything unreadable, absent, malformed or of an unrecognised version + /// returns a default SyncStatus, whose state is Unknown. Callers must + /// change no state on Unknown, exactly as they must for SyncOutcome and + /// SyncMonitor::State: it is the absence of evidence, not evidence of + /// absence. + /// + /// A whole-file read rather than a tail, unlike lastRunOutcome(): the file + /// holds one run and is a few hundred bytes, where the log holds every run + /// of the day. + static SyncStatus readStatus(const QString &statusPath); + + /// Where the status file lives when the config names none. + static QString defaultStatusPath(); + signals: void started(); void outputReceived(const QString &chunk); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 1da9ac7..01c0251 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -5089,6 +5089,10 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) return; m_externalSyncBusy = true; + // When this run began, so the status file it leaves can be told from + // one an earlier run left (item 174). A stale file must not be read as + // this run's result. + m_externalSyncStartedAt = QDateTime::currentDateTime(); updateSyncControls(); m_statusLabel->setText(tr("Background sync running...")); m_announcedExternalSync = true; @@ -5174,12 +5178,54 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // Without this the indicator kept reporting work that had already // shipped, and the exit prompt asked to sync for it. // - // The outcome comes from the RUN END line the script writes, because - // the process that ran this sync is gone and its exit status with it. - // Anything other than a definite OK changes nothing: the local path's - // rule is that only a SUCCESSFUL sync may clear the count, and Unknown - // is the absence of evidence rather than evidence of success. - if (MailSync::lastRunOutcome(m_config.syncLog()) == SyncOutcome::Ok) { + // The process that ran this sync is gone and its exit status with it, + // so what it did has to be read from what it left behind. + // + // Item 174: the status file the script writes, which says which + // CHANNELS the run carried. The RUN END line in the log remains the + // fallback for a status file that is missing or unreadable, which is + // what a first run after upgrading looks like; it cannot name channels, + // so that path keeps the old blanket clear. + // + // Anything other than a definite success changes nothing, on either + // path: the local rule is that only a SUCCESSFUL sync may clear the + // count, and Unknown is the absence of evidence rather than evidence of + // success. A SKIPPED run is neither, and clears nothing: the other run + // is doing the work and this one carried none of it. + // The status file is preferred, but only when it describes THIS run. + // A stale one outranking a fresh log would be worse than not having it: + // an old success would clear the count for a run that has just failed, + // which is the indicator lying in the direction that loses work. + // + // "This run" is judged on the file being at least as new as the sync + // that just ended. m_externalSyncStartedAt is when the lock appeared, + // and the script writes the file immediately before exiting, so a file + // older than that belongs to an earlier run. + const SyncStatus status = MailSync::readStatus(m_config.syncStatus()); + + // The script writes `date -Iseconds`, which carries no milliseconds, so + // a file written in the same second as the lock appeared parses as up + // to 999ms EARLIER than it. Measured: an ISODate round trip of "now" + // comes back 329ms behind. A plain `>=` therefore judges a fast sync's + // own status file stale and falls back to the log, which is the + // opposite of the intent. + // + // One second of slack, matching the precision the format actually + // carries. This cannot readmit a genuinely stale file: cron runs ten + // minutes apart, and a run whose file is a second old IS this run. + constexpr qint64 kTimestampSlackMs = 1000; + const bool statusIsForThisRun = + status.state != SyncState::Unknown + && (!m_externalSyncStartedAt.isValid() || !status.ended.isValid() + || status.ended.msecsTo(m_externalSyncStartedAt) + <= kTimestampSlackMs); + + const bool carried = + statusIsForThisRun + ? status.carriedEdits() + : MailSync::lastRunOutcome(m_config.syncLog()) == SyncOutcome::Ok; + + if (carried) { m_pendingTagEdits.clear(); // Cleared HERE, before flushHeldEdits() below, and the ordering is @@ -5188,10 +5234,21 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // writes m_editedAccounts SYNCHRONOUSLY. Clearing after the flush // would discard accounts whose edits this run did not carry, and // those edits would then sync only when some later edit happened to - // name the same account. Running first, everything in the set at - // this moment is exactly what the finished sync carried, so the - // local path's snapshot-and-subtract collapses to a clear. - m_editedAccounts.clear(); + // name the same account. + // + // WHICH accounts, when the status file said. A run naming channels + // carried those and no others, so clearing the whole set would + // report an untouched account's edits as shipped. `-a` and the log + // fallback both mean every account, where the blanket clear is + // right. + if (!status.everyChannel && !status.channels.isEmpty()) { + for (const Account &account : m_config.accounts()) { + if (status.channels.contains(account.syncChannel())) + m_editedAccounts.remove(account.key); + } + } else { + m_editedAccounts.clear(); + } updatePendingIndicator(); } } diff --git a/src/mainwindow.h b/src/mainwindow.h index 2b217d2..9c93f4f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -202,6 +202,20 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// Item 174. The set of accounts with edits not yet known to have reached + /// the mail store, so a test can assert that an external sync cleared the + /// accounts it carried and ONLY those. + QSet<QString> editedAccountsForTesting() const { return m_editedAccounts; } + + /// Marks an account edited, standing in for the write funnels: a test + /// asserting which accounts a sync clears needs more than one of them + /// edited, and driving two real writes through a worker to arrange that + /// would test the funnels rather than the clearing. + Q_INVOKABLE void noteEditedAccountForTesting(const QString &accountKey) + { + m_editedAccounts.insert(accountKey); + } + /// Item 178. Stands in for the digest round trip, which a bare window has /// no worker to make. Sets what onThreadDigestLoaded() would have set. void setConversationPathsForTesting(const QString &threadId, @@ -1485,6 +1499,17 @@ private: /// half. Tracked here rather than read back from SyncMonitor so the state /// the UI acted on is the state it was told about. bool m_externalSyncBusy = false; + + /// When the external sync now running began, from the lock appearing. + /// + /// Item 174. The status file the script leaves is preferred over the log's + /// banner, but only when it describes THIS run: a stale file outranking a + /// fresh log would clear the pending count on an old success for a run that + /// has just failed, which is the indicator lying in the direction that + /// loses work. Invalid when no external sync has been observed, where the + /// comparison is skipped rather than failing closed on a file that may well + /// be current. + QDateTime m_externalSyncStartedAt; QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; |