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
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GNU Mailutils.  If not, see <http://www.gnu.org/licenses/>.
+
+dnl             1      2      3       4       5 
+dnl MAIL_TEST([NAME],[MBOX],[CODE],[INPUT],[OUTPUT])
+m4_pushdef([MAIL_TEST],
+ [AT_SETUP($1)
+AT_CHECK(
+ [AT_DATA([inbox],[$2])
+AT_DATA([rc],[$4])
+export MAIL=inbox
+export MBOX=mbox
+> $MBOX
+CWD=$(pwd)
+MUT_MAIL_CMD -N -E "set folder=$CWD" < rc | sed -e 's/  *$//' -e "s|$CWD/||"
+$3
+],
+[0],
+[$5])
+AT_CLEANUP])
+
+m4_pushdef([MAILBOX_CONTENT],
+[From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+
+I don't see any wine
+
+])
+
+AT_BANNER([hold and keepsave variables])
+
+MAIL_TEST([read (nohold)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+],
+[set nohold nokeepsave
+1
+quit
+],
+[From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+Saved 1 message in mbox
+Held 1 message in inbox
+== inbox ==
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+])
+
+MAIL_TEST([read (hold)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+],
+[set hold
+1
+quit
+],
+[From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+Held 2 messages in inbox
+== inbox ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+Status: OR
+
+Have some wine
+
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+])
+
+MAIL_TEST([touch (nohold)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+],
+[set nohold
+touch 1
+quit
+],
+[Saved 1 message in mbox
+Held 1 message in inbox
+== inbox ==
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+])
+
+MAIL_TEST([touch (hold)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+],
+[set hold
+touch 1
+quit
+],
+[Held 2 messages in inbox
+== inbox ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+Status: O
+
+Have some wine
+
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+])
+
+MAIL_TEST([save (nohold nokeepsave)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+echo == dest ==
+catmbox dest
+],
+[set nohold nokeepsave
+save 1 +dest
+quit
+],
+["dest"   7/152
+Held 1 message in inbox
+== inbox ==
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+== dest == 
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+])
+ 
+MAIL_TEST([save (nohold keepsave)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+echo == dest ==
+catmbox dest
+],
+[set nohold keepsave
+save 1 +dest
+quit
+],
+["dest"   7/152
+Saved 1 message in mbox
+Held 1 message in inbox
+== inbox ==
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox == 
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+== dest == 
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+]) 
+ 
+MAIL_TEST([save (hold keepsave)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+echo == dest ==
+catmbox dest
+],
+[set hold keepsave
+save 1 +dest
+quit
+],
+["dest"   7/152
+Held 2 messages in inbox
+== inbox ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+Status: O
+
+Have some wine
+
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+== dest ==
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+]) 
+
+MAIL_TEST([save (hold nokeepsave)],
+[MAILBOX_CONTENT],
+[echo == inbox ==
+catmbox inbox
+echo == mbox ==
+catmbox mbox
+echo == dest ==
+catmbox dest
+],
+[set hold nokeepsave
+save 1 +dest
+quit
+],
+["dest"   7/152
+Held 1 message in inbox
+== inbox ==
+From alice@wonder.land Mon Jul 29 22:00:09 2002
+From: Alice  <alice@wonder.land>
+To: March Hare  <hare@wonder.land>
+Subject: Re: Invitation
+Status: O
+
+I don't see any wine
+
+== mbox ==
+== dest == 
+From hare@wonder.land Mon Jul 29 22:00:08 2002
+From: March Hare  <hare@wonder.land>
+To: Alice  <alice@wonder.land>
+Subject: Invitation
+
+Have some wine
+
+])
+
+m4_popdef([MAILBOX_CONTENT])
+m4_popdef([MAIL_TEST])
diff --git a/mail/tests/testsuite.at b/mail/tests/testsuite.at
index 5e3bb2ae3..41aabb526 100644
--- a/mail/tests/testsuite.at
+++ b/mail/tests/testsuite.at
@@ -31,4 +31,5 @@ m4_include([copy02.at])
 m4_include([copy03.at])
 m4_include([copy04.at])
 m4_include([align.at])
-m4_include([D.at])
\ No newline at end of file
+m4_include([D.at])
+m4_include([hold.at])