[RFC][PATCH] tags_to_maildir_flags: Add option to not move messages from "new/" to "cur/"

[RFC][PATCH] tags_to_maildir_flags: Add option to not move messages from "new/" to "cur/"

[Louis Rilling]

notmuch_message_tags_to_maildir_flags() 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.

This patch introduces notmuch_message_tags_to_maildir_flags_preserve(), which
does the same job as notmuch_message_tags_to_maildir_flags() except moving
from "maildir "new/" to maildir "cur/". A new option "preserve_new" is
introduced in "[maildir]" section of .notmuch-config, so that users can
configure whether commands "notmuch tag" and "notmuch restore" preserve the
"new" status or not.

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

I'm in the process of using notmuch, but the issue "addressed" by this patch
would make me change my habits a bit too fast. I use the "new" status for
quickly checking (often without reading) which emails I just received,
implementing some kind of context/mood/daytime-dependent quick filtering. I'd
also like to run a pre-tagging script automatically when synchronizing
periodically (and automatically too) my mailboxes. But the current behavior of
"notmuch tag" makes me lose my quick filtering ability.

This patch is mostly written for discussion. It is certainly not polished (API,
ABI, bindings) and not tested at all. In particular, I know that there are some
plans to customize flags synchronization, but I don't know how the library API
could/should be impacted.

Thanks for your comments!

Louis


 lib/message.cc    |   33 +++++++++++++++++++++++++--------
 lib/notmuch.h     |    7 +++++++
 notmuch-client.h  |    7 +++++++
 notmuch-config.c  |   35 ++++++++++++++++++++++++++++++++++-
 notmuch-restore.c |   10 ++++++++--
 notmuch-tag.c     |   10 ++++++++--
 6 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 4b59fa9..c6c4160 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1130,7 +1130,8 @@ static char*
 _new_maildir_filename (void *ctx,
 		       const char *filename,
 		       const char *flags_to_set,
-		       const char *flags_to_clear)
+		       const char *flags_to_clear,
+		       bool preserve_new)
 {
     const char *info, *flags;
     unsigned int flag, last_flag;
@@ -1211,16 +1212,19 @@ _new_maildir_filename (void *ctx,
     }
     *s = '\0';
 
-    /* If message is in new/ move it under cur/. */
-    dir = (char *) _filename_is_in_maildir (filename_new);
-    if (dir && STRNCMP_LITERAL (dir, "new/") == 0)
-	memcpy (dir, "cur/", 4);
+    if (!preserve_new) {
+	/* If message is in new/ move it under cur/. */
+	dir = (char *) _filename_is_in_maildir (filename_new);
+	if (dir && STRNCMP_LITERAL (dir, "new/") == 0)
+	    memcpy (dir, "cur/", 4);
+    }
 
     return filename_new;
 }
 
-notmuch_status_t
-notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
+static notmuch_status_t
+_notmuch_message_tags_to_maildir_flags (notmuch_message_t *message,
+					bool preserve_new)
 {
     notmuch_filenames_t *filenames;
     const char *filename;
@@ -1240,7 +1244,8 @@ notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
 	    continue;
 
 	filename_new = _new_maildir_filename (message, filename,
-					      to_set, to_clear);
+					      to_set, to_clear,
+					      preserve_new);
 	if (filename_new == NULL)
 	    continue;
 
@@ -1281,6 +1286,18 @@ notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
 }
 
 notmuch_status_t
+notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
+{
+    _notmuch_message_tags_to_maildir_flags(message, false);
+}
+
+notmuch_status_t
+notmuch_message_tags_to_maildir_flags_preserve (notmuch_message_t *message)
+{
+    _notmuch_message_tags_to_maildir_flags(message, true);
+}
+
+notmuch_status_t
 notmuch_message_remove_all_tags (notmuch_message_t *message)
 {
     notmuch_private_status_t private_status;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index e508309..eeddc17 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -978,6 +978,13 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message);
  */
 notmuch_status_t
 notmuch_message_tags_to_maildir_flags (notmuch_message_t *message);
