[commit] master: make MaxSize ignore source-side message flagging

[Oswald B. <os...@us...>]

commit cb687f1beeef8e90f4181b1788830ddbc8e26fdd
Author: Oswald Buddenhagen <os...@us...>
Date:   Fri Jun 17 16:49:33 2022 +0200

    make MaxSize ignore source-side message flagging
    
    when propagation of too big messages was entirely suppressed, the only
    way to force it was flagging the source message. however, now that we
    have placeholders that can be flagged to trigger full propagation, it's
    rather pointless to keep the old method working, and still doing it
    does in fact confuse users, see for example
    REFMAIL: CAO...@ma...
    
    to avoid this, we now almost completely shadow the regular meaning of
    flagging - it basically becomes a non-synchronizable flag until the
    placeholder is upgraded.

 NEWS             |  3 ++
 src/mbsync.1     | 11 ++++---
 src/run-tests.pl | 38 +++++++++--------------
 src/sync.c       | 80 +++++++++++++++++++++++++++---------------------
 4 files changed, 70 insertions(+), 62 deletions(-)

diff --git a/NEWS b/NEWS
index b862c249..4b1f260f 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,9 @@ The old locations remain supported.
 The reference point for relative local paths in the configuration file
 is now the file's containing directory.
 
+Placeholders will be now created for messages exceeding MaxSize even if
+they are flagged on the source side.
+
 The unfiltered list of mailboxes in each Store can be printed now.
 
 A proper summary is now printed prior to exiting.
diff --git a/src/mbsync.1 b/src/mbsync.1
index a681e6fa..7933d66f 100644
--- a/src/mbsync.1
+++ b/src/mbsync.1
@@ -160,14 +160,17 @@ directory.
 .TP
 \fBMaxSize\fR \fIsize\fR[\fBk\fR|\fBm\fR][\fBb\fR]
 Messages larger than \fIsize\fR will have only a small placeholder message
-propagated into this Store. To propagate the full message, it must be
-flagged in either Store; that can be done retroactively, in which case
-the \fBReNew\fR operation needs to be executed instead of \fBNew\fR.
+propagated into this Store.
 This is useful for avoiding downloading messages with large attachments
 unless they are actually needed.
