28/2a92d79c8d4c94fb6a51932af459abde8b5213

   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 56372431FB6\r
   6         for <notmuch@notmuchmail.org>; Sun,  2 Sep 2012 00:52:25 -0700 (PDT)\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 SPCrILXDRfq2 for <notmuch@notmuchmail.org>;\r
  17         Sun,  2 Sep 2012 00:52:24 -0700 (PDT)\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 DA000431FAF\r
  22         for <notmuch@notmuchmail.org>; Sun,  2 Sep 2012 00:52:23 -0700 (PDT)\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 1T84yf-0002nP-7D; Sun, 02 Sep 2012 08:52:19 +0100\r
  27 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
  28         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  29         (envelope-from <m.walters@qmul.ac.uk>)\r
  30         id 1T84ye-0006fi-IO; Sun, 02 Sep 2012 08:52:17 +0100\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: Jameson Graef Rollins <jrollins@finestructure.net>,\r
  33         Notmuch Mail <notmuch@notmuchmail.org>\r
  34 Subject: Re: [PATCH 06/11] lib: store thread recipients in thread structure\r
  35 In-Reply-To: <1345427570-26518-7-git-send-email-jrollins@finestructure.net>\r
  36 References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>\r
  37         <1345427570-26518-2-git-send-email-jrollins@finestructure.net>\r
  38         <1345427570-26518-3-git-send-email-jrollins@finestructure.net>\r
  39         <1345427570-26518-4-git-send-email-jrollins@finestructure.net>\r
  40         <1345427570-26518-5-git-send-email-jrollins@finestructure.net>\r
  41         <1345427570-26518-6-git-send-email-jrollins@finestructure.net>\r
  42         <1345427570-26518-7-git-send-email-jrollins@finestructure.net>\r
  43 User-Agent: Notmuch/0.14+28~ge689a44 (http://notmuchmail.org) Emacs/23.4.1\r
  44         (x86_64-pc-linux-gnu)\r
  45 Date: Sun, 02 Sep 2012 08:52:15 +0100\r
  46 Message-ID: <87ligsx3o0.fsf@qmul.ac.uk>\r
  47 MIME-Version: 1.0\r
  48 Content-Type: text/plain; charset=us-ascii\r
  49 X-Sender-Host-Address: 93.97.24.31\r
  50 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  51 X-QM-Body-MD5: d7b69fec47cff9dcbc3c3c3e010f9da1 (of first 20000 bytes)\r
  52 X-SpamAssassin-Score: -2.3\r
  53 X-SpamAssassin-SpamBar: --\r
  54 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  55         determine if it is\r
  56         spam. We require at least 5.0 points to mark a message as spam.\r
  57         This message scored -2.3 points.\r
  58         Summary of the scoring: \r
  59         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  60         *      medium trust\r
  61         *      [138.37.6.40 listed in list.dnswl.org]\r
  62         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  63         provider *      (markwalters1009[at]gmail.com)\r
  64 X-QM-Scan-Virus: ClamAV says the message is clean\r
  65 X-BeenThere: notmuch@notmuchmail.org\r
  66 X-Mailman-Version: 2.1.13\r
  67 Precedence: list\r
  68 List-Id: "Use and development of the notmuch mail system."\r
  69         <notmuch.notmuchmail.org>\r
  70 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  72 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  73 List-Post: <mailto:notmuch@notmuchmail.org>\r
  74 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  75 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  76         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  77 X-List-Received-Date: Sun, 02 Sep 2012 07:52:25 -0000\r
  78 \r
  79 \r
  80 On Mon, 20 Aug 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:\r
  81 > This utilizes the new thread addresses struct to store thread\r
  82 > recipients, again in parallel to authors.\r
  83 >\r
  84 > Since message recipients are not stored in the database, including\r
  85 > recipients in the thread structure exacts a significant overhead as\r
  86 > the recipients are retrieved from the original message files.  Because\r
  87 > of this, a new boolean argument, include_recipients, is added to the\r
  88 > necessary functions (_notmuch_thread_create, _thread_add_message and\r
  89 > _thread_add_matched_message) that controls whether the recipients are\r
  90 > fetched and included.  If message recipients are ever stored in the\r
  91 > database this new argument could probably be removed.\r
  92 > ---\r
  93 \r
  94 Hi\r
  95 \r
  96 I have briefly looked through the series and overall it looks good.\r
  97 \r
  98 >  lib/notmuch-private.h |    3 +-\r
  99 >  lib/notmuch.h         |   14 +++++++++\r
 100 >  lib/query.cc          |    3 +-\r
 101 >  lib/thread.cc         |   77 +++++++++++++++++++++++++++++++++++++------------\r
 102 >  4 files changed, 76 insertions(+), 21 deletions(-)\r
 103 >\r
 104 > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h\r
 105 > index 27a41b6..32d1523 100644\r
 106 > --- a/lib/notmuch-private.h\r
 107 > +++ b/lib/notmuch-private.h\r
 108 > @@ -232,7 +232,8 @@ _notmuch_thread_create (void *ctx,\r
 109 >                       unsigned int seed_doc_id,\r
 110 >                       notmuch_doc_id_set_t *match_set,\r
 111 >                       notmuch_string_list_t *excluded_terms,\r
 112 > -                     notmuch_sort_t sort);\r
 113 > +                     notmuch_sort_t sort,\r
 114 > +                     notmuch_bool_t include_recipients);\r
 115 >  \r
 116 >  /* message.cc */\r
 117 >  \r
 118 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 119 > index 6acd38d..f9e71c1 100644\r
 120 > --- a/lib/notmuch.h\r
 121 > +++ b/lib/notmuch.h\r
 122 > @@ -759,6 +759,20 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread);\r
 123 >  const char *\r
 124 >  notmuch_thread_get_authors (notmuch_thread_t *thread);\r
 125 >  \r
 126 > +/* Get the recipients of 'thread'\r
 127 > + *\r
 128 > + * The returned string is a comma-separated list of the names of the\r
 129 > + * recipients of mail messages in the query results that belong to this\r
 130 > + * thread.\r
 131 > + *\r
 132 > + * The returned string belongs to 'thread' and as such, should not be\r
 133 > + * modified by the caller and will only be valid for as long as the\r
 134 > + * thread is valid, (which is until notmuch_thread_destroy or until\r
 135 > + * the query from which it derived is destroyed).\r
 136 > + */\r
 137 > +const char *\r
 138 > +notmuch_thread_get_recipients (notmuch_thread_t *thread);\r
 139 > +\r
 140 >  /* Get the subject of 'thread'\r
 141 >   *\r
 142 >   * The subject is taken from the first message (according to the query\r
 143 > diff --git a/lib/query.cc b/lib/query.cc\r
 144 > index e9c1a2d..54833a7 100644\r
 145 > --- a/lib/query.cc\r
 146 > +++ b/lib/query.cc\r
 147 > @@ -486,7 +486,8 @@ notmuch_threads_get (notmuch_threads_t *threads)\r
 148 >                                  doc_id,\r
 149 >                                  &threads->match_set,\r
 150 >                                  threads->query->exclude_terms,\r
 151 > -                                threads->query->sort);\r
 152 > +                                threads->query->sort,\r
 153 > +                                FALSE);\r
 154 >  }\r
 155 >  \r
 156 >  void\r
 157 > diff --git a/lib/thread.cc b/lib/thread.cc\r
 158 > index 757e143..baf07c2 100644\r
 159 > --- a/lib/thread.cc\r
 160 > +++ b/lib/thread.cc\r
 161 > @@ -37,6 +37,7 @@ struct visible _notmuch_thread {\r
 162 >      char *thread_id;\r
 163 >      char *subject;\r
 164 >      notmuch_thread_addresses_t *authors;\r
 165 > +    notmuch_thread_addresses_t *recipients;\r
 166 >      GHashTable *tags;\r
 167 >  \r
 168 >      notmuch_message_list_t *message_list;\r
 169 > @@ -63,6 +64,7 @@ static int\r
 170 >  _notmuch_thread_destructor (notmuch_thread_t *thread)\r
 171 >  {\r
 172 >      _notmuch_thread_addresses_destructor (thread->authors);\r
 173 > +    _notmuch_thread_addresses_destructor (thread->recipients);\r
 174 >      g_hash_table_unref (thread->tags);\r
 175 >      g_hash_table_unref (thread->message_hash);\r
 176 >      return 0;\r
 177 > @@ -204,14 +206,17 @@ _thread_cleanup_address (notmuch_thread_t *thread,\r
 178 >  static void\r
 179 >  _thread_add_message (notmuch_thread_t *thread,\r
 180 >                    notmuch_message_t *message,\r
 181 > -                  notmuch_string_list_t *exclude_terms)\r
 182 > +                  notmuch_string_list_t *exclude_terms,\r
 183 > +                  notmuch_bool_t include_recipients)\r
 184 >  {\r
 185 >      notmuch_tags_t *tags;\r
 186 >      const char *tag;\r
 187 > -    InternetAddressList *list = NULL;\r
 188 > +    InternetAddressList *from_list = NULL;\r
 189 > +    InternetAddressList *to_list = NULL;\r
 190 >      InternetAddress *address;\r
 191 >      const char *from, *author;\r
 192 > -    char *clean_author;\r
 193 > +    const char *to, *recipient;\r
 194 > +    char *clean_address;\r
 195 >  \r
 196 >      _notmuch_message_list_add_message (thread->message_list,\r
 197 >                                      talloc_steal (thread, message));\r
 198 > @@ -223,10 +228,9 @@ _thread_add_message (notmuch_thread_t *thread,\r
 199 >  \r
 200 >      from = notmuch_message_get_header (message, "from");\r
 201 >      if (from)\r
 202 > -     list = internet_address_list_parse_string (from);\r
 203 > -\r
 204 > -    if (list) {\r
 205 > -     address = internet_address_list_get_address (list, 0);\r
 206 > +     from_list = internet_address_list_parse_string (from);\r
 207 > +    if (from_list) {\r
 208 > +     address = internet_address_list_get_address (from_list, 0);\r
 209 >       if (address) {\r
 210 >           author = internet_address_get_name (address);\r
 211 >           if (author == NULL) {\r
 212 > @@ -234,11 +238,32 @@ _thread_add_message (notmuch_thread_t *thread,\r
 213 >               mailbox = INTERNET_ADDRESS_MAILBOX (address);\r
 214 >               author = internet_address_mailbox_get_addr (mailbox);\r
 215 >           }\r
 216 > -         clean_author = _thread_cleanup_author (thread, author, from);\r
 217 > -         _thread_add_address (thread->authors, clean_author, FALSE);\r
 218 > -         notmuch_message_set_author (message, clean_author);\r
 219 > +         clean_address = _thread_cleanup_address (thread, author, from);\r
 220 > +         _thread_add_address (thread->authors, clean_address, FALSE);\r
 221 > +         notmuch_message_set_author (message, clean_address);\r
 222 > +     }\r
 223 > +     g_object_unref (G_OBJECT (from_list));\r
 224 > +    }\r
 225 > +\r
 226 > +    if (include_recipients) {\r
 227 > +    to = notmuch_message_get_header (message, "to");\r
 228 > +    if (to)\r
 229 > +     to_list = internet_address_list_parse_string (to);\r
 230 > +    if (to_list) {\r
 231 > +     address = internet_address_list_get_address (to_list, 0);\r
 232 \r
 233 I think you are just adding the first address on the to: line. Is that\r
 234 deliberate? The comment before notmuch_thread_get_recipients (above)\r
 235 suggests all recipients are being added, but doing so may make the\r
 236 string too large to be useful.\r
 237 \r
 238 Best wishes\r
 239 \r
 240 Mark\r
 241 \r
 242 \r
 243 \r
 244 > +     if (address) {\r
 245 > +         recipient = internet_address_get_name (address);\r
 246 > +         if (recipient == NULL) {\r
 247 > +             InternetAddressMailbox *mailbox;\r
 248 > +             mailbox = INTERNET_ADDRESS_MAILBOX (address);\r
 249 > +             recipient = internet_address_mailbox_get_addr (mailbox);\r
 250 > +         }\r
 251 > +         clean_address = _thread_cleanup_address (thread, recipient, to);\r
 252 > +         _thread_add_address (thread->recipients, clean_address, FALSE);\r
 253 > +         notmuch_message_set_recipients (message, clean_address);\r
 254 >       }\r
 255 > -     g_object_unref (G_OBJECT (list));\r
 256 > +     g_object_unref (G_OBJECT (to_list));\r
 257 > +    }\r
 258 >      }\r
 259 >  \r
 260 >      if (! thread->subject) {\r
 261 > @@ -301,7 +326,8 @@ _thread_set_subject_from_message (notmuch_thread_t *thread,\r
 262 >  static void\r
 263 >  _thread_add_matched_message (notmuch_thread_t *thread,\r
 264 >                            notmuch_message_t *message,\r
 265 > -                          notmuch_sort_t sort)\r
 266 > +                          notmuch_sort_t sort,\r
 267 > +                          notmuch_bool_t include_recipients)\r
 268 >  {\r
 269 >      time_t date;\r
 270 >      notmuch_message_t *hashed_message;\r
 271 > @@ -331,6 +357,8 @@ _thread_add_matched_message (notmuch_thread_t *thread,\r
 272 >      }\r
 273 >  \r
 274 >      _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE);\r
 275 > +    if (include_recipients)\r
 276 > +    _thread_add_address (thread->recipients, notmuch_message_get_recipients (hashed_message), TRUE);\r
 277 >  }\r
 278 >  \r
 279 >  static void\r
 280 > @@ -399,10 +427,10 @@ _thread_addresses_init (const void *ctx)\r
 281 >   *\r
 282 >   * Creating the thread will perform a database search to get all\r
 283 >   * messages belonging to the thread and will get the first subject\r
 284 > - * line, the total count of messages, and all authors in the thread.\r
 285 > - * Each message in the thread is checked against match_set to allow\r
 286 > - * for a separate count of matched messages, and to allow a viewer to\r
 287 > - * display these messages differently.\r
 288 > + * line, the total count of messages, and all authors and recipients\r
 289 > + * of the thread.  Each message in the thread is checked against\r
 290 > + * match_set to allow for a separate count of matched messages, and to\r
 291 > + * allow a viewer to display these messages differently.\r
 292 >   *\r
 293 >   * Here, 'ctx' is talloc context for the resulting thread object.\r
 294 >   *\r
 295 > @@ -414,7 +442,8 @@ _notmuch_thread_create (void *ctx,\r
 296 >                       unsigned int seed_doc_id,\r
 297 >                       notmuch_doc_id_set_t *match_set,\r
 298 >                       notmuch_string_list_t *exclude_terms,\r
 299 > -                     notmuch_sort_t sort)\r
 300 > +                     notmuch_sort_t sort,\r
 301 > +                     notmuch_bool_t include_recipients)\r
 302 >  {\r
 303 >      notmuch_thread_t *thread;\r
 304 >      notmuch_message_t *seed_message;\r
 305 > @@ -453,6 +482,9 @@ _notmuch_thread_create (void *ctx,\r
 306 >      thread->authors = _thread_addresses_init (thread);\r
 307 >      if (unlikely (thread->authors == NULL))\r
 308 >       return NULL;\r
 309 > +    thread->recipients = _thread_addresses_init (thread);\r
 310 > +    if (unlikely (thread->recipients == NULL))\r
 311 > +     return NULL;\r
 312 >  \r
 313 >      thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal,\r
 314 >                                         free, NULL);\r
 315 > @@ -486,11 +518,11 @@ _notmuch_thread_create (void *ctx,\r
 316 >       if (doc_id == seed_doc_id)\r
 317 >           message = seed_message;\r
 318 >  \r
 319 > -     _thread_add_message (thread, message, exclude_terms);\r
 320 > +     _thread_add_message (thread, message, exclude_terms, include_recipients);\r
 321 >  \r
 322 >       if ( _notmuch_doc_id_set_contains (match_set, doc_id)) {\r
 323 >           _notmuch_doc_id_set_remove (match_set, doc_id);\r
 324 > -         _thread_add_matched_message (thread, message, sort);\r
 325 > +         _thread_add_matched_message (thread, message, sort, include_recipients);\r
 326 >       }\r
 327 >  \r
 328 >       _notmuch_message_close (message);\r
 329 > @@ -499,6 +531,7 @@ _notmuch_thread_create (void *ctx,\r
 330 >      notmuch_query_destroy (thread_id_query);\r
 331 >  \r
 332 >      _resolve_thread_addresses_string (thread->authors);\r
 333 > +    _resolve_thread_addresses_string (thread->recipients);\r
 334 >  \r
 335 >      _resolve_thread_relationships (thread);\r
 336 >  \r
 337 > @@ -536,6 +569,12 @@ notmuch_thread_get_authors (notmuch_thread_t *thread)\r
 338 >  }\r
 339 >  \r
 340 >  const char *\r
 341 > +notmuch_thread_get_recipients (notmuch_thread_t *thread)\r
 342 > +{\r
 343 > +    return thread->recipients->string;\r
 344 > +}\r
 345 > +\r
 346 > +const char *\r
 347 >  notmuch_thread_get_subject (notmuch_thread_t *thread)\r
 348 >  {\r
 349 >      return thread->subject;\r
 350 > -- \r
 351 > 1.7.10.4\r
 352 >\r
 353 > _______________________________________________\r
 354 > notmuch mailing list\r
 355 > notmuch@notmuchmail.org\r
 356 > http://notmuchmail.org/mailman/listinfo/notmuch\r