+/* Rename message filename(s) to encode tags as maildir flags, without moving from new/ to cur/
+ *
+ * Same as notmuch_message_tags_to_maildir_flags, but messages living in
+ * directory "new" are not moved to neighboring directory "cur".
+ */
+notmuch_status_t
+notmuch_message_tags_to_maildir_flags_preserve (notmuch_message_t *message);
 
 /* Freeze the current state of 'message' within the database.
  *
diff --git a/notmuch-client.h b/notmuch-client.h
index 63be337..2372fe6 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -235,6 +235,13 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 					      notmuch_bool_t synchronize_flags);
 
 notmuch_bool_t
+notmuch_config_get_maildir_preserve_new (notmuch_config_t *config);
+
+void
+notmuch_config_set_maildir_preserve_new (notmuch_config_t *config,
+					 notmuch_bool_t preserve_new);
+
+notmuch_bool_t
 debugger_is_active (void);
 
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index 6e4c5c4..5ef5f44 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -81,7 +81,14 @@ static const char maildir_config_comment[] =
     "\n"
     "\tThe \"notmuch new\" command will notice flag changes in filenames\n"
     "\tand update tags, while the \"notmuch tag\" and \"notmuch restore\"\n"
-    "\tcommands will notice tag changes and update flags in filenames\n";
+    "\tcommands will notice tag changes and update flags in filenames\n"
+    "\n"
+    "\tpreserve_new           Valid values are true and false.\n"
+    "\n"
+    "\tThis setting has only effects if synchronize_flags is set to true.\n"
+    "\tIf true, the \"notmuch tag\" and \"notmuch restore\" commands will\n"
+    "\tpreserve the maildir \"new\" status of messages. If false, all updated\n"
+    "\tmessages lose their \"new\" status.\n";
 
 struct _notmuch_config {
     char *filename;
@@ -95,6 +102,7 @@ struct _notmuch_config {
     const char **new_tags;
     size_t new_tags_length;
     notmuch_bool_t maildir_synchronize_flags;
+    notmuch_bool_t maildir_preserve_new;
 };
 
 static int
@@ -251,6 +259,7 @@ notmuch_config_open (void *ctx,
     config->new_tags = NULL;
     config->new_tags_length = 0;
     config->maildir_synchronize_flags = TRUE;
+    config->maildir_preserve_new = FALSE;
 
     if (! g_key_file_load_from_file (config->key_file,
 				     config->filename,
@@ -353,6 +362,15 @@ notmuch_config_open (void *ctx,
 	g_error_free (error);
     }
 
+    error = NULL;
+    config->maildir_preserve_new =
+	g_key_file_get_boolean (config->key_file,
+				"maildir", "preserve_new", &error);
+    if (error) {
+	notmuch_config_set_maildir_preserve_new (config, FALSE);
+	g_error_free (error);
+    }
+
     /* Whenever we know of configuration sections that don't appear in
      * the configuration file, we add some comments to help the user
      * understand what can be done. */
@@ -764,3 +782,18 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 			    "maildir", "synchronize_flags", synchronize_flags);
     config->maildir_synchronize_flags = synchronize_flags;
 }
+
+notmuch_bool_t
+notmuch_config_get_maildir_preserve_new (notmuch_config_t *config)
+{
+    return config->maildir_preserve_new;
+}
+
+void
+notmuch_config_set_maildir_preserve_new (notmuch_config_t *config,
+					 notmuch_bool_t preserve_new)
+{
+    g_key_file_set_boolean (config->key_file,
+			    "maildir", "preserve_new", preserve_new);
+    config->maildir_preserve_new = preserve_new;
+}
diff --git a/notmuch-restore.c b/notmuch-restore.c
index f095f64..620a1f2 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -26,6 +26,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
     notmuch_bool_t synchronize_flags;
+    notmuch_bool_t preserve_new;
     FILE *input;
     char *line = NULL;
     size_t line_size;
