[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

[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

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] test: Adding non-maildir tags does not move message from new to cur

[david]

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

Some MUA's like mutt show the difference between "new" emails living in maildir
directory new/, and "old" emails living in maildir directory cur/. However
notmuch tag unconditionally moves selected messages from new/ to cur/, even if
no maildir synchronized tag is changed.

While maildir specification forbids messages with tags living in new/, there is
no need to move messages to cur/ when no maildir synchronized tag is changed.
Thus notmuch can remain transparent with respect to other MUA's.

[ Edited commit log to better describe the intended changes, and tag the
  test as broken until the actual changes are implemented -- Louis Rilling ]

Signed-off-by: Louis Rilling <l.rilling@av7.net>

[ Converted to use test_subtest_known_broken, David Bremner ]
---

Do we agree that the behaviour of moving messages to ./cur on tagging
is broken? If so, maybe it's worth tidying up and applying this.  The
use of cd and ls strikes me as slightly suspect, but I welcome other
opinions.

 test/maildir-sync |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/maildir-sync b/test/maildir-sync
index 0fc742a..6165782 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -83,6 +83,15 @@ 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"
+test_subtest_known_broken
+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.10.4

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

[Jani Nikula]

On Sat, 08 Dec 2012, david@tethera.net wrote:
> From: Michal Sojka <sojka@os.inf.tu-dresden.de>
>
> Some MUA's like mutt show the difference between "new" emails living in maildir
> directory new/, and "old" emails living in maildir directory cur/. However
> notmuch tag unconditionally moves selected messages from new/ to cur/, even if
> no maildir synchronized tag is changed.
>
> While maildir specification forbids messages with tags living in new/, there is
> no need to move messages to cur/ when no maildir synchronized tag is changed.
> Thus notmuch can remain transparent with respect to other MUA's.
>
> [ Edited commit log to better describe the intended changes, and tag the
>   test as broken until the actual changes are implemented -- Louis Rilling ]
>
> Signed-off-by: Louis Rilling <l.rilling@av7.net>
>
> [ Converted to use test_subtest_known_broken, David Bremner ]
> ---
>
> Do we agree that the behaviour of moving messages to ./cur on tagging
> is broken? If so, maybe it's worth tidying up and applying this.  The
> use of cd and ls strikes me as slightly suspect, but I welcome other
> opinions.

I think I would narrow down the special case a bit: I think messages in
./new that have no maildir flags, and have no ":2," in the end of the
filename, and and the tag change(s) will not affect maildir flags,
should stay in ./new. Files in ./new should not have ":2," or maildir
flags, and I see no reason to support having them there.

Thus any messages in ./new that do have maildir flags, or have ":2," in
the end of the filename should probably be moved to ./cur, even if the
tag change(s) do not affect maildir flags. The patch in this thread
fails here. It also changes the behaviour for messages in ./cur by not
appending ":2," to them.

As to the test, I think it should do something along the lines of (based
on search-output test):

notmuch search --output=files subject:"Message to stay in new" | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
cat <<EOF >EXPECTED
MAIL_DIR/new/message-to-stay-in-new
EOF
test_expect_equal_file OUTPUT EXPECTED

And would be nice to have similar tests for the other things I mentioned
above. If people agree with narrowing down the special case as I
suggest, that is.


BR,
Jani.

>
>  test/maildir-sync |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/test/maildir-sync b/test/maildir-sync
> index 0fc742a..6165782 100755
> --- a/test/maildir-sync
> +++ b/test/maildir-sync
> @@ -83,6 +83,15 @@ 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"
> +test_subtest_known_broken
> +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.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Michal Sojka]

Hello Jani,

On Sat, Dec 08 2012, Jani Nikula wrote:
> On Sat, 08 Dec 2012, david@tethera.net wrote:
>> From: Michal Sojka <sojka@os.inf.tu-dresden.de>
>>
>> Some MUA's like mutt show the difference between "new" emails living in maildir
>> directory new/, and "old" emails living in maildir directory cur/. However
>> notmuch tag unconditionally moves selected messages from new/ to cur/, even if
>> no maildir synchronized tag is changed.
>>
>> While maildir specification forbids messages with tags living in new/, there is
>> no need to move messages to cur/ when no maildir synchronized tag is changed.
>> Thus notmuch can remain transparent with respect to other MUA's.
>>
>> [ Edited commit log to better describe the intended changes, and tag the
>>   test as broken until the actual changes are implemented -- Louis Rilling ]
>>
>> Signed-off-by: Louis Rilling <l.rilling@av7.net>
>>
>> [ Converted to use test_subtest_known_broken, David Bremner ]
>> ---
>>
>> Do we agree that the behaviour of moving messages to ./cur on tagging
>> is broken? If so, maybe it's worth tidying up and applying this.  The
>> use of cd and ls strikes me as slightly suspect, but I welcome other
>> opinions.
>
> I think I would narrow down the special case a bit: I think messages in
> ./new that have no maildir flags, and have no ":2," in the end of the
> filename, and and the tag change(s) will not affect maildir flags,
> should stay in ./new. Files in ./new should not have ":2," or maildir
> flags, and I see no reason to support having them there.
>
> Thus any messages in ./new that do have maildir flags, or have ":2," in
> the end of the filename should probably be moved to ./cur, even if the
> tag change(s) do not affect maildir flags. The patch in this thread
> fails here. It also changes the behaviour for messages in ./cur by not
> appending ":2," to them.

I agree with you. In
id:1355952747-27350-1-git-send-email-sojkam1@fel.cvut.cz I sent the
tests for the cases descried above as well as the updated patch for tag
to maildir synchronization.

> As to the test, I think it should do something along the lines of (based
> on search-output test):
>
> notmuch search --output=files subject:"Message to stay in new" | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
> cat <<EOF >EXPECTED
> MAIL_DIR/new/message-to-stay-in-new
> EOF
> test_expect_equal_file OUTPUT EXPECTED

With this you test what notmuch thinks about the file names of messages,
not whether the files have actually been renamed. For this reason I kept
the previous way of testing in the new patches.

Cheers,
-Michal