Fix the semantics of the hold and keepsave variables in mail

* mail/delete.c (mail_delete_msg): Set MAIL_ATTRIBUTE_PRESERVED.
This is not necessary, stricto sensu, because mail_mbox_close
skips messages marked for deletion anyway.  But it's good for
clarity.
* mail/quit.c (mailbox_classify): New function.
(mail_mbox_commit): Rewrite

* mail/tests/Makefile.am: Add new test
* mail/tests/testsuite.at: Include new tests.
* mail/tests/atlocal.in (catmbox): New function.
* mail/tests/hold.at: New test.

7 files changed, 430 insertions, 45 deletions

diff --git a/mail/delete.c b/mail/delete.c
index 344857246..567fd4e03 100644
--- a/mail/delete.c
+++ b/mail/delete.c
@@ -27,6 +27,7 @@ mail_delete_msg (msgset_t *mspec, mu_message_t msg, void *data)
 
   mu_message_get_attribute (msg, &attr);
   mu_attribute_set_deleted (attr);
+  mu_attribute_unset_userflag (attr, MAIL_ATTRIBUTE_PRESERVED);
   cond_page_invalidate (mspec->msg_part[0]);
   return 0;
 }
diff --git a/mail/quit.c b/mail/quit.c
index 2e8b5a1cd..161582959 100644
--- a/mail/quit.c
+++ b/mail/quit.c
@@ -30,7 +30,7 @@ mail_quit (int argc MU_ARG_UNUSED, char **argv MU_ARG_UNUSED)
 }
 
 int
-mail_mbox_close ()
+mail_mbox_close (void)
 {
   mu_url_t url = NULL;
   size_t held_count = 0;
@@ -58,8 +58,36 @@ mail_mbox_close ()
   return 0;
 }
 
+enum mailbox_class
+  {
+    MBX_SYSTEM,
+    MBX_MBOX,
+    MBX_USER
+  };
+
+static enum mailbox_class
+mailbox_classify (void)
+{
+  mu_url_t url;
+
+  mu_mailbox_get_url (mbox, &url);
+  if (strcmp (util_url_to_string (url), getenv ("MBOX")) == 0)
+    return MBX_MBOX;
+  else
+    {
+      mu_mailbox_t mb;
+      mu_url_t u;
+      mu_mailbox_create_default (&mb, NULL);
+      mu_mailbox_get_url (mb, &u);
+      if (strcmp (mu_url_to_string (u), mu_url_to_string (url)) == 0)
+        return MBX_SYSTEM;
+    }
+
+  return MBX_USER;
+}
+
 int
-mail_mbox_commit ()
+mail_mbox_commit (void)
 {
   unsigned int i;
   mu_mailbox_t dest_mbox = NULL;
@@ -68,49 +96,54 @@ mail_mbox_commit ()
   mu_attribute_t attr;
   int keepsave = mailvar_is_true (mailvar_name_keepsave);
   int hold = mailvar_is_true (mailvar_name_hold);
-  mu_url_t url;
-  int is_user_mbox;
 
-  mu_mailbox_get_url (mbox, &url);
-  is_user_mbox = strcmp (util_url_to_string (url), getenv ("MBOX")) == 0;
-
-  {
-    mu_mailbox_t mb;
-    mu_url_t u;
-    mu_mailbox_create_default (&mb, NULL);
-    mu_mailbox_get_url (mb, &u);
-    if (strcmp (mu_url_to_string (u), mu_url_to_string (url)) != 0)
-      {
-        /* The mailbox we are closing is not a system one (%). Raise
-           hold flag */
-        hold = 1;
-        keepsave = 1;
-      }
-    mu_mailbox_destroy (&mb);
-  }
+  enum mailbox_class class = mailbox_classify ();
+  if (class != MBX_SYSTEM)
+    {
+      /* The mailbox we are closing is not a system one (%). Stay on the
+         safe side: retain both read and saved messages in the mailbox. */
+      hold = 1;
+      keepsave = 1;
+    }
 
   for (i = 1; i <= total; i++)
     {
+      int status;
+      enum { ACT_KEEP, ACT_MBOX, ACT_DELETE } action = ACT_KEEP;
+
       if (util_get_message (mbox, i, &msg))
         return 1;
 
       mu_message_get_attribute (msg, &attr);
+      if (mu_attribute_is_deleted (attr))
+        continue;
+
+      if (mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_PRESERVED))
+        action = ACT_KEEP;
+      else if (mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_MBOXED))
+        action = ACT_MBOX;
+      else if (class == MBX_SYSTEM)
+        {
+          if (mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_SHOWN
+                                              | MAIL_ATTRIBUTE_TOUCHED))
+            action = hold ? ACT_KEEP : ACT_MBOX;
+          else if (mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_SAVED))
+            {
+              if (keepsave)
+                action = hold ? ACT_KEEP : ACT_MBOX;
+              else
+                action = ACT_DELETE;
+            }
+        }
 