@@ -43,6 +44,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	return 1;
 
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
+    preserve_new = notmuch_config_get_maildir_preserve_new (config);
 
     if (argc) {
 	input = fopen (argv[0], "r");
@@ -134,8 +136,12 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
 	notmuch_message_thaw (message);
 
-	if (synchronize_flags)
-	    notmuch_message_tags_to_maildir_flags (message);
+	if (synchronize_flags) {
+	    if (preserve_new)
+		notmuch_message_tags_to_maildir_flags_preserve (message);
+	    else
+		notmuch_message_tags_to_maildir_flags (message);
+	}
 
       NEXT_LINE:
 	if (message)
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 6204ae3..c36c3c9 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -44,6 +44,7 @@ notmuch_tag_command (void *ctx, unused (int argc), unused (char *argv[]))
     notmuch_message_t *message;
     struct sigaction action;
     notmuch_bool_t synchronize_flags;
+    notmuch_bool_t preserve_new;
     int i;
 
     /* Setup our handler for SIGINT */
@@ -101,6 +102,7 @@ notmuch_tag_command (void *ctx, unused (int argc), unused (char *argv[]))
 	return 1;
 
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
+    preserve_new = notmuch_config_get_maildir_preserve_new (config);
 
     query = notmuch_query_create (notmuch, query_string);
     if (query == NULL) {
@@ -128,8 +130,12 @@ notmuch_tag_command (void *ctx, unused (int argc), unused (char *argv[]))
 
 	notmuch_message_thaw (message);
 
-	if (synchronize_flags)
-	    notmuch_message_tags_to_maildir_flags (message);
+	if (synchronize_flags) {
+	    if (preserve_new)
+		notmuch_message_tags_to_maildir_flags_preserve (message);
+	    else
+		notmuch_message_tags_to_maildir_flags (message);
+	}
 
 	notmuch_message_destroy (message);
     }
-- 
1.7.2.5

Re: [RFC][PATCH] tags_to_maildir_flags: Add option to not move messages from "new/" to "cur/"

[Austin Clements]

Welcome to notmuch!

From your description, I assume you're using both notmuch and another
MUA simultaneously.  I'm betting that MUA is mutt (though please
correct me if I'm wrong).

Unfortunately, mutt's interpretation of maildir doesn't agree with the
rest of the world.  I don't know of any other MUA that exposes the
distinction between mail in the "new" directory and mail in the "old"
directory (at least Dovecot, Evolution, Gnus, Kmail, and of course
notmuch don't).  In other MUA's, any mail in the "new" directory is
immediately moved to "cur" and "new" mail is simply anything without
the seen flag.  mutt, on the other hand, only considers mail in the
"new" directory to be "new".

While the maildir specification is a little vague, it strongly implies
two things that mutt's approach violates: mail should never move from
"cur" to "new", and only mail in "cur" can have flags [1].  While it
never states it outright, the philosophy is that "new" is simply a
staging ground for incoming mail, not a user-visible state (and
certainly not user-manipulable).

Because of this, I don't think notmuch is the right place to change
this (certainly not in a way that would also violate the spec).  Could
you elaborate a bit on your workflow?  (In particular, are you using
mutt's distinction between new and old?)  Maybe we can find an
alternate solution.

[1] There's a bug open about the second problem (thanks to ccxCZ for
finding this):
    http://dev.mutt.org/trac/ticket/2476
Of course, fixing that implies addressing the first problem, too.

On Thu, Jun 23, 2011 at 11:36 AM, Louis Rilling <l.rilling@av7.net> wrote:
> notmuch_message_tags_to_maildir_flags() 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.
>
> This patch introduces notmuch_message_tags_to_maildir_flags_preserve(), which
> does the same job as notmuch_message_tags_to_maildir_flags() except moving
> from "maildir "new/" to maildir "cur/". A new option "preserve_new" is
> introduced in "[maildir]" section of .notmuch-config, so that users can
> configure whether commands "notmuch tag" and "notmuch restore" preserve the
> "new" status or not.
>
> Signed-off-by: Louis Rilling <l.rilling@av7.net>
> ---
> Hi,
>
> I'm in the process of using notmuch, but the issue "addressed" by this patch
> would make me change my habits a bit too fast. I use the "new" status for
> quickly checking (often without reading) which emails I just received,
> implementing some kind of context/mood/daytime-dependent quick filtering. I'd
> also like to run a pre-tagging script automatically when synchronizing
> periodically (and automatically too) my mailboxes. But the current behavior of
> "notmuch tag" makes me lose my quick filtering ability.
>
> This patch is mostly written for discussion. It is certainly not polished (API,
> ABI, bindings) and not tested at all. In particular, I know that there are some
> plans to customize flags synchronization, but I don't know how the library API
> could/should be impacted.
>
> Thanks for your comments!
>
> Louis

Re: [RFC][PATCH] tags_to_maildir_flags: Add option to not move messages from "new/" to "cur/"

[Louis Rilling]

On 24/06/11 11:19 -0400, Austin Clements wrote:
> Welcome to notmuch!

Thanks!

> 
> From your description, I assume you're using both notmuch and another
> MUA simultaneously.  I'm betting that MUA is mutt (though please
> correct me if I'm wrong).

Correct :)

> 
> Unfortunately, mutt's interpretation of maildir doesn't agree with the
> rest of the world.  I don't know of any other MUA that exposes the
> distinction between mail in the "new" directory and mail in the "old"
> directory (at least Dovecot, Evolution, Gnus, Kmail, and of course
> notmuch don't).  In other MUA's, any mail in the "new" directory is
> immediately moved to "cur" and "new" mail is simply anything without
> the seen flag.  mutt, on the other hand, only considers mail in the
> "new" directory to be "new".
> 
> While the maildir specification is a little vague, it strongly implies
> two things that mutt's approach violates: mail should never move from
> "cur" to "new", and only mail in "cur" can have flags [1].  While it
> never states it outright, the philosophy is that "new" is simply a
> staging ground for incoming mail, not a user-visible state (and
> certainly not user-manipulable).

Thanks for the detailed explanation.

> 
> Because of this, I don't think notmuch is the right place to change
> this (certainly not in a way that would also violate the spec).  Could
> you elaborate a bit on your workflow?  (In particular, are you using
> mutt's distinction between new and old?)  Maybe we can find an
> alternate solution.

Yes my quick "mood-dependent" filtering just acts on new (as shown by mutt)
mails, while old mails are kept until either 1) they can be read (yes, this
happens!), or 2) they can be archived (because they might be interesting in
some future), or 3) they can be deleted (eg. became too old to be worth
reading them).

However, I don't think that I need the violating behavior of mutt: new mails
are just new, not replied to/read, or anything else. The only rare cases in
which I may set back the new flag are when I mistakenly start reading an email
(wrong keystroke most of the time). Even in that case, I don't care if the mail
becomes old (ie moves to cur/).

Maybe the alternate solution could consist in simply not renaming emails having
no flags to be changed (Currently notmuch_message_tags_to_maildir_flags()
unconditionally moves messages from new/ to cur/). This would even lead to a
shorter patch :) Do you think that this would be acceptable?

Thanks,

Louis

> 
> [1] There's a bug open about the second problem (thanks to ccxCZ for
> finding this):
>     http://dev.mutt.org/trac/ticket/2476
> Of course, fixing that implies addressing the first problem, too.
> 
> On Thu, Jun 23, 2011 at 11:36 AM, Louis Rilling <l.rilling@av7.net> wrote:
> > notmuch_message_tags_to_maildir_flags() 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.
> >
> > This patch introduces notmuch_message_tags_to_maildir_flags_preserve(), which
> > does the same job as notmuch_message_tags_to_maildir_flags() except moving
> > from "maildir "new/" to maildir "cur/". A new option "preserve_new" is
> > introduced in "[maildir]" section of .notmuch-config, so that users can
> > configure whether commands "notmuch tag" and "notmuch restore" preserve the
> > "new" status or not.
> >
> > Signed-off-by: Louis Rilling <l.rilling@av7.net>
> > ---
> > Hi,
> >
> > I'm in the process of using notmuch, but the issue "addressed" by this patch
> > would make me change my habits a bit too fast. I use the "new" status for
> > quickly checking (often without reading) which emails I just received,
> > implementing some kind of context/mood/daytime-dependent quick filtering. I'd
> > also like to run a pre-tagging script automatically when synchronizing
> > periodically (and automatically too) my mailboxes. But the current behavior of
> > "notmuch tag" makes me lose my quick filtering ability.
> >
> > This patch is mostly written for discussion. It is certainly not polished (API,
> > ABI, bindings) and not tested at all. In particular, I know that there are some
> > plans to customize flags synchronization, but I don't know how the library API
> > could/should be impacted.
> >
> > Thanks for your comments!
> >
> > Louis

Re: [RFC][PATCH] tags_to_maildir_flags: Add option to not move messages from "new/" to "cur/"

[Carl Worth]

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

On Fri, 24 Jun 2011 22:34:03 +0200, Louis Rilling <l.rilling@av7.net> wrote:
> Maybe the alternate solution could consist in simply not renaming emails having
> no flags to be changed (Currently notmuch_message_tags_to_maildir_flags()
> unconditionally moves messages from new/ to cur/). This would even lead to a
> shorter patch :) Do you think that this would be acceptable?

That could very well be acceptable.

A user would have to carefully configure notmuch to get this
behavior. Options for that could include the following in
~/.notmuch-config:

	[maildir]
	synchronize_flags=false

Or:

	[new]
	tags=

I think that if either of the above options are set that notmuch won't
need to change the message's filename. And in that case, I would feel
that it wouldn't be entirely unreasonable for notmuch to leave message
in the "new" directory.

This allows the user to effectively setup a mail-store that notmuch
treats as more-or-less "read only" and that seems like a useful thing.

Please experiment with that and feel free to send the patch or patches
that you generate.

Thanks again, and I hope you can find a way to make notmuch work for
you.

-Carl

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

[PATCH 0/2] lib: Don't always move from maildir new/ to maildir cur/

[Louis Rilling]

Hello,

Here is an alternative, as we discussed earlier. This is enough to keep me
using mutt's ability to show the "new" status, and to allow me to test notmuch
with my real emails. Altough my notmuch config uses

	[new]
	tags=new;

	[maildir]
	synchronize_flags=true

messages will be kept in "new/", even with

	[new]
	tags=inbox;unread;
	
	[maildir]
	synchronize_flags=true

as long as no maildir flags have to be changed.

The first patch can be applied independently.

Thanks,

Louis


Louis Rilling (2):
  tags_to_maildir_flags: Cleanup double assignement
  tags_to_maildir_flags: Don't rename if no flags change

 lib/message.cc |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.7.2.5

[PATCH 1/2] 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 d993cde..64b6cf8 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 2/2] 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.

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 64b6cf8..131d99b 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;
+    bool 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 NULL;
+
     filename_new = (char *) talloc_size (ctx,
 					 info - filename +
 					 strlen (":2,") + flags_in_map + 1);
-- 
1.7.2.5

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

[Austin Clements]

I worry that this may compound the confusion caused by mutt's handling
of the new flag, but I suppose people aren't likely to manipulate any
of the other maildir-synchronized flags without also marking the
message as seen.  At any rate, the change is certainly correct
technically.  A few nits below.

Quoth Louis Rilling on Jul 11 at  4:36 pm:
> 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.
> 
> 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 64b6cf8..131d99b 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;
> +    bool flags_changed = false;