-Caveat: Setting a size limit on a Store you never read directly (which is
+To upgrade the placeholder to the full message, it must be flagged, and
+the \fBReNew\fR operation executed.
+Caveats: Setting a size limit on a Store you never read directly (which is
 typically the case for servers) is not recommended, as you may never
 notice that affected messages were not propagated to it.
+Also, as flagging is (ab-)used to request an upgrade, changes to the
+message's flagging state will not be propagated in either direction until
+after the placeholder is upgraded.
 .br
 \fBK\fR and \fBM\fR can be appended to the size to specify KiBytes resp.
 MeBytes instead of bytes. \fBB\fR is accepted but superfluous.
diff --git a/src/run-tests.pl b/src/run-tests.pl
index 7d00f8de..deec5045 100755
--- a/src/run-tests.pl
+++ b/src/run-tests.pl
@@ -1113,8 +1113,8 @@ test("noop + expunge near side", \@x01, \@X0A, \@O0A);
 my @x20 = (
   0, 0, 0,
   A, "*", "", "",
-  B, "*P*", "", "",
-  C, "", "", "**",
+  B, "*FP*", "", "",
+  C, "", "", "*F*",
 );
 
 my @O21 = ("MaxSize 1k\n", "MaxSize 1k\n", "Expunge Near");
@@ -1127,42 +1127,34 @@ my @X21 = (
 test("max size", \@x20, \@X21, \@O21);
 
 my @x22 = (
-  E, 0, B,
+  E, 0, V,
   A, "*", "", "",
   B, "*PR*", "", "",
-  C, "*PR?", "*<DP", "*DFP*",
-  D, "*PR?", "*<DP", "*DP*",
+  V, "*FPR*", "", "",
+  C, "*FPR?", "*<DP", "*DP*",
+  W, "*FPR?", "*<DP", "*DFP*",
+  D, "*PR?", "*<DP", "*DFP*",
   E, "*PR*", "*>DP", "*DP?",
   A, "", "*", "*",
   B, "", "*>DP", "*DFP?",
+  V, "", "*>DP", "*DFP?",
 );
 
 my @X22 = (
-  C, 0, B,
+  W, 0, V,
   B, "", ">->D+R", "^PR*",
   B, "", "", "&1/",
-  C, "^FPR*", "<-<D+FR", "-D+R",
+  V, "", ">->D+FR", "^FPR*",
+  V, "", "", "&1/",
+  C, "^PR*", "<-<D+R", "-D+R",
   C, "&1+T", "^", "&",
+  W, "^FPR*", "<-<D+FR", "-D+R",
+  W, "&1+T", "^", "&",
   D, "", "-D+R", "-D+R",
   E, "", "-D+R", "-D+R",
 );
 test("max size + flagging", \@x22, \@X22, \@O21);
 
-my @x23 = (
-  0, 0, 0,
-  A, "*", "", "",
-  B, "*F*", "", "",
-  C, "", "", "*F*",
-);
-
-my @X23 = (
-  C, 0, B,
-  C, "*F*", "*F", "",
-  A, "", "*", "*",
-  B, "", "*F", "*F*",
-);
-test("max size + initial flagging", \@x23, \@X23, \@O21);
-
 my @x24 = (
   C, 0, A,
   A, "*", "*", "*",
@@ -1173,7 +1165,7 @@ my @x24 = (
 my @X24 = (
   C, 0, C,
   B, "", ">-^+>", "*?",
-  C, "", ">-^+FP", "*FP*",
+  C, "", ">-^+>P", "*P?",
 );
 test("max size (pre-1.4 legacy)", \@x24, \@X24, \@O21);
 
diff --git a/src/sync.c b/src/sync.c
index b84efc02..d3899dee 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -203,12 +203,15 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars, int t )
 	}
 
 	uint dummy_msg_len = 0;
-	char dummy_msg_buf[180];
+	char dummy_msg_buf[256];
 	static const char dummy_pfx[] = "[placeholder] ";
 	static const char dummy_subj[] = "Subject: [placeholder] (No Subject)";
 	static const char dummy_msg[] =
 		"Having a size of %s, this message is over the MaxSize limit.%s"
 		"Flag it and sync again (Sync mode ReNew) to fetch its real contents.%s";
+	static const char dummy_flag[] =
+		"%s"
+		"The original message is flagged as important.%s";
 
 	if (vars->minimal) {
 		char sz[32];
@@ -219,6 +222,10 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars, int t )
 			sprintf( sz, "%.1fMiB", vars->msg->size / 1048576. );
 		const char *nl = app_cr ? "\r\n" : "\n";
 		dummy_msg_len = (uint)sprintf( dummy_msg_buf, dummy_msg, sz, nl, nl );
+		if (vars->data.flags & F_FLAGGED) {
+			vars->data.flags &= ~F_FLAGGED;
+			dummy_msg_len += (uint)sprintf( dummy_msg_buf + dummy_msg_len, dummy_flag, nl, nl );
+		}
 		extra += dummy_msg_len;
 		extra += add_subj ? strlen(dummy_subj) + app_cr + 1 : strlen(dummy_pfx);
 	}
@@ -299,6 +306,8 @@ msg_fetched( int sts, void *aux )
 		} else {
 			vars->data.flags = sanitize_flags( vars->data.flags, svars, t );
 			if (srec) {
+				if (srec->status & S_DUMMY(t))
+					vars->data.flags &= ~F_FLAGGED;
 				if (vars->data.flags) {
 					srec->pflags = vars->data.flags;
 					JLOG( "%% %u %u %u", (srec->uid[F], srec->uid[N], srec->pflags),
@@ -759,14 +768,12 @@ box_opened2( sync_vars_t *svars, int t )
 				debug( "resuming %s of %d new message(s)\n", str_hl[t], any_new[t] );
 				opts[t^1] |= OPEN_NEW;
 				if (chan->stores[t]->max_size != UINT_MAX)
-					opts[t^1] |= OPEN_FLAGS | OPEN_NEW_SIZE;
+					opts[t^1] |= OPEN_NEW_SIZE;
 			}
 			if ((chan->ops[t] & OP_RENEW) || any_upgrades[t]) {
 				debug( "resuming %s of %d upgrade(s)\n", str_hl[t], any_upgrades[t] );
-				if (chan->ops[t] & OP_RENEW) {
+				if (chan->ops[t] & OP_RENEW)
 					opts[t] |= OPEN_OLD | OPEN_FLAGS | OPEN_SETFLAGS;
-					opts[t^1] |= OPEN_FLAGS;
-				}
 				opts[t^1] |= OPEN_OLD;
 			}
 			if ((chan->ops[t] | chan->ops[t^1]) & OP_EXPUNGE)  // Don't propagate doomed msgs
@@ -1036,9 +1043,29 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 			del[F] = no[F] && srec->uid[F];
 			del[N] = no[N] && srec->uid[N];
 
+			sync_rec_t *nsrec = srec;
 			for (t = 0; t < 2; t++) {
+				// Do this before possibly upgrading that side.
 				if (srec->msg[t] && (srec->msg[t]->flags & F_DELETED))
 					srec->status |= S_DEL(t);
+				// Flagging the message on the target side causes an upgrade of the dummy.
+				// We do this first in a separate loop, so flag propagation sees the upgraded
+				// state for both sides. After a journal replay, that would be the case anyway.
+				if ((svars->chan->ops[t] & OP_RENEW) && (srec->status & S_DUMMY(t)) && srec->uid[t^1] && srec->msg[t]) {
+					sflags = srec->msg[t]->flags;
+					if (sflags & F_FLAGGED) {
+						sflags &= ~(F_SEEN | F_FLAGGED);  // As below.
+						// We save away the dummy's flags, because after an
+						// interruption it may be already gone.
+						srec->pflags = sflags;
+						JLOG( "^ %u %u %u", (srec->uid[F], srec->uid[N], srec->pflags),
+						      "upgrading %s placeholder, dummy's flags %s",
+						      (str_fn[t], fmt_lone_flags( srec->pflags ).str) );
+						nsrec = upgrade_srec( svars, srec, t );
+					}
+				}
+			}
+			for (t = 0; t < 2; t++) {
 				if (srec->status & S_UPGRADE) {
 					// Such records hold orphans by definition, so the del[] cases are irrelevant.
 					if (srec->uid[t]) {
@@ -1108,10 +1135,19 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 							sflags &= ~F_DELETED;
 						}
 						if (srec->status & S_DUMMY(t^1)) {
-							// For placeholders, don't propagate:
+							// From placeholders, don't propagate:
 							// - Seen, because the real contents were obviously not seen yet
 							// - Flagged, because it's just a request to upgrade
 							sflags &= ~(F_SEEN|F_FLAGGED);
+						} else if (srec->status & S_DUMMY(t)) {
+							// Don't propagate Flagged to placeholders, as that would be
+							// misunderstood as a request to upgrade next time around. We
+							// could replace the placeholder with one with(out) the flag
+							// notice line (we can't modify the existing one due to IMAP
+							// semantics), but that seems like major overkill, esp. as the
+							// user likely wouldn't even notice the change. So the flag
+							// won't be seen until the placeholder is upgraded - tough luck.
+							sflags &= ~F_FLAGGED;
 						}
 						srec->aflags[t] = sflags & ~srec->flags;
 						srec->dflags[t] = ~sflags & srec->flags;
@@ -1122,25 +1158,6 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 					}
 				}
 			}
-
-			sync_rec_t *nsrec = srec;
-			if (((srec->status & S_DUMMY(F)) && (svars->chan->ops[F] & OP_RENEW)) ||
-			     ((srec->status & S_DUMMY(N)) && (svars->chan->ops[N] & OP_RENEW))) {
-				// Flagging the message on either side causes an upgrade of the dummy.
-				// We ignore flag resets, because that corner case is not worth it.
-				ushort muflags = srec->msg[F] ? srec->msg[F]->flags : 0;
-				ushort suflags = srec->msg[N] ? srec->msg[N]->flags : 0;
-				if ((muflags | suflags) & F_FLAGGED) {
-					t = (srec->status & S_DUMMY(F)) ? F : N;
-					// We save away the dummy's flags, because after an
-					// interruption it may be already gone. Filtering as above.
-					srec->pflags = srec->msg[t]->flags & ~(F_SEEN | F_FLAGGED);
-					JLOG( "^ %u %u %u", (srec->uid[F], srec->uid[N], srec->pflags),
-					      "upgrading %s placeholder, dummy's flags %s",
-					      (str_fn[t], fmt_lone_flags( srec->pflags ).str) );
-					nsrec = upgrade_srec( svars, srec, t );
-				}
-			}
 			srec = nsrec;  // Minor optimization: skip freshly created placeholder entry.
 		}
 	}
