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

2011-07-12 Thread 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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


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

2011-07-11 Thread 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.rill...@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);
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


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

2011-07-11 Thread 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.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch