[PATCH 0/3 v3] lib: Maildir flags synchronization fixes

[PATCH 0/3 v3] lib: Maildir flags synchronization fixes

[Louis Rilling]

Hello Carl,

Here is the updated series of fixes I have around maildir flags
synchronization. The first two patches are just cleanups that can be applied
independently.

The intent for the third patch (detailed in the commit log) is to allow mutt
users to keep using the "new" status, as long as notmuch can respect the
maildir specification.

Changelog:
* v3: Added patch to kill the last usage of C++ type bool
* v2: Fix bool type as well as NULL returned despite having no errors (Austin
      Clements)

Thanks,

Louis

Louis Rilling (3):
  lib: Kill last usage of C++ type bool
  tags_to_maildir_flags: Cleanup double assignement
  tags_to_maildir_flags: Don't rename if no flags change

 lib/message.cc |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] lib: Kill last usage of C++ type bool

[Louis Rilling]

Signed-off-by: Louis Rilling <l.rilling@av7.net>
---
 lib/message.cc |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index d993cde..cf651e5 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -49,16 +49,16 @@ struct visible _notmuch_message {
 struct maildir_flag_tag {
     char flag;
     const char *tag;
-    bool inverse;
+    notmuch_bool_t inverse;
 };
 
 /* ASCII ordered table of Maildir flags and associated tags */
 static struct maildir_flag_tag flag2tag[] = {
-    { 'D', "draft",   false},
-    { 'F', "flagged", false},
-    { 'P', "passed",  false},
-    { 'R', "replied", false},
-    { 'S', "unread",  true }
+    { 'D', "draft",   FALSE},
+    { 'F', "flagged", FALSE},
+    { 'P', "passed",  FALSE},
+    { 'R', "replied", FALSE},
+    { 'S', "unread",  TRUE }
 };
 
 /* We end up having to call the destructor explicitly because we had
-- 
1.7.2.5

Re: [PATCH 1/3] lib: Kill last usage of C++ type bool

[Felipe Contreras]

On Tue, Jul 12, 2011 at 4:59 PM, Louis Rilling <l.rilling@av7.net> wrote:
> Signed-off-by: Louis Rilling <l.rilling@av7.net>
> ---
>  lib/message.cc |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index d993cde..cf651e5 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -49,16 +49,16 @@ struct visible _notmuch_message {
>  struct maildir_flag_tag {
>     char flag;
>     const char *tag;
> -    bool inverse;
> +    notmuch_bool_t inverse;

That's not C++, that's C99.

-- 
Felipe Contreras

Re: [PATCH 1/3] lib: Kill last usage of C++ type bool

[Austin Clements]

Quoth Felipe Contreras on Jul 16 at  9:06 pm:
> On Tue, Jul 12, 2011 at 4:59 PM, Louis Rilling <l.rilling@av7.net> wrote:
> > Signed-off-by: Louis Rilling <l.rilling@av7.net>
> > ---
> >  lib/message.cc |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/message.cc b/lib/message.cc
> > index d993cde..cf651e5 100644
> > --- a/lib/message.cc
> > +++ b/lib/message.cc
> > @@ -49,16 +49,16 @@ struct visible _notmuch_message {
> >  struct maildir_flag_tag {
> >     char flag;
> >     const char *tag;
> > -    bool inverse;
> > +    notmuch_bool_t inverse;
> 
> That's not C++, that's C99.

Here it's C++.  But what's actually interesting is that this is the
only place where libnotmuch strays from completely consistent use of
notmuch_bool_t.

[PATCH 2/3] tags_to_maildir_flags: Cleanup double assignement

[Louis Rilling]

The for loop right after already does the job.

Signed-off-by: Louis Rilling <l.rilling@av7.net>
---
 lib/message.cc |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index cf651e5..b1b2942 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1172,8 +1172,6 @@ _new_maildir_filename (void *ctx,
     if (info == NULL) {
 	info = filename + strlen(filename);
     } else {
-	flags = info + 3;
-
 	/* Loop through existing flags in filename. */
 	for (flags = info + 3, last_flag = 0;
 	     *flags;
-- 
1.7.2.5

[PATCH 3/3] tags_to_maildir_flags: Don't rename if no flags change

[Louis Rilling]

notmuch_message_tags_to_maildir_flags() unconditionally moves messages from
maildir directory "new/" to maildir directory "cur/", which makes messages lose
their "new" status in the MUA. However some users want to keep this "new"
status after, for instance, an auto-tagging of new messages.

However, as Austin mentioned and according to the maildir specification,
messages living in "new/" are not allowed to have flags, even if mutt allows it
to happen. For this reason, this patch prevents moving messages from "new/" to
"cur/", only if no flags have to be changed. It's hopefully enough to satisfy
mutt (and maybe other MUAs showing the "new" status) users checking the "new"
status.

Changelog:
* v2: Fix bool type as well as NULL returned despite having no errors (Austin
      Clements)

Signed-off-by: Louis Rilling <l.rilling@av7.net>
---
 lib/message.cc |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index b1b2942..c003729 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1139,7 +1139,7 @@ _get_maildir_flag_actions (notmuch_message_t *message,
  * compute the new maildir filename.
  *
  * If the existing filename is in the directory "new", the new
- * filename will be in the directory "cur".
+ * filename will be in the directory "cur", unless no flags are changed.
  *
  * After a sequence of ":2," in the filename, any subsequent
  * single-character flags will be added or removed according to the
@@ -1162,6 +1162,7 @@ _new_maildir_filename (void *ctx,
     char *filename_new, *dir;
     char flag_map[128];
     int flags_in_map = 0;
+    notmuch_bool_t flags_changed = FALSE;
     unsigned int i;
     char *s;
 
@@ -1202,6 +1203,7 @@ _new_maildir_filename (void *ctx,
 	if (flag_map[flag] == 0) {
 	    flag_map[flag] = 1;
 	    flags_in_map++;
+	    flags_changed = TRUE;
 	}
     }
 
@@ -1210,9 +1212,17 @@ _new_maildir_filename (void *ctx,
 	if (flag_map[flag]) {
 	    flag_map[flag] = 0;
 	    flags_in_map--;
+	    flags_changed = TRUE;
 	}
     }
 
+    /* No need to rename. Messages in new/ can be kept in new/.
+     * Note: We don't even try to fix buggy messages having flags and living in
+     * new/. It's not our business.
+     */
+    if (!flags_changed)
+	return talloc_strdup (ctx, filename);
+
     filename_new = (char *) talloc_size (ctx,
 					 info - filename +
 					 strlen (":2,") + flags_in_map + 1);
-- 
1.7.2.5

[PATCH] test: Adding non-maildir tags does not move message from new to cur

[Michal Sojka]

From: Michal Sojka <sojka@os.inf.tu-dresden.de>

This adds a test for patch submitted by Louis Rilling. Without his patch
applied this test fails.
---
 test/maildir-sync |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/test/maildir-sync b/test/maildir-sync
index a60854f..e1ad81c 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -88,6 +88,12 @@ test_expect_equal "$output" "No new mail."
 # creating new directories in the mail store, then it should be
 # creating all necessary database state for those directories.
 
+test_begin_subtest "Adding non-maildir tags does not move message from new to cur"
+add_message [subject]='"Message to stay in new"' [date]='"Sat, 01 Jan 2000 12:00:00 -0000"' [filename]='message-to-stay-in-new' [dir]=new
+notmuch tag +donotmove subject:"Message to stay in new"
+output=$(cd "$MAIL_DIR"; ls */message-to-stay-in-new*)
+test_expect_equal "$output" "new/message-to-stay-in-new"
+
 test_begin_subtest "Removing 'S' flag from existing filename adds 'unread' tag"
 add_message [subject]='"Removing S flag"' [filename]='removing-s-flag:2,S' [dir]=cur
 output=$(notmuch search subject:"Removing S flag" | notmuch_search_sanitize)
-- 
1.7.5.4

Re: [PATCH] test: Adding non-maildir tags does not move message from new to cur

[Louis Rilling]

On 14/07/11  2:06 +0200, Michal Sojka wrote:
> From: Michal Sojka <sojka@os.inf.tu-dresden.de>
> 
> This adds a test for patch submitted by Louis Rilling. Without his patch
> applied this test fails.

Thanks a lot Michal! I was indeed wondering how to do this. This test perfectly matches the intended usage.

Louis

> ---
>  test/maildir-sync |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/test/maildir-sync b/test/maildir-sync
> index a60854f..e1ad81c 100755
> --- a/test/maildir-sync
> +++ b/test/maildir-sync
> @@ -88,6 +88,12 @@ test_expect_equal "$output" "No new mail."
>  # creating new directories in the mail store, then it should be
>  # creating all necessary database state for those directories.
>  
> +test_begin_subtest "Adding non-maildir tags does not move message from new to cur"
> +add_message [subject]='"Message to stay in new"' [date]='"Sat, 01 Jan 2000 12:00:00 -0000"' [filename]='message-to-stay-in-new' [dir]=new
> +notmuch tag +donotmove subject:"Message to stay in new"
> +output=$(cd "$MAIL_DIR"; ls */message-to-stay-in-new*)
> +test_expect_equal "$output" "new/message-to-stay-in-new"
> +
>  test_begin_subtest "Removing 'S' flag from existing filename adds 'unread' tag"
>  add_message [subject]='"Removing S flag"' [filename]='removing-s-flag:2,S' [dir]=cur
>  output=$(notmuch search subject:"Removing S flag" | notmuch_search_sanitize)
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch