[PATCH 0/3] Adding non-maildir tags does not move message from new to cur

[PATCH 0/3] Adding non-maildir tags does not move message from new to cur

[Michal Sojka]

Hello,

patches in this series should supersede the patches in
id:1316039001-32602-5-git-send-email-l.rilling@av7.net. They address
the comments from Jani Nikula and add tests for the behavior that was
unintentionally changed by the previous version of the patch.

Louis Rilling (1):
  tags_to_maildir_flags: Don't rename if no flags change

Michal Sojka (2):
  test: Adding non-maildir tags does not move message from new to cur
  test: Add some missing maildir synchronization tests

 lib/message.cc    |   13 ++++++++++++-
 test/maildir-sync |   20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

-- 
1.7.10.4

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

[Michal Sojka]

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

[David Bremner]

Michal Sojka <sojkam1@fel.cvut.cz> writes:

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

I pushed these three, and the three followup patches from Jani and I
(mine squashed in to avoid a commit with broken tests).

d

[PATCH 2/3] test: Add some missing maildir synchronization tests

[Michal Sojka]

As mentioned by Jani Nikula in id:87vcccp4y3.fsf@nikula.org, some cases
of maildir synchronization are not covered by our tests. Let's add the
missing tests.
---
 test/maildir-sync |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/maildir-sync b/test/maildir-sync
index 6165782..b2ac89f 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -92,6 +92,18 @@ 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 "Message in cur lacking maildir info gets one on any tag change"
+add_message [filename]='message-to-get-maildir-info' [dir]=cur
+notmuch tag +anytag id:$gen_msg_id
+output=$(cd "$MAIL_DIR"; ls */message-to-get-maildir-info*)
+test_expect_equal "$output" "cur/message-to-get-maildir-info:2,"
+
+test_begin_subtest "Message in new with maildir info is moved to cur on any tag change"
+add_message [filename]='message-with-info-to-be-moved-to-cur:2,' [dir]=new
+notmuch tag +anytag id:$gen_msg_id
+output=$(cd "$MAIL_DIR"; ls */message-with-info-to-be-moved-to-cur*)
+test_expect_equal "$output" "cur/message-with-info-to-be-moved-to-cur:2,"
+
 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

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

[Michal Sojka]

From: Louis Rilling <l.rilling@av7.net>

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>

[Condition for keeping messages in new/ was extended to satisfy all
 tests from the previous patch. -Michal Sojka]
---
 lib/message.cc    |   13 ++++++++++++-
 test/maildir-sync |    1 -
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 320901f..87369bb 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1195,7 +1195,9 @@ _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", except for the case when
+ * no flags are changed and the existing filename does not contain
+ * maildir info (starting with ",2:").
  *
  * After a sequence of ":2," in the filename, any subsequent
  * single-character flags will be added or removed according to the
@@ -1218,6 +1220,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;
 
@@ -1258,6 +1261,7 @@ _new_maildir_filename (void *ctx,
 	if (flag_map[flag] == 0) {
 	    flag_map[flag] = 1;
 	    flags_in_map++;
+	    flags_changed = TRUE;
 	}
     }
 
@@ -1266,9 +1270,16 @@ _new_maildir_filename (void *ctx,
 	if (flag_map[flag]) {
 	    flag_map[flag] = 0;
 	    flags_in_map--;
+	    flags_changed = TRUE;
 	}
     }
 
