Maildir-flags synchronization now on master branch

Maildir-flags synchronization now on master branch

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

Thanks to work by Michal, and a few followups by me, I'm now happy to
report that the patches for synchronizing tags with maildir flags are
now pushed out to the master branch.

I've done a fair bit of testing with this stuff, and I think it's ready
to be used. In fact, I've now got the synchronization turned on by
default.

Please, anyone that cares to test it, let me know how it goes.

There were two things I had wanted to do before pushing this out, but
I've decided to get some sleep now instead:

  1. Don't do maildir-flag things in non-maildir directories

  2. Preserve any unsupported flags when renaming

Of these, number 1 is likely less important. If someone is very
concerned about avoiding the maildir-style renaming then they can simply
configure maildir.synchronize_flags to false.

But this second feature is essential, I think. People may have valuable
information encoded in their mail store as maildir flags, and it's not
right of notmuch to destroy that, (particularly as notmuch has
previously treated the mailstore in an entirely read-only fashion).

So, be warned! If you do have precious information encoded as maildir
flags, (other than D,F,P,R, or S), then you'll want to avoid using the
latest notmuch or be sure to do:

	notmuch config set maildir.synchronize_flags false

Meanwhile, as soon as we fix this one misfeature, I think I'd like to
push out what we've got now as notmuch 0.5, (perhaps with a few of the
very recent, tiny changes that have been proposed).

Thanks for the help, everyone!

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Maildir-flags synchronization now on master branch

[Kristoffer Ström]

 
>   1. Don't do maildir-flag things in non-maildir directories
 
>   2. Preserve any unsupported flags when renaming
> 
> Of these, number 1 is likely less important. If someone is very
> concerned about avoiding the maildir-style renaming then they can simply
> configure maildir.synchronize_flags to false.

I'm just pinging that the small feedreader sluk i'm writing/maintaining relies on filenames not changing. 
So if anyone's using it be careful, and #1 would be a tad more important then.

Kristoffer

Re: Maildir-flags synchronization now on master branch

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Thu, 11 Nov 2010 15:11:23 +0100, Kristoffer Ström <kristoffer@rymdkoloni.se> wrote:
> >   1. Don't do maildir-flag things in non-maildir directories
...
> I'm just pinging that the small feedreader sluk i'm writing/maintaining relies on filenames not changing. 
> So if anyone's using it be careful, and #1 would be a tad more important then.

Hi Kristoffer,

I really like the idea of sluk, and I don't want to make it painful to
use. So that's for pointing out this problem.

I've now pushed a change that will avoid doing any renaming except in
directories named either "new" or "cur".

That's not the strongest heuristic for detecting maildir, (we could
actually look and see if all three of "cur", "new", and "tmp"
directories exist, for example). But it's at least a very cheap
heuristic. So hopefully this will help for your use case.

And obviously, one can still configure the maildir synchronization off
(globally) if desired.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Maildir-flags synchronization now on master branch

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Thu, 11 Nov 2010 05:04:48 -0800, Carl Worth <cworth@cworth.org> wrote:
> There were two things I had wanted to do before pushing this out, but
> I've decided to get some sleep now instead:
> 
>   1. Don't do maildir-flag things in non-maildir directories
> 
>   2. Preserve any unsupported flags when renaming

After some good sleep, both of these features seemed much easier to
implement. These are both pushed now.

And I've now done a much more thorough audit of all of the
implementation, (adapting things to my preferred coding style, adding
comments, fixing up little corner cases, etc.). So I'm quite comfortable
with all of this now. I think 0.5 will be imminent.

Now that all of this maildir-flag synchronization is possible, I wonder
if we shouldn't allow the user to configure the mapping of maildir-flag
characters to tag names. That would allow for (a limited number of) tags
to be synchronized on multiple machines using synchronization mechanisms
such as offlineimap without needing any notmuch-aware synchronization.

So that might be very interesting.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Maildir-flags synchronization now on master branch

[Dirk Hohndel]

