[PATCH 0/4 v4] lib: Maildir flags synchronization fixes

[PATCH 0/4 v4] lib: Maildir flags synchronization fixes

[Louis Rilling]

Hello,

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 fourth 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.

The third patch implements a test for the new desired behavior. From recent
discussions I decided to put it before the actual changes, but it certainly can
move after if this is preferred.

Changelog:
* v4: 
 - rebased on top of release 0.8
 - included the test contributed by Michal Sojka
* 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

Michal Sojka (1):
      test: Adding non-maildir tags does not move message from new to cur

 lib/message.cc    |   26 +++++++++++++++++---------
 test/maildir-sync |    6 ++++++
 2 files changed, 23 insertions(+), 9 deletions(-)

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

[Louis Rilling]

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>
---
 test/maildir-sync |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/test/maildir-sync b/test/maildir-sync
index a60854f..b3e90ae 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_failure "$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.2.5

[PATCH 4/4] 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)
* v4: Tag the related test (contributed by Michal Sojka) as working

Signed-off-by: Louis Rilling <l.rilling@av7.net>
---
 lib/message.cc    |   12 +++++++++++-
 test/maildir-sync |    2 +-
 2 files changed, 12 insertions(+), 2 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);
diff --git a/test/maildir-sync b/test/maildir-sync
index b3e90ae..e1ad81c 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -92,7 +92,7 @@ test_begin_subtest "Adding non-maildir tags does not move message from new to cu
 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_failure "$output" "new/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
-- 
1.7.2.5

Re: [PATCH 2/4] tags_to_maildir_flags: Cleanup double assignement

[David Bremner]

On Thu, 15 Sep 2011 00:23:19 +0200, Louis Rilling <l.rilling@av7.net> wrote:
> The for loop right after already does the job.
> 

I pushed the first two patches in this series; the second two need to be
updated for the new "broken test" framework, and reviewed.

d

Re: [PATCH 0/4 v4] lib: Maildir flags synchronization fixes

[Daniel]

On Thu, Sep 15, 2011 at 12:23:17AM +0200, Louis Rilling wrote:
> The intent for the fourth 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.
> 
> The third patch implements a test for the new desired behavior. From recent
> discussions I decided to put it before the actual changes, but it certainly can
> move after if this is preferred.

On Mon, Nov 21, 2011 at 08:41:42PM -0400, David Bremner wrote:
> I pushed the first two patches in this series; the second two need to be
> updated for the new "broken test" framework, and reviewed.

And old one... What came of this patch?

I came across it, having been annoyed by the fact that notmuch broke my reading
habits by marking messages (in the mutt sense) as seen, which I hadn't really
seen yet in mutt. I tried out the patch; it applied and seems to work. What can
I do to make it be applied? Other than giving it some attention. I must say
that I have zero insight in the test framework, but if that's what's needed
from me...

Re: [PATCH 0/4 v4] lib: Maildir flags synchronization fixes

[David Bremner]

Daniel <quite@hack.org> writes:

> On Mon, Nov 21, 2011 at 08:41:42PM -0400, David Bremner wrote:
>> I pushed the first two patches in this series; the second two need to be
>> updated for the new "broken test" framework, and reviewed.
>
> And old one... What came of this patch?
>
> I came across it, having been annoyed by the fact that notmuch broke my reading
> habits by marking messages (in the mutt sense) as seen, which I hadn't really
> seen yet in mutt. I tried out the patch; it applied and seems to work. What can
> I do to make it be applied? Other than giving it some attention. I must say
> that I have zero insight in the test framework, but if that's what's needed
> from me...

The patches need to be reviewed for correctness and style. Typically any
non-trivial patch (or series) needs two reviews. You can see examples in
the archives of how this works.  They are currently marked stale, which
is supposed to mean they no long apply. On the other hand, if they apply
for you, maybe that tagging is wrong (see http://notmuchmail.org/nmbug/
for more information about patch tagging).

By the way, it seems like commit b88030b might be relevant.

d

Re: [PATCH 0/4 v4] lib: Maildir flags synchronization fixes

[Daniel]

> > I came across it, having been annoyed by the fact that notmuch broke my reading
> > habits by marking messages (in the mutt sense) as seen, which I hadn't really
> > seen yet in mutt. I tried out the patch; it applied and seems to work. What can
> > I do to make it be applied? Other than giving it some attention. I must say
> > that I have zero insight in the test framework, but if that's what's needed
> > from me...
> 
> The patches need to be reviewed for correctness and style. Typically any
> non-trivial patch (or series) needs two reviews. You can see examples in
> the archives of how this works.  They are currently marked stale, which
> is supposed to mean they no long apply. On the other hand, if they apply
> for you, maybe that tagging is wrong (see http://notmuchmail.org/nmbug/
> for more information about patch tagging).

I'm completely new to the notmuch core (but have been a user of the alot and
emacs client for a while), and couldn't really say that they are completely
fine.

But they do apply (with some offset) to current master. They work, seem to do
what they say--and the test runs OK.

Thus I have now (after having been given access by David) removed
notmuch::stale and added notmuch::needs-review on the following:

2011-09-15 Louis Rilling
id:"1316039001-32602-4-git-send-email-l.rilling@av7.net"
  [PATCH 3/4] test: Adding non-maildir tags does not move message
id:"1316039001-32602-5-git-send-email-l.rilling@av7.net"
  [PATCH 4/4] tags_to_maildir_flags: Don't rename if no flags change

And reviewers?

[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