@@ -1165,15 +1182,9 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 					// Pre-1.4 legacy only: The message was skipped due to being too big.
 					if (!(svars->chan->ops[t] & OP_RENEW))
 						continue;
-					srec->status = S_PENDING;
 					// The message size was not queried, so this won't be dummified below.
-					if (!(tmsg->flags & F_FLAGGED)) {
-						srec->status |= S_DUMMY(t);
-						JLOG( "_ %u %u", (srec->uid[F], srec->uid[N]), "placeholder only - was previously skipped" );
-					} else {
-						JLOG( "~ %u %u %d", (srec->uid[F], srec->uid[N], srec->status & S_LOGGED),
-						      "was previously skipped" );
-					}
+					srec->status = S_PENDING | S_DUMMY(t);
+					JLOG( "_ %u %u", (srec->uid[F], srec->uid[N]), "placeholder only - was previously skipped" );
 				} else {
 					if (!(svars->chan->ops[t] & OP_NEW) && !(srec->status & S_UPGRADE))
 						continue;
@@ -1241,8 +1252,7 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 				srec->status = S_DEAD;
 				continue;
 			}
-			if (!(tmsg->flags & F_FLAGGED) && tmsg->size > svars->chan->stores[t]->max_size &&
-			    !(srec->status & (S_DUMMY(F) | S_DUMMY(N)))) {
+			if (tmsg->size > svars->chan->stores[t]->max_size && !(srec->status & (S_DUMMY(F) | S_DUMMY(N)))) {
 				srec->status |= S_DUMMY(t);
 				JLOG( "_ %u %u", (srec->uid[F], srec->uid[N]), "placeholder only - too big" );
 			}