On Thu, 11 Nov 2010 16:43:45 -0800, Carl Worth <cworth@cworth.org> wrote:
> 
> Now that all of this maildir-flag synchronization is possible, I wonder
> if we shouldn't allow the user to configure the mapping of maildir-flag
> characters to tag names. That would allow for (a limited number of) tags
> to be synchronized on multiple machines using synchronization mechanisms
> such as offlineimap without needing any notmuch-aware synchronization.
> 
> So that might be very interesting.

Oh I LOVE that idea. There are only a small number of tags that I
/really/ need and to be able to access them from multiplme machines,
kept in sync through the imap server... awesome.

/D

Re: Maildir-flags synchronization now on master branch

[Ruben Pollan]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On 05:04, Thu 11 Nov 10, Carl Worth wrote:
> Thanks to work by Michal, and a few followups by me, I'm now happy to
> report that the patches for synchronizing tags with maildir flags are
> now pushed out to the master branch.

Nice to see that, I was waiting for this feature since a while.

I only find a problem with it. The 'unread' tag is not working as I expected.
notmuch only tags as 'unread' my email at 'cur' folder. fetchmail download the
email to the 'new' folder. As I understand that is a correct behaviour[0].


[0] http://cr.yp.to/proto/maildir.html

-- 
Rubén Pollán  | jabber:meskio@jabber.org
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
               Hay un mundo
  a la vuelta de la esquina de tu mente,
     donde la realidad es un intruso
      y los sueños se hacen realidad.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] Including 'unread' tag to mails without maildir flags

[meskio]

From: Ruben Pollan <meskio@sindominio.net>

Some mail fetchers, like fetchmail, leaves the email without ':2,' at the end of
the filename. Notmuch didn't detect this emails as maildir, it didn't add the
maildir flags to them.

