7f/909465b8739a48ac5572414a48b77f0b7b0900

   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 A8104429E54\r
   6         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 18:45:57 -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 fsT7AA8+VDIP for <notmuch@notmuchmail.org>;\r
  16         Mon, 23 Jan 2012 18:45:57 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-5.mit.edu (DMZ-MAILSEC-SCANNER-5.MIT.EDU\r
  18         [18.7.68.34])\r
  19         by olra.theworths.org (Postfix) with ESMTP id D0986429E21\r
  20         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 18:45:56 -0800 (PST)\r
  21 X-AuditID: 12074422-b7fd66d0000008f9-6a-4f1e1b620d8d\r
  22 Received: from mailhub-auth-1.mit.edu ( [18.9.21.35])\r
  23         by dmz-mailsec-scanner-5.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 3B.DD.02297.26B1E1F4; Mon, 23 Jan 2012 21:45:54 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-1.mit.edu (8.13.8/8.9.2) with ESMTP id q0O2jrWj008982; \r
  27         Mon, 23 Jan 2012 21:45:54 -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 q0O2jqOI011949\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Mon, 23 Jan 2012 21:45:53 -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 1RpWNt-0004Sg-VU; Mon, 23 Jan 2012 21:45:22 -0500\r
  37 Date: Mon, 23 Jan 2012 21:45:21 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Mark Walters <markwalters1009@gmail.com>\r
  40 Subject: Re: [RFC PATCH 2/4] Add NOTMUCH_MESSAGE_FLAG_EXCLUDED flag\r
  41 Message-ID: <20120124024521.GY16740@mit.edu>\r
  42 References: <20120124011609.GX16740@mit.edu>\r
  43         <1327367923-18228-2-git-send-email-markwalters1009@gmail.com>\r
  44 MIME-Version: 1.0\r
  45 Content-Type: text/plain; charset=us-ascii\r
  46 Content-Disposition: inline\r
  47 In-Reply-To: <1327367923-18228-2-git-send-email-markwalters1009@gmail.com>\r
  48 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  49 X-Brightmail-Tracker:\r
  50  H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IR4hRV1k2SlvM3eL9Tw2L1XB6L6zdnMjsw\r
  51         eeycdZfd49mqW8wBTFFcNimpOZllqUX6dglcGTv+bGAq2KlYcWvBc6YGxhbpLkZODgkBE4mf\r
  52         d96zQdhiEhfurQeyuTiEBPYxSizrnMoK4WxglJh29QeUc5JJYvOiq1BlSxgltn3dww7SzyKg\r
  53         KrFr2RZWEJtNQENi2/7ljCC2iICOxO1DC8BqmAWkJb79bmYCsYUFnCWeN30Fq+cFqmncvA3I\r
  54         5gAamiNxvjcOIiwocXLmExaIVi2JG/9eMoGUgIxZ/o8DJMwp4CWxbMllZhBbVEBFYsrJbWwT\r
  55         GIVmIemehaR7FkL3AkbmVYyyKblVurmJmTnFqcm6xcmJeXmpRbqmermZJXqpKaWbGEFBze6i\r
  56         tIPx50GlQ4wCHIxKPLwSM2X9hVgTy4orcw8xSnIwKYnyxknJ+QvxJeWnVGYkFmfEF5XmpBYf\r
  57         YpTgYFYS4VU7B1TOm5JYWZValA+TkuZgURLnVdd65yckkJ5YkpqdmlqQWgSTleHgUJLgnQcy\r
  58         VLAoNT21Ii0zpwQhzcTBCTKcB2j4DZAa3uKCxNzizHSI/ClGRSlx3uUgCQGQREZpHlwvLOm8\r
  59         YhQHekWY9zpIFQ8wYcF1vwIazAQ0mCNPCmRwSSJCSqqBUbj5tefWnROWbHF6t22rVIusnvYb\r
  60         lsqHLUJBa3d1/25dJft7FtucfYcrJZSSk5UvB4uk6968aSpXujeoZFrOEkuuo92LuQU/HfrX\r
  61         kZHy8MyC13Mz2G+fcbBYPD/hx6ar31hXsHc4x5TfLbZq2Vb1eQnf7A5RX3eZmd9yztdMr1sc\r
  62         +8Ds5vJ8diWW4oxEQy3mouJEADVg1bQVAwAA\r
  63 Cc: notmuch@notmuchmail.org\r
  64 X-BeenThere: notmuch@notmuchmail.org\r
  65 X-Mailman-Version: 2.1.13\r
  66 Precedence: list\r
  67 List-Id: "Use and development of the notmuch mail system."\r
  68         <notmuch.notmuchmail.org>\r
  69 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  70         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  71 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  72 List-Post: <mailto:notmuch@notmuchmail.org>\r
  73 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  74 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  75         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  76 X-List-Received-Date: Tue, 24 Jan 2012 02:45:57 -0000\r
  77 \r
  78 The overall structure of this series looks great.  There's obviously a\r
  79 lot of clean up to do, but I'll reply with a few high-level comments.\r
  80 \r
  81 Quoth Mark Walters on Jan 24 at  1:18 am:\r
  82 > Form excluded doc_ids set and use that to exclude messages.\r
  83 > Should be no functional change.\r
  84 > \r
  85 > ---\r
  86 >  lib/notmuch-private.h |    1 +\r
  87 >  lib/query.cc          |   28 ++++++++++++++++++++++++++--\r
  88 >  2 files changed, 27 insertions(+), 2 deletions(-)\r
  89 > \r
  90 > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h\r
  91 > index 7bf153e..e791bb0 100644\r
  92 > --- a/lib/notmuch-private.h\r
  93 > +++ b/lib/notmuch-private.h\r
  94 > @@ -401,6 +401,7 @@ typedef struct _notmuch_message_list {\r
  95 >   */\r
  96 >  struct visible _notmuch_messages {\r
  97 >      notmuch_bool_t is_of_list_type;\r
  98 > +    notmuch_doc_id_set_t *excluded_doc_ids;\r
  99 >      notmuch_message_node_t *iterator;\r
 100 >  };\r
 101 >  \r
 102 > diff --git a/lib/query.cc b/lib/query.cc\r
 103 > index c25b301..92fa834 100644\r
 104 > --- a/lib/query.cc\r
 105 > +++ b/lib/query.cc\r
 106 > @@ -57,6 +57,11 @@ struct visible _notmuch_threads {\r
 107 >      notmuch_doc_id_set_t match_set;\r
 108 >  };\r
 109 >  \r
 110 > +static notmuch_bool_t\r
 111 > +_notmuch_doc_id_set_init (void *ctx,\r
 112 > +                       notmuch_doc_id_set_t *doc_ids,\r
 113 > +                       GArray *arr);\r
 114 > +\r
 115 >  notmuch_query_t *\r
 116 >  notmuch_query_create (notmuch_database_t *notmuch,\r
 117 >                     const char *query_string)\r
 118 > @@ -173,6 +178,7 @@ notmuch_query_search_messages (notmuch_query_t *query)\r
 119 >                                                  "mail"));\r
 120 >       Xapian::Query string_query, final_query, exclude_query;\r
 121 >       Xapian::MSet mset;\r
 122 > +     Xapian::MSetIterator iterator;\r
 123 >       unsigned int flags = (Xapian::QueryParser::FLAG_BOOLEAN |\r
 124 >                             Xapian::QueryParser::FLAG_PHRASE |\r
 125 >                             Xapian::QueryParser::FLAG_LOVEHATE |\r
 126 > @@ -193,8 +199,21 @@ notmuch_query_search_messages (notmuch_query_t *query)\r
 127 >  \r
 128 >       exclude_query = _notmuch_exclude_tags (query, final_query);\r
 129 >  \r
 130 > -     final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,\r
 131 > -                                      final_query, exclude_query);\r
 132 > +     enquire.set_weighting_scheme (Xapian::BoolWeight());\r
 133 > +     enquire.set_query (exclude_query);\r
 134 > +\r
 135 > +     mset = enquire.get_mset (0, notmuch->xapian_db->get_doccount ());\r
 136 > +\r
 137 > +     GArray *excluded_doc_ids = g_array_new (FALSE, FALSE, sizeof (unsigned int));\r
 138 > +\r
 139 > +     for (iterator = mset.begin (); iterator != mset.end (); iterator++)\r
 140 > +     {\r
 141 > +         unsigned int doc_id = *iterator;\r
 142 > +         g_array_append_val (excluded_doc_ids, doc_id);\r
 143 > +     }\r
 144 > +     messages->base.excluded_doc_ids = talloc (query, _notmuch_doc_id_set);\r
 145 > +     _notmuch_doc_id_set_init (query, messages->base.excluded_doc_ids,\r
 146 > +                               excluded_doc_ids);\r
 147 \r
 148 This might be inefficient for message-only queries, since it will\r
 149 fetch *all* excluded docids.  This highlights a basic difference\r
 150 between message and thread search: thread search can return messages\r
 151 that don't match the original query and hence needs to know all\r
 152 potentially excluded messages, while message search can only return\r
 153 messages that match the original query.\r
 154 \r
 155 It's entirely possible this doesn't matter because Xapian probably\r
 156 still needs to fetch the full posting lists of the excluded terms, but\r
 157 it would be worth doing a quick/hacky benchmark to verify this, with\r
 158 enough excluded messages to make the cost non-trivial.\r
 159 \r
 160 If it does matter, you could pass in a flag indicating if the exclude\r
 161 query should be limited by the original query or not.  Or you could do\r
 162 the limited exclude query in notmuch_query_search_messages and a\r
 163 separate open-ended exclude query in notmuch_query_search_threads.\r
 164 \r
 165 >  \r
 166 >       enquire.set_weighting_scheme (Xapian::BoolWeight());\r
 167 >  \r
 168 > @@ -294,6 +313,11 @@ _notmuch_mset_messages_move_to_next (notmuch_messages_t *messages)\r
 169 >      mset_messages = (notmuch_mset_messages_t *) messages;\r
 170 >  \r
 171 >      mset_messages->iterator++;\r
 172 > +\r
 173 > +    while ((mset_messages->iterator != mset_messages->iterator_end) &&\r
 174 > +        (_notmuch_doc_id_set_contains (messages->excluded_doc_ids,\r
 175 > +                                       *mset_messages->iterator)))\r
 176 > +     mset_messages->iterator++;\r
 177 \r
 178 This seemed a little weird, since you remove it in the next patch.  Is\r
 179 this just to keep the tests happy?  (If so, it would be worth\r
 180 mentioning in the commit message; other reviewers will definitely have\r
 181 the same question.)\r
 182 \r
 183 >  }\r
 184 >  \r
 185 >  static notmuch_bool_t\r