c9/35b9fb8efffce2bbae939f2740ce25198e0b27

   1 Return-Path: <m.walters@qmul.ac.uk>\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 E4F21429E34\r
   6         for <notmuch@notmuchmail.org>; Tue, 31 Jan 2012 03:44:38 -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: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id g7IAq5B7TMLf for <notmuch@notmuchmail.org>;\r
  17         Tue, 31 Jan 2012 03:44:38 -0800 (PST)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id B7ED6431E64\r
  22         for <notmuch@notmuchmail.org>; Tue, 31 Jan 2012 03:44:37 -0800 (PST)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1RsC8Z-0003xS-Nt; Tue, 31 Jan 2012 11:44:36 +0000\r
  27 Received: from 94-192-233-223.zone6.bethere.co.uk ([94.192.233.223]\r
  28         helo=localhost)\r
  29         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  30         (envelope-from <m.walters@qmul.ac.uk>)\r
  31         id 1RsC8Z-0006ft-6z; Tue, 31 Jan 2012 11:44:35 +0000\r
  32 From: Mark Walters <markwalters1009@gmail.com>\r
  33 To: Austin Clements <amdragon@MIT.EDU>\r
  34 Subject: Re: [PATCH 3/7] lib: Make notmuch_query_search_messages set the\r
  35         exclude flag\r
  36 In-Reply-To: <20120131044352.GZ17991@mit.edu>\r
  37 References: <8762fu4aqt.fsf@qmul.ac.uk>\r
  38         <1327862394-14334-3-git-send-email-markwalters1009@gmail.com>\r
  39         <20120131044352.GZ17991@mit.edu>\r
  40 User-Agent: Notmuch/0.11+137~g98adc3d (http://notmuchmail.org) Emacs/23.2.1\r
  41         (i486-pc-linux-gnu)\r
  42 Date: Tue, 31 Jan 2012 11:45:41 +0000\r
  43 Message-ID: <87y5soccyi.fsf@qmul.ac.uk>\r
  44 MIME-Version: 1.0\r
  45 Content-Type: text/plain; charset=us-ascii\r
  46 X-Sender-Host-Address: 94.192.233.223\r
  47 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  48 X-QM-Body-MD5: a1fd102ac7732633f006b6340c8647ed (of first 20000 bytes)\r
  49 X-SpamAssassin-Score: -1.8\r
  50 X-SpamAssassin-SpamBar: -\r
  51 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  52         determine if it is\r
  53         spam. We require at least 5.0 points to mark a message as spam.\r
  54         This message scored -1.8 points.\r
  55         Summary of the scoring: \r
  56         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  57         *      medium trust\r
  58         *      [138.37.6.40 listed in list.dnswl.org]\r
  59         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  60         provider *      (markwalters1009[at]gmail.com)\r
  61         * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay\r
  62         *      domain\r
  63         *  0.5 AWL AWL: From: address is in the auto white-list\r
  64 X-QM-Scan-Virus: ClamAV says the message is clean\r
  65 Cc: notmuch@notmuchmail.org\r
  66 X-BeenThere: notmuch@notmuchmail.org\r
  67 X-Mailman-Version: 2.1.13\r
  68 Precedence: list\r
  69 List-Id: "Use and development of the notmuch mail system."\r
  70         <notmuch.notmuchmail.org>\r
  71 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  72         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  73 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  74 List-Post: <mailto:notmuch@notmuchmail.org>\r
  75 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  76 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  77         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  78 X-List-Received-Date: Tue, 31 Jan 2012 11:44:39 -0000\r
  79 \r
  80 On Mon, 30 Jan 2012 23:43:52 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  81 > Quoth Mark Walters on Jan 29 at  6:39 pm:\r
  82 > > Add a flag NOTMUCH_MESSAGE_FLAG_EXCLUDED which is set by\r
  83 > > notmuch_query_search_messages for excluded messages. Also add an\r
  84 > > option omit_excluded_messages to the search that we do not want the\r
  85 > > excludes at all.\r
  86 > > \r
  87 > > This exclude flag will be added to notmuch_query_search threads in the\r
  88 > > next patch.\r
  89 > > ---\r
  90 > >  lib/notmuch-private.h |    1 +\r
  91 > >  lib/notmuch.h         |    8 ++++++-\r
  92 > >  lib/query.cc          |   52 +++++++++++++++++++++++++++++++++++++++++++++---\r
  93 > >  3 files changed, 56 insertions(+), 5 deletions(-)\r
  94 > > \r
  95 > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h\r
  96 > > index 7bf153e..e791bb0 100644\r
  97 > > --- a/lib/notmuch-private.h\r
  98 > > +++ b/lib/notmuch-private.h\r
  99 > > @@ -401,6 +401,7 @@ typedef struct _notmuch_message_list {\r
 100 > >   */\r
 101 > >  struct visible _notmuch_messages {\r
 102 > >      notmuch_bool_t is_of_list_type;\r
 103 > > +    notmuch_doc_id_set_t *excluded_doc_ids;\r
 104 > \r
 105 > I might be following the diff wrong, but shouldn't this be a field of\r
 106 > notmuch_mset_messages_t?  (Then it also doesn't have to be a pointer,\r
 107 > which is really how notmuch_doc_id_set_t was designed to be used.)\r
 108 \r
 109 I will need to think about that.\r
 110 \r
 111 > >      notmuch_message_node_t *iterator;\r
 112 > >  };\r
 113 > >  \r
 114 > > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 115 > > index 7929fe7..740d005 100644\r
 116 > > --- a/lib/notmuch.h\r
 117 > > +++ b/lib/notmuch.h\r
 118 > > @@ -449,6 +449,11 @@ typedef enum {\r
 119 > >  const char *\r
 120 > >  notmuch_query_get_query_string (notmuch_query_t *query);\r
 121 > >  \r
 122 > > +/* specify whether to results should omit the excluded results rather\r
 123 > > + * than just marking them excluded */\r
 124 > > +void\r
 125 > > +notmuch_query_set_omit_excluded_messages (notmuch_query_t *query, notmuch_bool_t omit);\r
 126 > > +\r
 127 > \r
 128 > I don't think we should add this API.  The library behavior will not\r
 129 > change for library users that don't use excludes and library users\r
 130 > that do use excludes should by aware of the excluded flag and do the\r
 131 > appropriate thing.\r
 132 > \r
 133 > I can see why this is handy in some cases, but I don't think it\r
 134 > provides enough utility to warrant becoming part of the permanent and\r
 135 > minimal library interface.\r
 136 \r
 137 This is really a performance improvement: suppose that there are lots of\r
 138 threads that only match in excluded messages. Then without this flag we\r
 139 will spend lots of time constructing the thread only for it to be\r
 140 ignored. (In contrived situations this could be arbitrarily slower.)\r
 141 \r
 142 Note the benchmarks were against master with the exclude code switched\r
 143 off so that I was comparing the creation of the same threads. Sorry if I\r
 144 didn't make that clear.\r
 145 \r
 146 > >  /* Specify the sorting desired for this query. */\r
 147 > >  void\r
 148 > >  notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort);\r
 149 > > @@ -895,7 +900,8 @@ notmuch_message_get_filenames (notmuch_message_t *message);\r
 150 > >  \r
 151 > >  /* Message flags */\r
 152 > >  typedef enum _notmuch_message_flag {\r
 153 > > -    NOTMUCH_MESSAGE_FLAG_MATCH\r
 154 > > +    NOTMUCH_MESSAGE_FLAG_MATCH,\r
 155 > > +    NOTMUCH_MESSAGE_FLAG_EXCLUDED\r
 156 > >  } notmuch_message_flag_t;\r
 157 > >  \r
 158 > >  /* Get a value of a flag for the email corresponding to 'message'. */\r
 159 > > diff --git a/lib/query.cc b/lib/query.cc\r
 160 > > index c25b301..7d165d2 100644\r
 161 > > --- a/lib/query.cc\r
 162 > > +++ b/lib/query.cc\r
 163 > > @@ -28,6 +28,7 @@ struct _notmuch_query {\r
 164 > >      const char *query_string;\r
 165 > >      notmuch_sort_t sort;\r
 166 > >      notmuch_string_list_t *exclude_terms;\r
 167 > > +    notmuch_bool_t omit_excluded_messages;\r
 168 > >  };\r
 169 > >  \r
 170 > >  typedef struct _notmuch_mset_messages {\r
 171 > > @@ -57,6 +58,12 @@ struct visible _notmuch_threads {\r
 172 > >      notmuch_doc_id_set_t match_set;\r
 173 > >  };\r
 174 > >  \r
 175 > > +/* we need this in the message functions so forward declare */\r
 176 > \r
 177 > Comments should start with a capital letter and end with a period.\r
 178 > (The code isn't completely consistent about this, but it is something\r
 179 > we're codifying in the upcoming style guide.)\r
 180 \r
 181 Will fix\r
 182 \r
 183 > > +static notmuch_bool_t\r
 184 > > +_notmuch_doc_id_set_init (void *ctx,\r
 185 > > +                     notmuch_doc_id_set_t *doc_ids,\r
 186 > > +                     GArray *arr);\r
 187 > > +\r
 188 > >  notmuch_query_t *\r
 189 > >  notmuch_query_create (notmuch_database_t *notmuch,\r
 190 > >                   const char *query_string)\r
 191 > > @@ -79,6 +86,8 @@ notmuch_query_create (notmuch_database_t *notmuch,\r
 192 > >  \r
 193 > >      query->exclude_terms = _notmuch_string_list_create (query);\r
 194 > >  \r
 195 > > +    query->omit_excluded_messages = FALSE;\r
 196 > > +\r
 197 > >      return query;\r
 198 > >  }\r
 199 > >  \r
 200 > > @@ -89,6 +98,12 @@ notmuch_query_get_query_string (notmuch_query_t *query)\r
 201 > >  }\r
 202 > >  \r
 203 > >  void\r
 204 > > +notmuch_query_set_omit_excluded_messages (notmuch_query_t *query, notmuch_bool_t omit)\r
 205 > > +{\r
 206 > > +    query->omit_excluded_messages = omit;\r
 207 > > +}\r
 208 > > +\r
 209 > > +void\r
 210 > >  notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort)\r
 211 > >  {\r
 212 > >      query->sort = sort;\r
 213 > > @@ -173,6 +188,7 @@ notmuch_query_search_messages (notmuch_query_t *query)\r
 214 > >                                                "mail"));\r
 215 > >     Xapian::Query string_query, final_query, exclude_query;\r
 216 > >     Xapian::MSet mset;\r
 217 > > +   Xapian::MSetIterator iterator;\r
 218 > >     unsigned int flags = (Xapian::QueryParser::FLAG_BOOLEAN |\r
 219 > >                           Xapian::QueryParser::FLAG_PHRASE |\r
 220 > >                           Xapian::QueryParser::FLAG_LOVEHATE |\r
 221 > > @@ -190,11 +206,35 @@ notmuch_query_search_messages (notmuch_query_t *query)\r
 222 > >         final_query = Xapian::Query (Xapian::Query::OP_AND,\r
 223 > >                                      mail_query, string_query);\r
 224 > >     }\r
 225 > > +   messages->base.excluded_doc_ids = NULL;\r
 226 > > +\r
 227 > > +   if (query->exclude_terms) {\r
 228 > > +       exclude_query = _notmuch_exclude_tags (query, final_query);\r
 229 > > +       exclude_query = Xapian::Query (Xapian::Query::OP_AND,\r
 230 > > +                                      exclude_query, final_query);\r
 231 > > +\r
 232 > > +       if (query->omit_excluded_messages)\r
 233 > > +           final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,\r
 234 > > +                                        final_query, exclude_query);\r
 235 > > +       else {\r
 236 > > +           enquire.set_weighting_scheme (Xapian::BoolWeight());\r
 237 > > +           enquire.set_query (exclude_query);\r
 238 > > +\r
 239 > > +           mset = enquire.get_mset (0, notmuch->xapian_db->get_doccount ());\r
 240 > > +\r
 241 > > +           GArray *excluded_doc_ids = g_array_new (FALSE, FALSE, sizeof (unsigned int));\r
 242 > > +\r
 243 > > +           for (iterator = mset.begin (); iterator != mset.end (); iterator++)\r
 244 > > +           {\r
 245 > \r
 246 > No newline before the brace.\r
 247 \r
 248 Will fix.\r
 249 \r
 250 > \r
 251 > > +               unsigned int doc_id = *iterator;\r
 252 > > +               g_array_append_val (excluded_doc_ids, doc_id);\r
 253 > > +           }\r
 254 > > +           messages->base.excluded_doc_ids = talloc (query, _notmuch_doc_id_set);\r
 255 > > +           _notmuch_doc_id_set_init (query, messages->base.excluded_doc_ids,\r
 256 > > +                                     excluded_doc_ids);\r
 257 > \r
 258 > Don't forget to g_array_unref excluded_doc_ids.\r
 259 \r
 260 Yes I will add that.\r
 261 \r
 262 > > +       }\r
 263 > > +   }\r
 264 > >  \r
 265 > > -   exclude_query = _notmuch_exclude_tags (query, final_query);\r
 266 > > -\r
 267 > > -   final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,\r
 268 > > -                                    final_query, exclude_query);\r
 269 > >  \r
 270 > >     enquire.set_weighting_scheme (Xapian::BoolWeight());\r
 271 > >  \r
 272 > > @@ -283,6 +323,10 @@ _notmuch_mset_messages_get (notmuch_messages_t *messages)\r
 273 > >     INTERNAL_ERROR ("a messages iterator contains a non-existent document ID.\n");\r
 274 > >      }\r
 275 > >  \r
 276 > > +    if ((messages->excluded_doc_ids) &&\r
 277 > > +   (_notmuch_doc_id_set_contains (messages->excluded_doc_ids, doc_id)))\r
 278 > \r
 279 > No need for so many parens (just a nit).\r
 280 \r
 281 Will fix.\r
 282 \r
 283 > > +   notmuch_message_set_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED, TRUE);\r
 284 > > +\r
 285 > >      return message;\r
 286 > >  }\r
 287 > >  \r
 288 \r
 289 Thanks\r
 290 \r
 291 Mark\r
 292 \r