-      if (!is_user_mbox
-          && (mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_MBOXED)
-              || (!hold
-                  && !mu_attribute_is_deleted (attr)
-                  && !mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_PRESERVED)
-                  && ((mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_SAVED)
-                       && keepsave)
-                      || (!mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_SAVED)
-                          && (mu_attribute_is_userflag (attr,
-                                                        MAIL_ATTRIBUTE_SHOWN)
-                              || mu_attribute_is_userflag (attr,
-                                                   MAIL_ATTRIBUTE_TOUCHED)))))))
+      switch (action)
         {
-          int status;
-          
+        case ACT_KEEP:
+          if (mu_attribute_is_read (attr))
+            mu_attribute_set_seen (attr);
+          break;
+
+        case ACT_MBOX:
           if (!dest_mbox)
             {
               char *name = getenv ("MBOX");
@@ -139,15 +172,12 @@ mail_mbox_commit ()
               mu_attribute_set_deleted (attr);
               saved_count++;
             }
+          break;
+
+        case ACT_DELETE:
+          mu_attribute_set_deleted (attr);
+          break;
         }
-      else if (mu_attribute_is_deleted (attr))
-        /* Skip this one */;
-      else if (!keepsave
-               && !mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_PRESERVED)
-               && mu_attribute_is_userflag (attr, MAIL_ATTRIBUTE_SAVED))
-        mu_attribute_set_deleted (attr);
-      else if (mu_attribute_is_read (attr))
-        mu_attribute_set_seen (attr);
     }
 
   if (saved_count)
diff --git a/mail/tests/Makefile.am b/mail/tests/Makefile.am
index 867ca1856..bf3762f7f 100644
--- a/mail/tests/Makefile.am
+++ b/mail/tests/Makefile.am
@@ -49,6 +49,7 @@ TESTSUITE_AT =\
  copy03.at\
  copy04.at\
  nohome.at\
+ hold.at\
  testsuite.at\
  version.at
 
diff --git a/mail/tests/atlocal.in b/mail/tests/atlocal.in
index f8b2cc84d..80e3ea021 100644
--- a/mail/tests/atlocal.in
+++ b/mail/tests/atlocal.in
@@ -6,3 +6,6 @@ PATH=@abs_builddir@:@abs_top_builddir@/mail:$top_srcdir:$srcdir:$PATH
 testsuitedir=@abs_top_srcdir@/testsuite
 MALLOC_CHECK_=2
 export MALLOC_CHECK_
+catmbox() {
+    sed -e /^X-IMAPbase:/d -e /^X-UID:/d $1
+}
diff --git a/mail/tests/copy00.at b/mail/tests/copy00.at
index ae003dd01..21f60d574 100644
--- a/mail/tests/copy00.at
+++ b/mail/tests/copy00.at
@@ -27,7 +27,7 @@ test -f /dev/stdout || AT_SKIP_TEST
 MUT_MBCOPY($abs_top_srcdir/testsuite/spool/mbox)
 echo 'copy 1 /dev/stdout' | \
   MUT_MAIL_CMD -N -E 'set readonly' -f ./mbox 2>err | \
-  sed 's/  *$//;/^Held 1 message/d'
+  sed -e 's/  *$//' -e '/^Held 1 message/d'
 
 if test -s err; then
   # On OSX. /dev/stdout cannot be locked.
diff --git a/mail/tests/hold.at b/mail/tests/hold.at
new file mode 100644
index 000000000..1bf2531e8
--- /dev/null
+++ b/mail/tests/hold.at
@@ -0,0 +1,349 @@
+# This file is part of GNU Mailutils. -*- Autotest -*-
+# Copyright (C) 2015-2019 Free Software Foundation, Inc.
+#
+# GNU Mailutils is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 3, or (at
+# your option) any later version.
+#
+# GNU Mailutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU