From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 86D98431FC0; Wed, 25 Nov 2009 09:52:15 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qIbLhilVKbpd; Wed, 25 Nov 2009 09:52:14 -0800 (PST) Received: from cworth.org (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 4A7CC431FAE; Wed, 25 Nov 2009 09:52:14 -0800 (PST) From: Carl Worth To: Michiel Buddingh' , notmuch@notmuchmail.org In-Reply-To: <9bfdedddeab9c58cd45d8d448323d0fc@localhost> References: <87fx8bygi7.fsf@linux.vnet.ibm.com> <87bpiv4t9h.fsf@yoom.home.cworth.org> <87y6lz39nd.fsf@yoom.home.cworth.org> <20091121221207.GB17268@jukie.net> <9cce5525b093b87fe74d427954ffad89@localhost> <87d43b2oif.fsf@yoom.home.cworth.org> <9bfdedddeab9c58cd45d8d448323d0fc@localhost> Date: Wed, 25 Nov 2009 09:52:00 -0800 Message-ID: <87skc23327.fsf@yoom.home.cworth.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [PATCH] notmuch: Add Maildir directory name as tag name for messages X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Nov 2009 17:52:15 -0000 On Sun, 22 Nov 2009 10:33:53 +0100, Michiel Buddingh' wrote: > On the positive side, this allows us to map these flags onto tags, > at least for indexing (the patch at the bottom implements this), and, > if I can pry you away from your principles, later for modification > as well. Hi Michiel, I'm finally getting around to reviewing this patch. I think the functionality of respecting these maildir flags for initial import is going to be really important. So thanks for looking at this! Some comments on the patches below. > +enum storage_type > +notmuch_config_get_storage_type (notmuch_config_t *config); > + > notmuch_bool_t > debugger_is_active (void); > > + > + > + > #endif [nit] Try to look out for introducing excess whitespace like that. > + " The other value is 'storage_type', which can currently be set to\n" > + " 'maildir' or 'none'.\n"; This part of the patch I don't like. I've got a mail collection spanning over a decade, and it's seen a lot of strange things. Most of my mail is in maildir format, but not quite all of it. And I actually like the ability to just shove random new messages into the mail store manually without having to create a maildir name for it. So I don't think a global configuration makes sense here. Meanwhile, it's really easy to detect the presence of a maildir. Whenever we see child directories of "cur", "new", and "tmp" then we should turn on the processing of maildir flags for when processing mail in "cur" and "new". > @@ -257,7 +262,7 @@ notmuch_config_open (void *ctx, > talloc_free (email); > } > } > - > + > /* When we create a new configuration file here, we add some > * comments to help the user understand what can be done. */ > if (is_new) { [nit] Trailing whitespace inserted there as well. Hmm... I was going to say that git ships with a pre-commit hook you can turn on that checks for trailing whitespace and aborts the commit if it's present. But it looks like the currently shipping pre-commit.sample hook doesn't do this anymore. So I'll have to find out what the current recommended way is to get this behavior. Does anybody here know? > + i += 3; > + for (; i < (path + l) && !end_of_flags; i++) { > + switch (*i) { > + case 'F': /* flagged */ > + notmuch_message_add_tag (message, "flagged"); > + break; > + case 'R': /* the message has been replied to */ > + notmuch_message_add_tag (message, "replied"); > + break; > + case 'D': > + notmuch_message_add_tag (message, "draft"); > + break; > + case 'S': /* Indicate a message has been read */ > + notmuch_message_add_tag (message, "seen"); > + seen = TRUE; > + break; > + case 'T': /* Indicates a message has been marked as trash */ > + notmuch_message_add_tag (message, "trashed"); > + break; > + case 'P': /* indicates a message has been forwarded */ > + notmuch_message_add_tag (message, "passed"); > + break; > + default: > + end_of_flags = TRUE; > + break; > + } > + } > + } > + > + if (i == NULL || !seen) { > + /* mark messages without any flags, or the seen flag as 'unseen' */ > + notmuch_message_add_tag (message, "unseen"); > + } OK, now we're into the meat of things. Clearly, you're directly supporting the documented flags of maildir. But we need to do a few things differently here. Most importantly, notmuch is already using an "unread" tag, so maildir's S flag should map that *that* rather than adding new "unseen" and "seen" flags. So messages with the S flag would not get the "unread" tag and messages without S would get the "unread" tag. The "flagged" and "replied" tags seem reasonable enough. But for "trashed" and "passed" I think I'd rather see the tag names as "deleted" and "forwarded". (Since I can imagine adding commands to notmuch for "delete" and "forward" but not for "trash" nor "pass"). Finally, all of the new behavior here needs to be documented in both notmuch.1 and notmuch.c where the current behavior with respect to "inbox" and "unread" is already documented. Oh, and setting the "inbox" tag correctly here based on the maildir tags is the final and most important thing. It looks like that's missing from the above. So, a missing "S" flag should map to adding both the "inbox" and "unread" tags. > (state->ignore_read_only_directories)) { > state->saw_read_only_directory = TRUE; > - goto DONE; > + goto DONE; > } Trailing whitespace strikes again! :-) > + if (state->storage_type == MAILDIR) { > + char * leaf = basename(next); You could save the basename call by examining the leaf name when it is available as a standalone string up in the caller. So this patch is close, but needs a few fixes. Thanks again, -Carl