The convention in notmuch is to use notmuch_bool_t, TRUE, and FALSE
(though, admittedly, I don't know why; avoiding C99-isms?)

>      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 NULL;
> +

NULL generally indicates an error in notmuch and is currently used
that way in _new_maildir_filename, so even though the caller currently
doesn't really care, I'd lean against overloading it to indicate that
the filename doesn't need to change.  Despite the slight inefficiency,
I would recommend returning talloc_strdup (ctx, filename).

>      filename_new = (char *) talloc_size (ctx,
>  					 info - filename +
>  					 strlen (":2,") + flags_in_map + 1);

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

[Louis Rilling]

On 11/07/11 16:07 -0400, Austin Clements wrote:
> I worry that this may compound the confusion caused by mutt's handling
> of the new flag, but I suppose people aren't likely to manipulate any
> of the other maildir-synchronized flags without also marking the
> message as seen.

Even if they don't mark the message as seen, any flag changed would move the message to cur/. The only buggy behavior would be from mutt, with the bug you mentioned about mutt putting messages with flags back to new/.

> At any rate, the change is certainly correct
> technically.  A few nits below.

They should be addressed by the follow-up patch. Just a comment below.

> 
> Quoth Louis Rilling on Jul 11 at  4:36 pm:
> > 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.
> > 
> > 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 64b6cf8..131d99b 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;
> > +    bool flags_changed = false;
> 
> The convention in notmuch is to use notmuch_bool_t, TRUE, and FALSE
> (though, admittedly, I don't know why; avoiding C99-isms?)

And bool is already used at another place in message.cc:

	struct maildir_flag_tag {
	    char flag;
	    const char *tag;
	    bool inverse;
	};

IIUC it should be changed to notmuch_bool_t too.

> 
> >      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 NULL;
> > +
> 
> NULL generally indicates an error in notmuch and is currently used
> that way in _new_maildir_filename, so even though the caller currently
> doesn't really care, I'd lean against overloading it to indicate that
> the filename doesn't need to change.  Despite the slight inefficiency,
> I would recommend returning talloc_strdup (ctx, filename).

Ok.

Thanks,

Louis

> 
> >      filename_new = (char *) talloc_size (ctx,
> >  					 info - filename +
> >  					 strlen (":2,") + flags_in_map + 1);

[PATCH v2] 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 64b6cf8..3f8c4ba 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

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

[Austin Clements]

Quoth Louis Rilling on Jul 12 at 12:38 am:
> On 11/07/11 16:07 -0400, Austin Clements wrote:
> > I worry that this may compound the confusion caused by mutt's handling
> > of the new flag, but I suppose people aren't likely to manipulate any
> > of the other maildir-synchronized flags without also marking the
> > message as seen.
> 
> Even if they don't mark the message as seen, any flag changed would
> move the message to cur/. The only buggy behavior would be from
> mutt, with the bug you mentioned about mutt putting messages with
> flags back to new/.

Yes.  I was thinking of someone tagging a message as, say, flagged,
while it's still tagged unread.  Then it would change from new to old
in mutt.  OTOH, adding some other non-synchronized tag wouldn't change
it from new to old.  I don't think there is a "correct" solution; your
approach is probably the best compromise.

> > The convention in notmuch is to use notmuch_bool_t, TRUE, and FALSE
> > (though, admittedly, I don't know why; avoiding C99-isms?)
> 
> And bool is already used at another place in message.cc:
> 
> 	struct maildir_flag_tag {
> 	    char flag;
> 	    const char *tag;
> 	    bool inverse;
> 	};
> 
> IIUC it should be changed to notmuch_bool_t too.

Yes, I suppose it should (something slipped by cworth's eagle-eyed
reviews!).  Though that appears to be the sole use of bool in all of
libnotmuch.

Re: [PATCH v2] tags_to_maildir_flags: Don't rename if no flags change

[Austin Clements]

LGTM.

Quoth Louis Rilling on Jul 12 at 12:41 am:
> 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 64b6cf8..3f8c4ba 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);

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

[Louis Rilling]

On 11/07/11 20:03 -0400, Austin Clements wrote:
> > > The convention in notmuch is to use notmuch_bool_t, TRUE, and FALSE
> > > (though, admittedly, I don't know why; avoiding C99-isms?)
> > 
> > And bool is already used at another place in message.cc:
> > 
> > 	struct maildir_flag_tag {
> > 	    char flag;
> > 	    const char *tag;
> > 	    bool inverse;
> > 	};
> > 
> > IIUC it should be changed to notmuch_bool_t too.
> 
> Yes, I suppose it should (something slipped by cworth's eagle-eyed
> reviews!).  Though that appears to be the sole use of bool in all of
> libnotmuch.

I wonder if this is due to incompatible definitions of type bool in C99 and
C++. In that case this is probably harmless since struct maildir_flag_tag is
only visible from message.cc. Anyway, I'm sending a conversion patch together
with the updated series to make it clearer for Carl.

Thanks,

Louis