50/dcd63ad0074904bcf00ba8bc98822b78299276

   1 Return-Path: <peterz@infradead.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 3467A431FDA\r
   6         for <notmuch@notmuchmail.org>; Tue, 12 Nov 2013 11:47:39 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -2.3\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-2.3 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id zK3Owp4lsx65 for <notmuch@notmuchmail.org>;\r
  16         Tue, 12 Nov 2013 11:47:31 -0800 (PST)\r
  17 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134])\r
  18         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 2E633431FD2\r
  21         for <notmuch@notmuchmail.org>; Tue, 12 Nov 2013 11:47:31 -0800 (PST)\r
  22 Received: from dhcp-077-248-225-117.chello.nl ([77.248.225.117] helo=laptop)\r
  23         by merlin.infradead.org with esmtpsa (Exim 4.80.1 #2 (Red Hat Linux))\r
  24         id 1VgJvt-0000kX-RB; Tue, 12 Nov 2013 19:47:30 +0000\r
  25 Received: by laptop (Postfix, from userid 1000)\r
  26         id 96DD8103BA2B9; Tue, 12 Nov 2013 20:47:27 +0100 (CET)\r
  27 Date: Tue, 12 Nov 2013 20:47:27 +0100\r
  28 From: Peter Zijlstra <peterz@infradead.org>\r
  29 To: Austin Clements <aclements@csail.mit.edu>\r
  30 Subject: Re: [PATCH] notmuch: Add "maildir:" search option\r
  31 Message-ID: <20131112194727.GE16796@laptop.programming.kicks-ass.net>\r
  32 References: <20131112155637.GA16796@laptop.programming.kicks-ass.net>\r
  33         <87mwl94dte.fsf@awakening.csail.mit.edu>\r
  34 MIME-Version: 1.0\r
  35 Content-Type: text/plain; charset=us-ascii\r
  36 Content-Disposition: inline\r
  37 In-Reply-To: <87mwl94dte.fsf@awakening.csail.mit.edu>\r
  38 User-Agent: Mutt/1.5.21 (2012-12-30)\r
  39 X-Mailman-Approved-At: Tue, 12 Nov 2013 12:40:21 -0800\r
  40 Cc: notmuch@notmuchmail.org\r
  41 X-BeenThere: notmuch@notmuchmail.org\r
  42 X-Mailman-Version: 2.1.13\r
  43 Precedence: list\r
  44 List-Id: "Use and development of the notmuch mail system."\r
  45         <notmuch.notmuchmail.org>\r
  46 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  47         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  48 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  49 List-Post: <mailto:notmuch@notmuchmail.org>\r
  50 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  51 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  52         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  53 X-List-Received-Date: Tue, 12 Nov 2013 19:47:39 -0000\r
  54 \r
  55 On Tue, Nov 12, 2013 at 02:31:25PM -0500, Austin Clements wrote:\r
  56 > > XXX: now I need to go figure out how to do searches like:\r
  57 > >\r
  58 > >   subject:PATCH/0\r
  59 > >\r
  60 > > which would mandate that PATCH is the first word occurring in the\r
  61 > > subject. I think the position index holds enough information but I\r
  62 > > need to look into that and obviously the query parser needs work for\r
  63 > > this.\r
  64 > \r
  65 > This is a frequently requested feature.  Unfortunately, there are two\r
  66 > technical hurdles.  One is that the position information actually\r
  67 > *isn't* enough as it is both because the subject will start at some\r
  68 > arbitrary term offset that depends on the from and to (and maybe other\r
  69 > things) and because the Xapian Query structure doesn't expose a way to\r
  70 > search for a specific term offset (only relative offsets).  The other is\r
  71 > that we currently use Xapian's query parser, which isn't extensible,\r
  72 > though I took a stab at a custom query parser long ago and swear that\r
  73 > one of these days I'll return to it.\r
  74 \r
  75 Bah, I knew that would end up being more complex :-)\r
  76 \r
  77 > >  lib/database.cc |  7 ++++---\r
  78 > >  lib/message.cc  | 40 ++++++++++++++++++++++++++++++++++++++++\r
  79 > >  2 files changed, 44 insertions(+), 3 deletions(-)\r
  80 > >\r
  81 > > diff --git a/lib/database.cc b/lib/database.cc\r
  82 > > index a021bf17253c..53aeaa68954d 100644\r
  83 > > --- a/lib/database.cc\r
  84 > > +++ b/lib/database.cc\r
  85 > > @@ -208,15 +208,16 @@ static prefix_t BOOLEAN_PREFIX_EXTERNAL[] = {\r
  86 > >      { "thread",                    "G" },\r
  87 > >      { "tag",                       "K" },\r
  88 > >      { "is",                        "K" },\r
  89 > > -    { "id",                        "Q" }\r
  90 > > +    { "id",                        "Q" },\r
  91 > > +    { "maildir",           "XMAILDIR:" },\r
  92 > \r
  93 > No colon.  That is, the term prefix should just be "XMAILDIR".\r
  94 \r
  95 No, that colon is crucial; see http://xapian.org/docs/omega/termprefixes.html\r
  96 \r
  97 "X starts a multi-capital letter user-defined prefix. If you want a\r
  98 prefix for something without a standard prefix, you create your own\r
  99 starting with an X (e.g. XSHOESIZE). The prefix ends with the first\r
 100 non-capital. If the term you're prefixing starts with a capital, add a\r
 101 ":" between prefix and term to resolve ambiguity about where the prefix\r
 102 ends and the term begins."\r
 103 \r
 104 Since maildir folder names typically start with a capital we need that\r
 105 ':' in between the prefix and value. I tried, my initial versions didn't\r
 106 have the ':' and reliably didn't work.\r
 107 \r
 108 > >  };\r
 109 > >  \r
 110 > >  static prefix_t PROBABILISTIC_PREFIX[]= {\r
 111 > >      { "from",                      "XFROM" },\r
 112 > >      { "to",                        "XTO" },\r
 113 > >      { "attachment",                "XATTACHMENT" },\r
 114 > > -    { "subject",           "XSUBJECT"},\r
 115 > > -    { "folder",                    "XFOLDER"}\r
 116 > > +    { "subject",           "XSUBJECT" },\r
 117 > > +    { "folder",                    "XFOLDER" },\r
 118 > \r
 119 > Unintentional whitespace change?\r
 120 \r
 121 Oops yes.\r
 122 \r
 123 > > +    /* Convert the directory into a maildir path */\r
 124 > > +    maildir = talloc_strdup(local, directory);\r
 125 > \r
 126 > Add a space before the "(".  (Same thing throughout the rest of the\r
 127 > patch.)\r
 128 \r
 129 Urgh, weird coding style but yes you're right, I should've kept it\r
 130 consistent with the rest of the file. Will fix.\r
 131 \r
 132 > > +\r
 133 > > +    /* Strip the maildir "cur", "new" directory entries. */\r
 134 > > +    i = strlen(maildir);\r
 135 > > +    if (strncmp(maildir + i - 3, "cur", 3) == 0 ||\r
 136 > > +   strncmp(maildir + i - 3, "new", 3) == 0) {\r
 137 > \r
 138 > This is unsafe if directory is less than three characters, which I\r
 139 > believe could happen if the message is in the root mail directory (which\r
 140 > shouldn't happen with a well-formed maildir, but notmuch doesn't require\r
 141 > maildir, and, regardless, we should be defensive).\r
 142 > \r
 143 > Also, we have a STRNCMP_LITERAL macro that we often use for comparisons\r
 144 > with string literals, but I'm good with this, too.\r
 145 \r
 146 Quite so, I haven't actually seen that, but you're quite right.\r
 147 \r
 148 > > +       maildir[i - 3] = '\0';\r
 149 > > +       i -= 3;\r
 150 > > +    }\r
 151 > > +\r
 152 > > +    /* Strip trailing '/' */\r
 153 > > +    while (maildir[i-1] == '/') {\r
 154 > \r
 155 > This is also unsafe if maildir is the empty string.\r
 156 > \r
 157 > Also, add spaces around the "-" (likewise on the next line).\r
 158 \r
 159 Will fix.\r
 160 \r
 161 > > +       maildir[i-1] = '\0';\r
 162 > > +       i--;\r
 163 > > +    }\r
 164 > > +\r
 165 > > +    /* Strip leading '/' */\r
 166 > > +    while (maildir[0] == '/')\r
 167 > > +       maildir++;\r
 168 > > +\r
 169 > > +    /* Strip leading '.' */\r
 170 > > +    while (maildir[0] == '.')\r
 171 > \r
 172 > Why strip multiple dots?  (I'm not saying you shouldn't, I'm just\r
 173 > curious; and it may be worth explaining in the comment.)\r
 174 \r
 175 No reason, copy paste damage from above mostly.\r
 176 \r
 177 > > +       maildir++;\r
 178 > > +\r
 179 > > +    /* Replace all remaining '.' with '/' */\r
 180 > \r
 181 > I think this should only happen if there was a leading '.', indicating a\r
 182 > Maildir++ folder hierarchy.  A lot of people use the "file system"\r
 183 > Maildir layout, which just consists of nested directories where the\r
 184 > leaves are Maildirs (e.g., Dovecot's LAYOUT=fs,\r
 185 > http://wiki2.dovecot.org/MailboxFormat/Maildir#Directory_Structure).  In\r
 186 > this case, it's perfectly legitimate to have '.'s in folder names and it\r
 187 > would be confusing and unexpected to translate them to '/'s.  This would\r
 188 > also make this compatible with MH folders (which notmuch supports,\r
 189 > though admittedly it's unclear if anyone actually uses them).\r
 190 \r
 191 Ah, I wasn't aware Dovecot actually supported this Maildir variant --\r
 192 although I've recently come across this variant someplace and thought it\r
 193 was odd.\r
 194 \r
 195 OK, I can make it conditional on the initial leading dot.\r
 196 \r
 197 I'll respin the patch and try to come up with manpage and test additions\r
 198 to cover this new functionality.\r