e0/2f552880da799e2c73a8c2085616b7ae04df68

   1 Return-Path: <cworth@cworth.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 86D98431FC0;\r
   6         Wed, 25 Nov 2009 09:52:15 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 Received: from olra.theworths.org ([127.0.0.1])\r
   9         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  10         with ESMTP id qIbLhilVKbpd; Wed, 25 Nov 2009 09:52:14 -0800 (PST)\r
  11 Received: from cworth.org (localhost [127.0.0.1])\r
  12         by olra.theworths.org (Postfix) with ESMTP id 4A7CC431FAE;\r
  13         Wed, 25 Nov 2009 09:52:14 -0800 (PST)\r
  14 From: Carl Worth <cworth@cworth.org>\r
  15 To: Michiel Buddingh' <michiel@michielbuddingh.net>, notmuch@notmuchmail.org\r
  16 In-Reply-To: <9bfdedddeab9c58cd45d8d448323d0fc@localhost>\r
  17 References: <87fx8bygi7.fsf@linux.vnet.ibm.com>\r
  18         <87bpiv4t9h.fsf@yoom.home.cworth.org>\r
  19         <87y6lz39nd.fsf@yoom.home.cworth.org>\r
  20         <20091121221207.GB17268@jukie.net>\r
  21         <9cce5525b093b87fe74d427954ffad89@localhost>\r
  22         <87d43b2oif.fsf@yoom.home.cworth.org>\r
  23         <9bfdedddeab9c58cd45d8d448323d0fc@localhost>\r
  24 Date: Wed, 25 Nov 2009 09:52:00 -0800\r
  25 Message-ID: <87skc23327.fsf@yoom.home.cworth.org>\r
  26 MIME-Version: 1.0\r
  27 Content-Type: text/plain; charset=us-ascii\r
  28 Subject: Re: [notmuch] [PATCH] notmuch: Add Maildir directory name as tag\r
  29  name for messages\r
  30 X-BeenThere: notmuch@notmuchmail.org\r
  31 X-Mailman-Version: 2.1.12\r
  32 Precedence: list\r
  33 List-Id: "Use and development of the notmuch mail system."\r
  34         <notmuch.notmuchmail.org>\r
  35 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  36         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  37 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  38 List-Post: <mailto:notmuch@notmuchmail.org>\r
  39 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  40 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  41         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  42 X-List-Received-Date: Wed, 25 Nov 2009 17:52:15 -0000\r
  43 \r
  44 On Sun, 22 Nov 2009 10:33:53 +0100, Michiel Buddingh' <michiel@michielbuddingh.net> wrote:\r
  45 > On the positive side, this allows us to map these flags onto tags,\r
  46 > at least for indexing (the patch at the bottom implements this), and,\r
  47 > if I can pry you away from your principles, later for modification\r
  48 > as well.\r
  49 \r
  50 Hi Michiel,\r
  51 \r
  52 I'm finally getting around to reviewing this patch. I think the\r
  53 functionality of respecting these maildir flags for initial import is\r
  54 going to be really important. So thanks for looking at this!\r
  55 \r
  56 Some comments on the patches below.\r
  57 \r
  58 > +enum storage_type\r
  59 > +notmuch_config_get_storage_type (notmuch_config_t *config);\r
  60 > +\r
  61 >  notmuch_bool_t\r
  62 >  debugger_is_active (void);\r
  63 >  \r
  64 > +\r
  65 > +\r
  66 > +\r
  67 >  #endif\r
  68 \r
  69 [nit] Try to look out for introducing excess whitespace like that.\r
  70 \r
  71 > +    " The other value is 'storage_type', which can currently be set to\n"\r
  72 > +    " 'maildir' or 'none'.\n";\r
  73 \r
  74 This part of the patch I don't like. I've got a mail collection spanning\r
  75 over a decade, and it's seen a lot of strange things. Most of my mail is\r
  76 in maildir format, but not quite all of it. And I actually like the\r
  77 ability to just shove random new messages into the mail store manually\r
  78 without having to create a maildir name for it.\r
  79 \r
  80 So I don't think a global configuration makes sense here. Meanwhile,\r
  81 it's really easy to detect the presence of a maildir. Whenever we see\r
  82 child directories of "cur", "new", and "tmp" then we should turn on the\r
  83 processing of maildir flags for when processing mail in "cur" and "new".\r
  84 \r
  85 > @@ -257,7 +262,7 @@ notmuch_config_open (void *ctx,\r
  86 >           talloc_free (email);\r
  87 >       }\r
  88 >      }\r
  89 > -\r
  90 > +    \r
  91 >      /* When we create a new configuration file here, we  add some\r
  92 >       * comments to help the user understand what can be done. */\r
  93 >      if (is_new) {\r
  94 \r
  95 [nit] Trailing whitespace inserted there as well.\r
  96 \r
  97 Hmm... I was going to say that git ships with a pre-commit hook you can\r
  98 turn on that checks for trailing whitespace and aborts the commit if\r
  99 it's present. But it looks like the currently shipping pre-commit.sample\r
 100 hook doesn't do this anymore. So I'll have to find out what the current\r
 101 recommended way is to get this behavior. Does anybody here know?\r
 102 \r
 103 > +     i += 3;\r
 104 > +     for (; i < (path + l) && !end_of_flags; i++) {\r
 105 > +         switch (*i) {\r
 106 > +         case 'F': /* flagged */\r
 107 > +             notmuch_message_add_tag (message, "flagged");\r
 108 > +             break;\r
 109 > +         case 'R': /* the message has been replied to */\r
 110 > +             notmuch_message_add_tag (message, "replied");\r
 111 > +             break;\r
 112 > +         case 'D': \r
 113 > +             notmuch_message_add_tag (message, "draft");\r
 114 > +             break;\r
 115 > +         case 'S': /* Indicate a message has been read */\r
 116 > +             notmuch_message_add_tag (message, "seen");\r
 117 > +             seen = TRUE;\r
 118 > +             break;\r
 119 > +         case 'T': /* Indicates a message has been marked as trash */\r
 120 > +             notmuch_message_add_tag (message, "trashed");\r
 121 > +             break;\r
 122 > +         case 'P': /* indicates a message has been forwarded */\r
 123 > +             notmuch_message_add_tag (message, "passed");\r
 124 > +             break;\r
 125 > +         default:\r
 126 > +             end_of_flags = TRUE;\r
 127 > +             break;\r
 128 > +         }\r
 129 > +     }\r
 130 > +    }\r
 131 > +    \r
 132 > +    if (i == NULL || !seen) { \r
 133 > +     /* mark messages without any flags, or the seen flag as 'unseen' */\r
 134 > +     notmuch_message_add_tag (message, "unseen");\r
 135 > +    }\r
 136 \r
 137 OK, now we're into the meat of things. Clearly, you're directly\r
 138 supporting the documented flags of maildir. But we need to do a few\r
 139 things differently here. Most importantly, notmuch is already using an\r
 140 "unread" tag, so maildir's S flag should map that *that* rather than\r
 141 adding new "unseen" and "seen" flags. So messages with the S flag would\r
 142 not get the "unread" tag and messages without S would get the "unread"\r
 143 tag.\r
 144 \r
 145 The "flagged" and "replied" tags seem reasonable enough. But for\r
 146 "trashed" and "passed" I think I'd rather see the tag names as "deleted"\r
 147 and "forwarded". (Since I can imagine adding commands to notmuch for\r
 148 "delete" and "forward" but not for "trash" nor "pass").\r
 149 \r
 150 Finally, all of the new behavior here needs to be documented in both\r
 151 notmuch.1 and notmuch.c where the current behavior with respect to\r
 152 "inbox" and "unread" is already documented.\r
 153 \r
 154 Oh, and setting the "inbox" tag correctly here based on the maildir tags\r
 155 is the final and most important thing. It looks like that's missing from\r
 156 the above. So, a missing "S" flag should map to adding both the "inbox"\r
 157 and "unread" tags.\r
 158 \r
 159 > (state->ignore_read_only_directories)) {\r
 160 >       state->saw_read_only_directory = TRUE;\r
 161 > -     goto DONE;\r
 162 > +     goto DONE;          \r
 163 >      }\r
 164 \r
 165 Trailing whitespace strikes again! :-)\r
 166 \r
 167 \r
 168 > +         if (state->storage_type == MAILDIR) {\r
 169 > +             char * leaf = basename(next);\r
 170 \r
 171 You could save the basename call by examining the leaf name when it is\r
 172 available as a standalone string up in the caller.\r
 173 \r
 174 So this patch is close, but needs a few fixes.\r
 175 \r
 176 Thanks again,\r
 177 \r
 178 -Carl\r