--- /dev/null
+Return-Path: <amdragon@mit.edu>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 42822431FAF\r
+ for <notmuch@notmuchmail.org>; Sat, 8 Sep 2012 10:24:10 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id kVpP1Lkx8YWX for <notmuch@notmuchmail.org>;\r
+ Sat, 8 Sep 2012 10:24:06 -0700 (PDT)\r
+Received: from dmz-mailsec-scanner-1.mit.edu (DMZ-MAILSEC-SCANNER-1.MIT.EDU\r
+ [18.9.25.12])\r
+ by olra.theworths.org (Postfix) with ESMTP id 818DA431FAE\r
+ for <notmuch@notmuchmail.org>; Sat, 8 Sep 2012 10:24:06 -0700 (PDT)\r
+X-AuditID: 1209190c-b7fd26d0000008d9-30-504b7f362ae9\r
+Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
+ by dmz-mailsec-scanner-1.mit.edu (Symantec Messaging Gateway) with SMTP\r
+ id 5C.5B.02265.63F7B405; Sat, 8 Sep 2012 13:24:06 -0400 (EDT)\r
+Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
+ by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id q88HO53C019056; \r
+ Sat, 8 Sep 2012 13:24:05 -0400\r
+Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
+ (authenticated bits=0)\r
+ (User authenticated as amdragon@ATHENA.MIT.EDU)\r
+ by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q88HO39C026315\r
+ (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
+ Sat, 8 Sep 2012 13:24:04 -0400 (EDT)\r
+Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
+ (envelope-from <amdragon@mit.edu>)\r
+ id 1TAOlH-00061Z-KL; Sat, 08 Sep 2012 13:24:03 -0400\r
+Date: Sat, 8 Sep 2012 13:24:03 -0400\r
+From: Austin Clements <amdragon@MIT.EDU>\r
+To: Jameson Graef Rollins <jrollins@finestructure.net>\r
+Subject: Re: [PATCH 01/11] lib: new thread addresses structure\r
+Message-ID: <20120908172403.GA21371@mit.edu>\r
+References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>\r
+ <1345427570-26518-2-git-send-email-jrollins@finestructure.net>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+Content-Disposition: inline\r
+In-Reply-To: <1345427570-26518-2-git-send-email-jrollins@finestructure.net>\r
+User-Agent: Mutt/1.5.21 (2010-09-15)\r
+X-Brightmail-Tracker:\r
+ H4sIAAAAAAAAA+NgFupnleLIzCtJLcpLzFFi42IR4hTV1jWr9w4wuNHBZrFnn5fF9ZszmR2Y\r
+ PO6e5vJ4tuoWcwBTFJdNSmpOZllqkb5dAlfG+V/7mQru2VS8ffiMtYGxz6CLkZNDQsBE4tbS\r
+ C6wQtpjEhXvr2boYuTiEBPYxSnRvWcUI4axnlDjV9YoZwjnBJPFq1ncoZwmjxKM5n9lB+lkE\r
+ VCSmzdrEBGKzCWhIbNu/nBHEFhEwk+j58gfMZhbQkti68QOYLSxgJzHzRwMziM0roCOxYNIk\r
+ Foih3YwSHQcmQyUEJU7OfMIC03zj30ugBRxAtrTE8n8cIGFOAW+JyfdXgf0gCnTDlJPb2CYw\r
+ Cs1C0j0LSfcshO4FjMyrGGVTcqt0cxMzc4pTk3WLkxPz8lKLdA31cjNL9FJTSjcxgsNakmcH\r
+ 45uDSocYBTgYlXh4N8h5BQixJpYVV+YeYpTkYFIS5d1d4x0gxJeUn1KZkVicEV9UmpNafIhR\r
+ goNZSYT3ejpQjjclsbIqtSgfJiXNwaIkzns55aa/kEB6YklqdmpqQWoRTFaGg0NJgre8DqhR\r
+ sCg1PbUiLTOnBCHNxMEJMpwHaPhKkBre4oLE3OLMdIj8KUZdjtk3V9xnFGLJy89LlRLnXQFS\r
+ JABSlFGaBzcHlo5eMYoDvSXMWw1SxQNMZXCTXgEtYQJaIvLMA2RJSSJCSqqBceW26XkmE7mc\r
+ XDd7CdaHpYU+Zyh4IKnTsKFFR1+d8W7PgzeeO0p/sG+t3uI3QVi85N4dnYwniceZeQ9xq4gu\r
+ 0ZyztE1wafGF0t+l1fdd3ma+bJEy/9G0QHzPiolbvh6bqdWy4LIzp4ZdeeO159UTa6cHdpy5\r
+ Z/nCnFPSPVBzoTCbj5tUnP4PJZbijERDLeai4kQAtr/5xiIDAAA=\r
+Cc: Notmuch Mail <notmuch@notmuchmail.org>\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Sat, 08 Sep 2012 17:24:10 -0000\r
+\r
+Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm:\r
+> This new structure holds addresses associated with a thread, both\r
+> matched and unmatched. Initially this will be used to replace the\r
+> existing infrastructure for storing the addresses of thread authors.\r
+> Further patches will use it to store the addresses of threads\r
+> recipients.\r
+> \r
+> Init and destructor functions are included, as well as a function to\r
+> add addresses to a struct, either "matched" or not.\r
+> ---\r
+> lib/notmuch.h | 1 +\r
+> lib/thread.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
+> 2 files changed, 117 insertions(+)\r
+> \r
+> diff --git a/lib/notmuch.h b/lib/notmuch.h\r
+> index 3633bed..6acd38d 100644\r
+> --- a/lib/notmuch.h\r
+> +++ b/lib/notmuch.h\r
+> @@ -118,6 +118,7 @@ typedef struct _notmuch_database notmuch_database_t;\r
+> typedef struct _notmuch_query notmuch_query_t;\r
+> typedef struct _notmuch_threads notmuch_threads_t;\r
+> typedef struct _notmuch_thread notmuch_thread_t;\r
+> +typedef struct _notmuch_thread_addresses notmuch_thread_addresses_t;\r
+> typedef struct _notmuch_messages notmuch_messages_t;\r
+> typedef struct _notmuch_message notmuch_message_t;\r
+> typedef struct _notmuch_tags notmuch_tags_t;\r
+> diff --git a/lib/thread.cc b/lib/thread.cc\r
+> index e976d64..7af9eeb 100644\r
+> --- a/lib/thread.cc\r
+> +++ b/lib/thread.cc\r
+> @@ -24,6 +24,14 @@\r
+> #include <gmime/gmime.h>\r
+> #include <glib.h> /* GHashTable */\r
+> \r
+> +struct visible _notmuch_thread_addresses {\r
+> + GHashTable *matched_hash;\r
+> + GPtrArray *matched_array;\r
+> + GHashTable *unmatched_hash;\r
+> + GPtrArray *unmatched_array;\r
+> + char *string;\r
+> +};\r
+> +\r
+> struct visible _notmuch_thread {\r
+> notmuch_database_t *notmuch;\r
+> char *thread_id;\r
+> @@ -44,6 +52,18 @@ struct visible _notmuch_thread {\r
+> };\r
+> \r
+> static int\r
+> +_notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses)\r
+> +{\r
+> + g_hash_table_unref (addresses->matched_hash);\r
+> + g_hash_table_unref (addresses->unmatched_hash);\r
+> + g_ptr_array_free (addresses->matched_array, TRUE);\r
+> + g_ptr_array_free (addresses->unmatched_array, TRUE);\r
+\r
+The second argument should be FALSE for both of these, since the\r
+pointers contained in these pointer arrays are talloc-managed.\r
+\r
+> + addresses->matched_array = NULL;\r
+> + addresses->unmatched_array = NULL;\r
+\r
+Is this necessary? (Obviously there's no harm.)\r
+\r
+> + return 0;\r
+> +}\r
+> +\r
+> +static int\r
+> _notmuch_thread_destructor (notmuch_thread_t *thread)\r
+> {\r
+> g_hash_table_unref (thread->authors_hash);\r
+> @@ -64,6 +84,81 @@ _notmuch_thread_destructor (notmuch_thread_t *thread)\r
+> return 0;\r
+> }\r
+> \r
+> +/* Add address to a thread addresses struct. If matched is TRUE, then\r
+> + * the address will be added to the matched list.*/\r
+> +static void\r
+> +_thread_add_address (notmuch_thread_addresses_t *addresses,\r
+\r
+_thread_addresses_add?\r
+\r
+> + const char *address,\r
+> + notmuch_bool_t matched)\r
+> +{\r
+> + char *address_copy;\r
+> + GHashTable *hash;\r
+> + GPtrArray *array;\r
+> +\r
+> + if (matched) {\r
+> + hash = addresses->matched_hash;\r
+> + array = addresses->matched_array;\r
+> + } else {\r
+> + hash = addresses->unmatched_hash;\r
+> + array = addresses->unmatched_array;\r
+> + }\r
+> +\r
+> + if (address == NULL)\r
+> + return;\r
+> +\r
+> + if (g_hash_table_lookup_extended (hash, address, NULL, NULL))\r
+> + return;\r
+> +\r
+> + address_copy = talloc_strdup (addresses, address);\r
+> +\r
+> + g_hash_table_insert (hash, address_copy, NULL);\r
+> +\r
+> + g_ptr_array_add (array, address_copy);\r
+> +}\r
+> +\r
+> +/* Construct an addresses string from matched and unmatched addresses\r
+> + * in notmuch_thread_addresses_t. The string contains matched\r
+> + * addresses first, then non-matched addresses (with the two groups\r
+> + * separated by '|'). Within each group, addresses are listed in date\r
+> + * order. */\r
+> +static void\r
+> +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses)\r
+\r
+A better API for this would be to return a const char*. On the first\r
+call (when addresses->string is NULL), construct the string and return\r
+it. On subsequent calls, just immediately return addresses->string.\r
+The approach you're using here makes sense from an incremental\r
+perspective, but since you're introducing an abstraction for thread\r
+address lists, I think it makes more sense to look at it from an API\r
+perspective.\r
+\r
+Also, the name should probably start with thread_addresses, since\r
+that's the abstraction it's part of.\r
+_thread_addresses_resolve_string? _thread_addresses_to_string?\r
+\r
+> +{\r
+> + unsigned int i;\r
+> + char *address;\r
+> + int first_non_matched_address = 1;\r
+> +\r
+> + /* First, list all matched addressses in date order. */\r
+\r
+Michal's comment about this not necessarily being date order applies\r
+here, too.\r
+\r
+> + for (i = 0; i < addresses->matched_array->len; i++) {\r
+> + address = (char *) g_ptr_array_index (addresses->matched_array, i);\r
+> + if (addresses->string)\r
+> + addresses->string = talloc_asprintf (addresses, "%s, %s",\r
+> + addresses->string,\r
+> + address);\r
+> + else\r
+> + addresses->string = address;\r
+> + }\r
+> +\r
+> + /* Next, append any non-matched addresses that haven't already appeared. */\r
+> + for (i = 0; i < addresses->unmatched_array->len; i++) {\r
+> + address = (char *) g_ptr_array_index (addresses->unmatched_array, i);\r
+> + if (g_hash_table_lookup_extended (addresses->matched_hash,\r
+> + address, NULL, NULL))\r
+> + continue;\r
+> + if (first_non_matched_address) {\r
+> + addresses->string = talloc_asprintf (addresses, "%s| %s",\r
+> + addresses->string,\r
+> + address);\r
+> + } else {\r
+> + addresses->string = talloc_asprintf (addresses, "%s, %s",\r
+> + addresses->string,\r
+> + address);\r
+> + }\r
+\r
+I second Michal's comments on this code; especially the one about\r
+using talloc_asprintf_append, even if you don't combine the two\r
+branches. Currently this code leaks O(n^2) memory (in a talloc\r
+context, so it's not permanent, but it's still unfortunate).\r
+\r
+> +\r
+> + first_non_matched_address = 0;\r
+> + }\r
+> +}\r
+> +\r
+> /* Add each author of the thread to the thread's authors_hash and to\r
+> * the thread's authors_array. */\r
+> static void\r
+> @@ -382,6 +477,27 @@ _resolve_thread_relationships (unused (notmuch_thread_t *thread))\r
+> */\r
+> }\r
+> \r
+> +/* Initialize a thread addresses struct. */\r
+> +notmuch_thread_addresses_t *\r
+> +_thread_addresses_init (const void *ctx)\r
+\r
+This should be "create" instead of "init". Also, this should probably\r
+be static, given that everything else is. If there's reason to make\r
+it non-static, then it should also start with _notmuch and have a\r
+prototype in notmuch-private.h.\r
+\r
+> +{\r
+> + notmuch_thread_addresses_t *addresses;\r
+> +\r
+> + addresses = talloc (ctx, notmuch_thread_addresses_t);\r
+> + if (unlikely (addresses == NULL))\r
+> + return NULL;\r
+\r
+You should set _notmuch_thread_addresses_destructor as the talloc\r
+destructor here, rather than calling it by hand from\r
+_notmuch_thread_destructor (in patch 2).\r
+\r
+> +\r
+> + addresses->matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal,\r
+> + NULL, NULL);\r
+> + addresses->matched_array = g_ptr_array_new ();\r
+> + addresses->unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal,\r
+> + NULL, NULL);\r
+> + addresses->unmatched_array = g_ptr_array_new ();\r
+> + addresses->string = NULL;\r
+> +\r
+> + return addresses;\r
+> +}\r
+> +\r
+> /* Create a new notmuch_thread_t object by finding the thread\r
+> * containing the message with the given doc ID, treating any messages\r
+> * contained in match_set as "matched". Remove all messages in the\r
projects / notmuch-archives.git / commitdiff