a2/d9ef3a56fa1920c9ffa796906b7894a29e619f

   1 Return-Path: <amdragon@mit.edu>\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 780CA429E21\r
   6         for <notmuch@notmuchmail.org>; Wed, 11 Jan 2012 10:48:35 -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: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 NvGxMEIV7LhK for <notmuch@notmuchmail.org>;\r
  16         Wed, 11 Jan 2012 10:48:34 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-6.mit.edu (DMZ-MAILSEC-SCANNER-6.MIT.EDU\r
  18         [18.7.68.35])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 73D8D431FB6\r
  20         for <notmuch@notmuchmail.org>; Wed, 11 Jan 2012 10:48:34 -0800 (PST)\r
  21 X-AuditID: 12074423-b7f9c6d0000008c3-a7-4f0dd9824e6c\r
  22 Received: from mailhub-auth-4.mit.edu ( [18.7.62.39])\r
  23         by dmz-mailsec-scanner-6.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 42.EC.02243.289DD0F4; Wed, 11 Jan 2012 13:48:34 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id q0BImXt0013787; \r
  27         Wed, 11 Jan 2012 13:48:34 -0500\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q0BImWho011482\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Wed, 11 Jan 2012 13:48:33 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1Rl3Dy-0004ET-Gz; Wed, 11 Jan 2012 13:48:38 -0500\r
  37 Date: Wed, 11 Jan 2012 13:48:38 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jani Nikula <jani@nikula.org>\r
  40 Subject: Re: [PATCH 2/3] lib: Add support for automatically excluding tags\r
  41         from queries\r
  42 Message-ID: <20120111184838.GT20796@mit.edu>\r
  43 References: <20120109043101.GH20796@mit.edu>\r
  44         <1326258173-21163-1-git-send-email-amdragon@mit.edu>\r
  45         <1326258173-21163-3-git-send-email-amdragon@mit.edu>\r
  46         <87ty42v99m.fsf@nikula.org>\r
  47 MIME-Version: 1.0\r
  48 Content-Type: text/plain; charset=us-ascii\r
  49 Content-Disposition: inline\r
  50 In-Reply-To: <87ty42v99m.fsf@nikula.org>\r
  51 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  52 X-Brightmail-Tracker:\r
  53  H4sIAAAAAAAAA+NgFmphleLIzCtJLcpLzFFi42IRYrdT1226yetv8G0Fu0XTdGeL6zdnMjsw\r
  54         edy6/5rd49mqW8wBTFFcNimpOZllqUX6dglcGTsWnGcqWGBYcfVdJ1MD41m1LkZODgkBE4n1\r
  55         7/pYIGwxiQv31rN1MXJxCAnsY5RY19vICOFsYJR49vUlVOYkk8SRd49ZIJwljBJzXj0D62cR\r
  56         UJXYPekuG4jNJqAhsW3/ckYQW0RAUWLzyf1gNrOAtMS3381MILawQKTEh01/2UFsXgEdidub\r
  57         dkGt28EoceXKVWaIhKDEyZlPWCCatSRu/HsJ1MwBNmj5Pw6QMCfQrr2TF4LNERVQkZhychvb\r
  58         BEahWUi6ZyHpnoXQvYCReRWjbEpulW5uYmZOcWqybnFyYl5eapGumV5uZoleakrpJkZwYLso\r
  59         72D8c1DpEKMAB6MSD++Ovbz+QqyJZcWVuYcYJTmYlER5V90ACvEl5adUZiQWZ8QXleakFh9i\r
  60         lOBgVhLhdaoByvGmJFZWpRblw6SkOViUxHk1tN75CQmkJ5akZqemFqQWwWRlODiUJHg3ggwV\r
  61         LEpNT61Iy8wpQUgzcXCCDOcBGr4cpIa3uCAxtzgzHSJ/ilFRSpx3EkhCACSRUZoH1wtLPK8Y\r
  62         xYFeEebNB6niASYtuO5XQIOZgAZvWccDMrgkESEl1cAo8e7tdu6TH2te1ITEXm/eF/uAKyQ3\r
  63         gOPh1puFVy/n3NyzJnnZ0fcf6wt0zW5GrP7SqdrT4L//+f/glCXPJCOC3i8yfJC/96VjY2JK\r
  64         xt+sOWd2njt7RU5Rg/PGEg1GtZtxMStidoQt8bb9UVFb0bDz2vfuR+vWOrPILWzw12r9wrdk\r
  65         gVbM64W/lFiKMxINtZiLihMBpsQZ9BcDAAA=\r
  66 Cc: notmuch@notmuchmail.org\r
  67 X-BeenThere: notmuch@notmuchmail.org\r
  68 X-Mailman-Version: 2.1.13\r
  69 Precedence: list\r
  70 List-Id: "Use and development of the notmuch mail system."\r
  71         <notmuch.notmuchmail.org>\r
  72 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  74 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  75 List-Post: <mailto:notmuch@notmuchmail.org>\r
  76 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  77 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  78         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  79 X-List-Received-Date: Wed, 11 Jan 2012 18:48:35 -0000\r
  80 \r
  81 Quoth Jani Nikula on Jan 11 at 10:11 am:\r
  82 > On Wed, 11 Jan 2012 00:02:52 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  83 > > This is useful for tags like "deleted" and "spam" that people\r
  84 > > generally want to exclude from query results.  These exclusions will\r
  85 > > be overridden if a tag is explicitly mentioned in a query.\r
  86 > > ---\r
  87 > >  lib/notmuch.h |    6 ++++++\r
  88 > >  lib/query.cc  |   33 +++++++++++++++++++++++++++++++++\r
  89 > >  2 files changed, 39 insertions(+), 0 deletions(-)\r
  90 > > \r
  91 > > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
  92 > > index 9f23a10..0a3ae2b 100644\r
  93 > > --- a/lib/notmuch.h\r
  94 > > +++ b/lib/notmuch.h\r
  95 > > @@ -457,6 +457,12 @@ notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort);\r
  96 > >  notmuch_sort_t\r
  97 > >  notmuch_query_get_sort (notmuch_query_t *query);\r
  98 > >  \r
  99 > > +/* Add a tag that will be excluded by default from the query results.\r
 100 > > + * This exclusion will be overridden if this tag appears explicitly in\r
 101 > > + * the query. */\r
 102 > > +void\r
 103 > > +notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag);\r
 104 > > +\r
 105 > >  /* Execute a query for threads, returning a notmuch_threads_t object\r
 106 > >   * which can be used to iterate over the results. The returned threads\r
 107 > >   * object is owned by the query and as such, will only be valid until\r
 108 > > diff --git a/lib/query.cc b/lib/query.cc\r
 109 > > index b6c0f12..716db1c 100644\r
 110 > > --- a/lib/query.cc\r
 111 > > +++ b/lib/query.cc\r
 112 > > @@ -27,6 +27,7 @@ struct _notmuch_query {\r
 113 > >      notmuch_database_t *notmuch;\r
 114 > >      const char *query_string;\r
 115 > >      notmuch_sort_t sort;\r
 116 > > +    notmuch_string_list_t *exclude_terms;\r
 117 > >  };\r
 118 > >  \r
 119 > >  typedef struct _notmuch_mset_messages {\r
 120 > > @@ -76,6 +77,8 @@ notmuch_query_create (notmuch_database_t *notmuch,\r
 121 > >  \r
 122 > >      query->sort = NOTMUCH_SORT_NEWEST_FIRST;\r
 123 > >  \r
 124 > > +    query->exclude_terms = _notmuch_string_list_create (query);\r
 125 > > +\r
 126 > >      return query;\r
 127 > >  }\r
 128 > >  \r
 129 > > @@ -97,6 +100,13 @@ notmuch_query_get_sort (notmuch_query_t *query)\r
 130 > >      return query->sort;\r
 131 > >  }\r
 132 > >  \r
 133 > > +void\r
 134 > > +notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag)\r
 135 > > +{\r
 136 > > +    char *term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);\r
 137 > > +    _notmuch_string_list_append (query->exclude_terms, term);\r
 138 > > +}\r
 139 > > +\r
 140 > >  /* We end up having to call the destructors explicitly because we had\r
 141 > >   * to use "placement new" in order to initialize C++ objects within a\r
 142 > >   * block that we allocated with talloc. So C++ is making talloc\r
 143 > > @@ -112,6 +122,25 @@ _notmuch_messages_destructor (notmuch_mset_messages_t *messages)\r
 144 > >      return 0;\r
 145 > >  }\r
 146 > >  \r
 147 > \r
 148 > I'd like to have a comment here, or inline in the code, explaining the\r
 149 > following function a little bit.\r
 150 \r
 151 Done.\r
 152 \r
 153 > > +static Xapian::Query\r
 154 > > +_notmuch_exclude_tags (notmuch_query_t *query, Xapian::Query xquery)\r
 155 > > +{\r
 156 > > +    Xapian::TermIterator end = xquery.get_terms_end ();\r
 157 > > +\r
 158 > > +    for (notmuch_string_node_t *term = query->exclude_terms->head; term;\r
 159 > > +    term = term->next) {\r
 160 > > +   Xapian::TermIterator it = xquery.get_terms_begin ();\r
 161 > > +   for (; it != end; it++) {\r
 162 > > +       if (*it == term->string)\r
 163 > \r
 164 > [This is a double reminder to me why I'm not that enthusiastic about\r
 165 > operator overloading in C++.]\r
 166 \r
 167 Actually, that's a good point.  I had originally done this to prevent\r
 168 std::string from performing any internal allocation or copying (which\r
 169 calling c_str could do), but since it took me some digging to figure\r
 170 out if I had *actually* avoided this (turns out there's an overloaded\r
 171 string==char*, so I had), I've switched to using string.compare, which\r
 172 I think makes the behavior and performance more obvious.\r
 173 \r
 174 > > +           break;\r
 175 > > +   }\r
 176 > > +   if (it == end)\r
 177 > > +       xquery = Xapian::Query (Xapian::Query::OP_AND_NOT,\r
 178 > > +                               xquery, Xapian::Query (term->string));\r
 179 > \r
 180 > I briefly dug into Xapian documentation and source code, and became none\r
 181 > the wiser whether this copies the queries passed to it or not, i.e. does\r
 182 > this leak memory or not. I just presume you know what you're doing. ;)\r
 183 \r
 184 Since my code isn't doing any memory allocation, it can't leak memory.\r
 185 It could do a lot of copying, but it turns out that isn't a problem\r
 186 either because Xapian objects are internally reference-counted\r
 187 handles.  So, in other words, if you write the obvious thing, it will\r
 188 do the right thing, which is totally contrary to what C++ has\r
 189 conditioned us to expect.  ]:--8)\r
 190 \r
 191 > I think the function fails if someone is stupid enough to exclude the\r
 192 > same tag twice. I'm not sure if you should care. If you think so, you\r
 193 > could just check double add in notmuch_query_add_tag_exclude().\r
 194 \r
 195 It handles this correctly for two reasons.  Suppose tag x is excluded\r
 196 twice.  At worst, if tag:x doesn't appear in the query, the returned\r
 197 query will get two AND NOT tag:x's, but that doesn't affect the\r
 198 results.  It turns out it doesn't even get doubled up for a slightly\r
 199 subtle reason: when the loop reaches the second x in the exclude list,\r
 200 it will iterate over the new query, which already has the AND NOT\r
 201 tag:x in it, so it will see the tag:x as if it were in the original\r
 202 query and not further modify the query.\r
 203 \r
 204 However, this did point out a bug.  I was using the end iterator from\r
 205 the original query even when I started iterating over the modified\r
 206 query.\r
 207 \r
 208 I'll send out an updated series after someone looks at the third\r
 209 patch.\r
 210 \r
 211 > Otherwise, looks good.\r
 212 > \r
 213 > > +    }\r
 214 > > +    return xquery;\r
 215 > > +}\r
 216 > > +\r
 217 > >  notmuch_messages_t *\r
 218 > >  notmuch_query_search_messages (notmuch_query_t *query)\r
 219 > >  {\r
 220 > > @@ -157,6 +186,8 @@ notmuch_query_search_messages (notmuch_query_t *query)\r
 221 > >                                      mail_query, string_query);\r
 222 > >     }\r
 223 > >  \r
 224 > > +   final_query = _notmuch_exclude_tags (query, final_query);\r
 225 > > +\r
 226 > >     enquire.set_weighting_scheme (Xapian::BoolWeight());\r
 227 > >  \r
 228 > >     switch (query->sort) {\r
 229 > > @@ -436,6 +467,8 @@ notmuch_query_count_messages (notmuch_query_t *query)\r
 230 > >                                      mail_query, string_query);\r
 231 > >     }\r
 232 > >  \r
 233 > > +   final_query = _notmuch_exclude_tags (query, final_query);\r
 234 > > +\r
 235 > >     enquire.set_weighting_scheme(Xapian::BoolWeight());\r
 236 > >     enquire.set_docid_order(Xapian::Enquire::ASCENDING);\r
 237 > >  \r
 238 > \r