Now it detects if a mail is in a maildir by the directory structure, and add its
maildir flags correctly.
---
 lib/message.cc |   87 +++++++++++++++++++++++++++++---------------------------
 1 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 225b7e9..996c1df 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -854,6 +854,47 @@ notmuch_message_remove_tag (notmuch_message_t *message, const char *tag)
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/* Is the given filename within a maildir directory?
+ *
+ * Specifically, is the final directory component of 'filename' either
+ * "cur" or "new". If so, return a pointer to that final directory
+ * component within 'filename'. If not, return NULL.
+ *
+ * A non-NULL return value is guaranteed to be a valid string pointer
+ * pointing to the characters "new/" or "cur/", (but not
+ * NUL-terminated).
+ */
+static const char *
+_filename_is_in_maildir (const char *filename)
+{
+    const char *slash, *dir = NULL;
+
+    /* Find the last '/' separating directory from filename. */
+    slash = strrchr (filename, '/');
+    if (slash == NULL)
+	return NULL;
+
+    /* Jump back 4 characters to where the previous '/' will be if the
+     * directory is named "cur" or "new". */
+    if (slash - filename < 4)
+	return NULL;
+
+    slash -= 4;
+
+    if (*slash != '/')
+	return NULL;
+
+    dir = slash + 1;
+
+    if (STRNCMP_LITERAL (dir, "cur/") == 0 ||
+	STRNCMP_LITERAL (dir, "new/") == 0)
+    {
+	return dir;
+    }
+
+    return NULL;
+}
+
 notmuch_status_t
 notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
 {
@@ -871,11 +912,14 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
     {
 	filename = notmuch_filenames_get (filenames);
 
+	if (! _filename_is_in_maildir(filename))
+	    continue;
+	seen_maildir_info = 1;
+
 	flags = strstr (filename, ":2,");
 	if (! flags)
 	    continue;
 
-	seen_maildir_info = 1;
 	flags += 3;
 
 	combined_flags = talloc_strdup_append (combined_flags, flags);
@@ -910,47 +954,6 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
     return status;
 }
 
-/* Is the given filename within a maildir directory?
- *
- * Specifically, is the final directory component of 'filename' either
- * "cur" or "new". If so, return a pointer to that final directory
- * component within 'filename'. If not, return NULL.
- *
- * A non-NULL return value is guaranteed to be a valid string pointer
- * pointing to the characters "new/" or "cur/", (but not
- * NUL-terminated).
- */
-static const char *
-_filename_is_in_maildir (const char *filename)
-{
-    const char *slash, *dir = NULL;
-
-    /* Find the last '/' separating directory from filename. */
-    slash = strrchr (filename, '/');
-    if (slash == NULL)
-	return NULL;
-
-    /* Jump back 4 characters to where the previous '/' will be if the
-     * directory is named "cur" or "new". */
-    if (slash - filename < 4)
-	return NULL;
-
-    slash -= 4;
-
-    if (*slash != '/')
-	return NULL;
-
-    dir = slash + 1;
-
-    if (STRNCMP_LITERAL (dir, "cur/") == 0 ||
-	STRNCMP_LITERAL (dir, "new/") == 0)
-    {
-	return dir;
-    }
-
-    return NULL;
-}
-
 /* From the set of tags on 'message' and the flag2tag table, compute a
  * set of maildir-flag actions to be taken, (flags that should be
  * either set or cleared).
-- 
1.7.1

Re: [PATCH] Including 'unread' tag to mails without maildir flags

[Michal Sojka]

On Fri, 19 Nov 2010, meskio@sindominio.net wrote:
> Some mail fetchers, like fetchmail, leaves the email without ':2,' at the end of
> the filename. Notmuch didn't detect this emails as maildir, it didn't add the
> maildir flags to them.
> 
> Now it detects if a mail is in a maildir by the directory structure, and add its
> maildir flags correctly.

Hi Ruben,

your patch fixes the problem, but it also breaks one of the tests in
test suite. I created the test for your patch and attempted to fix the
broken test. See the replies to this message.

-Michal

[PATCH 1/2] test: Add test for messages without maildir info in new

[Michal Sojka]

A message in new without maildir info should be tagged the same as if
empty info (:,2) is present, i.e. unread tag must be present.

To test this behavior, notmuch new must be configured to add only inbox
tag and not two tags inbox and unread, which is the default.
---
 test/maildir-sync |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/test/maildir-sync b/test/maildir-sync
index 50a5b8e..4419a7e 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -4,6 +4,11 @@ test_description="maildir synchronization"
 
 . ./test-lib.sh
 
+cat <<EOF >>"${NOTMUCH_CONFIG}"
+[new]
+tags=inbox;
+EOF
+
 # Much easier to examine differences if the "notmuch show
 # --format=json" output includes some newlines. Also, need to avoid
 # including the local value of MAIL_DIR in the result.
@@ -175,4 +180,9 @@ add_message [subject]='"Non-compliant maildir info"' [dir]=cur [filename]='non-c
 notmuch tag +unread +draft -flagged subject:"Non-compliant maildir info"
 test_expect_equal "$(cd $MAIL_DIR/cur/; ls non-compliant*)" "non-compliant-maildir-info:2,These-are-not-flags-in-ASCII-order-donottouch"
 
+test_begin_subtest "Message in new without maildir info is tagges as unread"
+add_message [subject]='"Message in new without maildir info"' [date]='"Sat, 01 Jan 2000 12:00:00 -0000"' [filename]='message-in-new-without-maildir-info' [dir]=new
+output=$(notmuch search subject:"Message in new without maildir info" | notmuch_search_sanitize)
+test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; Message in new without maildir info (inbox unread)"
+
 test_done
-- 
1.7.1.90.g94d6d4

[PATCH 2/2] Include 'unread' tag only to mails in new

[Michal Sojka]

Ruben's previous patch fixed the problem that he experienced, but it
also caused the test "Removing info from filename leaves tags unchanged"
to fail. This is an attempt to make all tests to pass.

The Ruben's change is restricted to be only effective in 'new' directory.
---
 lib/message.cc |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 996c1df..e769bf5 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -905,6 +905,7 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
     char *combined_flags = talloc_strdup (message, "");
     unsigned i;
     int seen_maildir_info = 0;
+    const char *dir;
 
     for (filenames = notmuch_message_get_filenames (message);
 	 notmuch_filenames_valid (filenames);
@@ -912,13 +913,17 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
     {
 	filename = notmuch_filenames_get (filenames);
 
-	if (! _filename_is_in_maildir(filename))
+	dir = _filename_is_in_maildir(filename);
+	if (! dir)
 	    continue;
-	seen_maildir_info = 1;
+
+	if (STRNCMP_LITERAL (dir, "new/") == 0)
+	    seen_maildir_info = 1;
 
 	flags = strstr (filename, ":2,");
 	if (! flags)
 	    continue;
+	seen_maildir_info = 1;
 
 	flags += 3;
 
-- 
1.7.1.90.g94d6d4

[PATCH] Additional maildir-sync test

[Michal Sojka]

On Sat, 20 Nov 2010, Michal Sojka wrote:
> your patch fixes the problem, but it also breaks one of the tests in
> test suite. I created the test for your patch ...

and here is another test which was in my original synchronization
patches, but Carl decided to remove it. It checks that tagging message
by an unrelated tag does not move the message from new to cur. This test
currently fails.

In id:"20100323143714.GE28596@blackspot" Ruben wrote that for some mail
readers there is a difference between mails in new and mails without "S"
flag in cur.

Ruben, is the current behavior problematic for you?

Carl, did you have some reason to remove this functionality?

-Michal

diff --git a/test/maildir-sync b/test/maildir-sync
index 4419a7e..8da12dd 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -185,4 +185,10 @@ add_message [subject]='"Message in new without maildir info"' [date]='"Sat, 01 J
 output=$(notmuch search subject:"Message in new without maildir info" | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; Message in new without maildir info (inbox unread)"
 
+test_begin_subtest "Non-maildir-syncable tag should not move the message from new"
+add_message [subject]='"Message to stay in new after tagging"' [date]='"Sat, 01 Jan 2000 12:00:00 -0000"' [filename]='message-to-stay-in-new-after-tagging' [dir]=new
+notmuch tag +non-maildir-syncable subject:"Message to stay in new after tagging"
+output=$(cd "$MAIL_DIR"; find -name 'message-to-stay-in-new-after-tagging*')
+test_expect_equal "$output" "./new/message-to-stay-in-new-after-tagging"
+
 test_done

Re: [PATCH] Additional maildir-sync test

[Ruben]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On 00:54, Sun 21 Nov 10, Michal Sojka wrote:
> On Sat, 20 Nov 2010, Michal Sojka wrote:
> > your patch fixes the problem, but it also breaks one of the tests in
> > test suite. I created the test for your patch ...
> 
> and here is another test which was in my original synchronization
> patches, but Carl decided to remove it. It checks that tagging message
> by an unrelated tag does not move the message from new to cur. This test
> currently fails.
> 
> In id:"20100323143714.GE28596@blackspot" Ruben wrote that for some mail
> readers there is a difference between mails in new and mails without "S"
> flag in cur.
> 
> Ruben, is the current behavior problematic for you?

It's not a problem for me. For mutt (which I use often) to have it on 'new' or
'cur' don't makes any difference, if the 'S' flag is not there I see them as
'unread'. Actually in mutt if you read an email and later flags it as 'unread'
it moves the email 'cur' and puts the ':2,' without the 'S' flag.

For me is the same to have the email at 'new' or at 'cur', the only thing I
wanted is to have support to fetchmail new emails.

-- 
Rubén Pollán  | jabber:meskio@jabber.org
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Lo hago para no volverme loco cuando noto
que solo me queda un demonio en un hombro
por que se ha cortado las venas
el ángel que había en el otro.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2] Include 'unread' tag only to mails in new

[Ruben Pollan]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On 00:27, Sun 21 Nov 10, Michal Sojka wrote:
> Ruben's previous patch fixed the problem that he experienced, but it
> also caused the test "Removing info from filename leaves tags unchanged"
> to fail. This is an attempt to make all tests to pass.
> 
> The Ruben's change is restricted to be only effective in 'new' directory.

That patch works fine for me.

-- 
Rubén Pollán  | jabber:meskio@jabber.org
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Ésta es la historia de una sociedad que se hunde
y mientras cae se repite:
hasta ahora todo va bien, hasta ahora todo va bien ...
Pero lo importante no es la caida sino el aterrizaje.
                               el odio

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]