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