+    /* Messages in new/ without maildir info can be kept in new/ if no
+     * flags have changed. */
+    dir = (char *) _filename_is_in_maildir (filename);
+    if (dir && STRNCMP_LITERAL (dir, "new/") == 0 && !*info && !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 b2ac89f..33d2c58 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -84,7 +84,6 @@ test_expect_equal "$output" "No new mail."
 # 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
-- 
1.7.10.4

[PATCH] test: update insert tests for new maildir synchronization rules

[david]

From: David Bremner <bremner@debian.org>

As of id:1355952747-27350-4-git-send-email-sojkam1@fel.cvut.cz
we are more conservative about moving messages from ./new to ./cur.
This updates the insert tests to match
---

Hopefully this makes sense, and is not just papering over a real
problem that should be fixed.

 test/insert | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/insert b/test/insert
index 021edb6..1718120 100755
--- a/test/insert
+++ b/test/insert
@@ -82,7 +82,7 @@ gen_insert_msg
 notmuch insert --folder=Drafts < "$gen_msg_filename"
 output=$(notmuch search --output=files folder:Drafts)
 dirname=$(dirname "$output")
-test_expect_equal "$dirname" "$MAIL_DIR/Drafts/cur"
+test_expect_equal "$dirname" "$MAIL_DIR/Drafts/new"
 
 test_begin_subtest "Insert message into folder, add/remove tags"
 gen_insert_msg
@@ -99,14 +99,14 @@ gen_insert_msg
 notmuch insert --folder=F --create-folder +folder < "$gen_msg_filename"
 output=$(notmuch search --output=files folder:F tag:folder)
 basename=$(basename "$output")
-test_expect_equal_file "$gen_msg_filename" "$MAIL_DIR/F/cur/${basename}"
+test_expect_equal_file "$gen_msg_filename" "$MAIL_DIR/F/new/${basename}"
 
 test_begin_subtest "Insert message, create subfolder"
 gen_insert_msg
 notmuch insert --folder=F/G/H/I/J --create-folder +folder < "$gen_msg_filename"
 output=$(notmuch search --output=files folder:F/G/H/I/J tag:folder)
 basename=$(basename "$output")
-test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/cur/${basename}"
+test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/new/${basename}"
 
 test_begin_subtest "Insert message, create existing subfolder"
 gen_insert_msg
-- 
1.8.3.2

[PATCH 1/2] test: add more maildir flag syncing related tests to insert

[Jani Nikula]

Specifically test maildir flag syncing with insert.

---

These are additional tests on top of David's fix.
---
 test/insert | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/test/insert b/test/insert
index 1718120..9b448e5 100755
--- a/test/insert
+++ b/test/insert
@@ -77,6 +77,42 @@ notmuch insert +custom -unread < "$gen_msg_filename"
 output=$(notmuch count tag:custom NOT tag:unread)
 test_expect_equal "$output" "1"
 
+test_begin_subtest "Insert message with default tags stays in new/"
+gen_insert_msg
+notmuch insert < "$gen_msg_filename"
+output=$(notmuch search --output=files id:$gen_msg_id)
+dirname=$(dirname "$output")
+test_expect_equal "$dirname" "$MAIL_DIR/new"
+
+test_begin_subtest "Insert message with non-maildir synced tags stays in new/"
+gen_insert_msg
+notmuch insert +custom -inbox < "$gen_msg_filename"
+output=$(notmuch search --output=files id:$gen_msg_id)
+dirname=$(dirname "$output")
+test_expect_equal "$dirname" "$MAIL_DIR/new"
+
+test_begin_subtest "Insert message with custom new.tags goes to cur/"
+OLDCONFIG=$(notmuch config get new.tags)
+notmuch config set new.tags test
+gen_insert_msg
+notmuch insert < "$gen_msg_filename"
+output=$(notmuch search --output=files id:$gen_msg_id)
+dirname=$(dirname "$output")
+notmuch config set new.tags $OLDCONFIG
+test_expect_equal "$dirname" "$MAIL_DIR/cur"
+
+# additional check on the previous message
+test_begin_subtest "Insert message with custom new.tags actually gets the tags"
+output=$(notmuch search --output=tags id:$gen_msg_id)
+test_expect_equal "$output" "test"
+
+test_begin_subtest "Insert message with maildir synced tags goes to cur/"
+gen_insert_msg
+notmuch insert +flagged < "$gen_msg_filename"
+output=$(notmuch search --output=files id:$gen_msg_id)
+dirname=$(dirname "$output")
+test_expect_equal "$dirname" "$MAIL_DIR/cur"
+
 test_begin_subtest "Insert message into folder"
 gen_insert_msg
 notmuch insert --folder=Drafts < "$gen_msg_filename"
-- 
1.8.4.rc3

[PATCH 2/2] test: improve insert test reliability by checking message-id instead of count

[Jani Nikula]

There isn't a reported issue this would fix. Spotted by reading the
test.
---
 test/insert | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/insert b/test/insert
index 9b448e5..550b413 100755
--- a/test/insert
+++ b/test/insert
@@ -68,14 +68,14 @@ test_expect_equal_json "$output" '["inbox", "unread"]'
 test_begin_subtest "Insert message, add tag"
 gen_insert_msg
 notmuch insert +custom < "$gen_msg_filename"
-output=$(notmuch count tag:custom)
-test_expect_equal "$output" "1"
+output=$(notmuch search --output=messages tag:custom)
+test_expect_equal "$output" "id:$gen_msg_id"
 
 test_begin_subtest "Insert message, add/remove tags"
 gen_insert_msg
 notmuch insert +custom -unread < "$gen_msg_filename"
-output=$(notmuch count tag:custom NOT tag:unread)
-test_expect_equal "$output" "1"
+output=$(notmuch search --output=messages tag:custom NOT tag:unread)
+test_expect_equal "$output" "id:$gen_msg_id"
 
 test_begin_subtest "Insert message with default tags stays in new/"
 gen_insert_msg
@@ -123,8 +123,8 @@ test_expect_equal "$dirname" "$MAIL_DIR/Drafts/new"
 test_begin_subtest "Insert message into folder, add/remove tags"
 gen_insert_msg
 notmuch insert --folder=Drafts +draft -unread < "$gen_msg_filename"
-output=$(notmuch count folder:Drafts tag:draft NOT tag:unread)
-test_expect_equal "$output" "1"
+output=$(notmuch search --output=messages folder:Drafts tag:draft NOT tag:unread)
+test_expect_equal "$output" "id:$gen_msg_id"
 
 gen_insert_msg
 test_expect_code 1 "Insert message into non-existent folder" \
-- 
1.8.4.rc3

Re: [PATCH 1/2] test: add more maildir flag syncing related tests to insert

[David Bremner]

Jani Nikula <jani@nikula.org> writes:
>  
> +test_begin_subtest "Insert message with default tags stays in new/"
> +gen_insert_msg
> +notmuch insert < "$gen_msg_filename"
> +output=$(notmuch search --output=files id:$gen_msg_id)
> +dirname=$(dirname "$output")
> +test_expect_equal "$dirname" "$MAIL_DIR/new"

I'm not sure if this is a bug or a feature, but this enforces a
constraint on the default tags that notmuch ships enabled. Is that what
we want, or would it be better to set unread explicitely here?

d

Re: [PATCH 1/2] test: add more maildir flag syncing related tests to insert

[Jani Nikula]

On Tue, 03 Sep 2013, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>>  
>> +test_begin_subtest "Insert message with default tags stays in new/"
>> +gen_insert_msg
>> +notmuch insert < "$gen_msg_filename"
>> +output=$(notmuch search --output=files id:$gen_msg_id)
>> +dirname=$(dirname "$output")
>> +test_expect_equal "$dirname" "$MAIL_DIR/new"
>
> I'm not sure if this is a bug or a feature, but this enforces a
> constraint on the default tags that notmuch ships enabled. Is that what
> we want, or would it be better to set unread explicitely here?

We already pretty heavily depend on those default tags, so I'd say just
leave it like this. If we want to fix it later, there's some other
fixing to do too. Try this patch and 'make test':

diff --git a/notmuch-config.c b/notmuch-config.c
index 6845e3c..8362c13 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -365,8 +365,8 @@ notmuch_config_open (void *ctx,
     }
 
     if (notmuch_config_get_new_tags (config, &tmp) == NULL) {
-        const char *tags[] = { "unread", "inbox" };
-        notmuch_config_set_new_tags (config, tags, 2);
+        const char *tags[] = { "flagged" };
+        notmuch_config_set_new_tags (config, tags, 1);
     }
 
     if (notmuch_config_get_new_ignore (config, &tmp) == NULL) {

I get:

Notmuch test suite complete.
396/579 tests passed.
183 tests failed.


BR,
Jani.

Re: [PATCH] test: update insert tests for new maildir synchronization rules

[Jani Nikula]

On Tue, 03 Sep 2013, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> As of id:1355952747-27350-4-git-send-email-sojkam1@fel.cvut.cz
> we are more conservative about moving messages from ./new to ./cur.
> This updates the insert tests to match
> ---
>
> Hopefully this makes sense, and is not just papering over a real
> problem that should be fixed.

LGTM. I sent some additional tests on top.

Jani.



>
>  test/insert | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/test/insert b/test/insert
> index 021edb6..1718120 100755
> --- a/test/insert
> +++ b/test/insert
> @@ -82,7 +82,7 @@ gen_insert_msg
>  notmuch insert --folder=Drafts < "$gen_msg_filename"
>  output=$(notmuch search --output=files folder:Drafts)
>  dirname=$(dirname "$output")
> -test_expect_equal "$dirname" "$MAIL_DIR/Drafts/cur"
> +test_expect_equal "$dirname" "$MAIL_DIR/Drafts/new"
>  
>  test_begin_subtest "Insert message into folder, add/remove tags"
>  gen_insert_msg
> @@ -99,14 +99,14 @@ gen_insert_msg
>  notmuch insert --folder=F --create-folder +folder < "$gen_msg_filename"
>  output=$(notmuch search --output=files folder:F tag:folder)
>  basename=$(basename "$output")
> -test_expect_equal_file "$gen_msg_filename" "$MAIL_DIR/F/cur/${basename}"
> +test_expect_equal_file "$gen_msg_filename" "$MAIL_DIR/F/new/${basename}"
>  
>  test_begin_subtest "Insert message, create subfolder"
>  gen_insert_msg
>  notmuch insert --folder=F/G/H/I/J --create-folder +folder < "$gen_msg_filename"
>  output=$(notmuch search --output=files folder:F/G/H/I/J tag:folder)
>  basename=$(basename "$output")
> -test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/cur/${basename}"
> +test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/new/${basename}"
>  
>  test_begin_subtest "Insert message, create existing subfolder"
>  gen_insert_msg
> -- 
> 1.8.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

-- 
Jani

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

[Jani Nikula]

On Wed, 19 Dec 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> patches in this series should supersede the patches in
> id:1316039001-32602-5-git-send-email-l.rilling@av7.net. They address
> the comments from Jani Nikula and add tests for the behavior that was
> unintentionally changed by the previous version of the patch.

Michal, sorry about not following through with the series in a timely
manner. We want the feature and it looks good to me.

BR,
Jani.

[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 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