Sender: Louis Rilling <lrilling@gmail.com>
Date: Tue, 12 Jul 2011 00:38:05 +0200
From: Louis Rilling <l.rilling@av7.net>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 2/2] tags_to_maildir_flags: Don't rename if no flags
 change
Message-ID: <20110711223805.GC3583@localdomain>
References: <87d3hzgemi.fsf@yoom.home.cworth.org>
	<1310394978-11883-3-git-send-email-l.rilling@av7.net>
	<20110711200712.GB25558@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20110711200712.GB25558@mit.edu>
User-Agent: Mutt/1.5.20 (2009-06-14)
Cc: notmuch@notmuchmail.org
Precedence: list

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);