From: Jameson Graef Rollins Date: Thu, 30 Aug 2012 16:33:56 +0000 (+1700) Subject: Re: [PATCH 01/11] lib: new thread addresses structure X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=99e88e2326a701ebc7cf0b058768f86579ec0922;p=notmuch-archives.git Re: [PATCH 01/11] lib: new thread addresses structure --- diff --git a/c2/a6c136302f4213829ddea35dfba1e607d48698 b/c2/a6c136302f4213829ddea35dfba1e607d48698 new file mode 100644 index 000000000..4febe0455 --- /dev/null +++ b/c2/a6c136302f4213829ddea35dfba1e607d48698 @@ -0,0 +1,151 @@ +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 D7309431FBC + for ; Thu, 30 Aug 2012 09:34:11 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -2.29 +X-Spam-Level: +X-Spam-Status: No, score=-2.29 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_MED=-2.3, T_MIME_NO_TEXT=0.01] 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 hiYKKLFe7Q+q for ; + Thu, 30 Aug 2012 09:34:10 -0700 (PDT) +Received: from outgoing-mail.its.caltech.edu (outgoing-mail.its.caltech.edu + [131.215.239.19]) + by olra.theworths.org (Postfix) with ESMTP id B2D67431FAE + for ; Thu, 30 Aug 2012 09:34:10 -0700 (PDT) +Received: from fire-doxen.imss.caltech.edu (localhost [127.0.0.1]) + by fire-doxen-postvirus (Postfix) with ESMTP id DB3FA328060; + Thu, 30 Aug 2012 09:34:01 -0700 (PDT) +X-Spam-Scanned: at Caltech-IMSS on fire-doxen by amavisd-new +Received: from finestructure.net (m9.ligo.caltech.edu [131.215.115.109]) + (Authenticated sender: jrollins) + by fire-doxen-submit (Postfix) with ESMTP id 04B432E50B61; + Thu, 30 Aug 2012 09:33:58 -0700 (PDT) +Received: by finestructure.net (Postfix, from userid 1000) + id CFE6E3A9; Thu, 30 Aug 2012 09:33:58 -0700 (PDT) +From: Jameson Graef Rollins +To: Michal Sojka , Notmuch Mail +Subject: Re: [PATCH 01/11] lib: new thread addresses structure +In-Reply-To: <87y5kw4cfo.fsf@steelpick.2x.cz> +References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net> + <1345427570-26518-2-git-send-email-jrollins@finestructure.net> + <87y5kw4cfo.fsf@steelpick.2x.cz> +User-Agent: Notmuch/0.14+6~gb305ce9 (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Thu, 30 Aug 2012 09:33:56 -0700 +Message-ID: <874nnkpcej.fsf@servo.finestructure.net> +MIME-Version: 1.0 +Content-Type: multipart/signed; boundary="=-=-="; + micalg=pgp-sha256; protocol="application/pgp-signature" +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: Thu, 30 Aug 2012 16:34:12 -0000 + +--=-=-= + +Hey, Michal. Thanks for the review. + +On Thu, Aug 30 2012, Michal Sojka wrote: +>> +/* 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. */ +> +> I'd say the the addresses are listed in the order in which they have +> been added, which might or might not be the date order. + +I actually did not write the _resolve_thread_addresses_string function. +I left the logic and behavior unchanged, and just modified to use the +new addresses structure. If we think the logic should be changed, maybe +we should do that in a separate patch. + +>> +static void +>> +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) +>> +{ +>> + unsigned int i; +>> + char *address; +>> + int first_non_matched_address = 1; +>> + +>> + /* First, list all matched addressses in date order. */ +>> + 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); +>> + } +> +> Simpler would be: +> +> addresses->string = talloc_asprintf (addresses, "%s%c %s", +> addresses->string, +> first_non_matched_address ? '|' : ',' +> address); +> +> Also, you might want to talloc_free the old address->string to not waste +> memory in the case of long lived notmuch_thread_addresses_t object. Or +> better use talloc_asprintf_append() function, which hopefully implements +> freeing internally. + +This looks like a nice simplification, but see comment above. + +I'll look into including these changes in the next version of the +series. + +jamie. + +--=-=-= +Content-Type: application/pgp-signature + +-----BEGIN PGP SIGNATURE----- +Version: GnuPG v1.4.12 (GNU/Linux) + +iQIcBAEBCAAGBQJQP5X0AAoJEO00zqvie6q8+YQQAKrKycrpRrNJdqWg7d/0SwSP +BU0GZsEPB2p7i8uKpwQ6CdnOKXHxme9mnMlD6MpTR5SZqqoOZaEkWRK/E5e8FBh4 +8Zh1OMgpwrPIy/hQAwMJ7/z718P4h34wjA8utB9WtHUJH6f+Hm2IDfV7MeHk/4mz +FJmv87JH4cmj2Q8Clgp8HTbLeg15wyatc9eV+AT2AeRVcSo56AHBXTj34Q6Uj/3M +Usq/7jLn7kukEr+pMcW+sXJWSEdV9asbAY7a7O63cuFg9mHJamkZAtajvJ5cBIw6 +y8D/SxWzv6XCg20oSgnhxfXxoE6WWOgdeDMQLdoT9KP/ywcvU4X9H7/bOkxnsp5G ++n+BKgDtbBE+CFVgYZ2MMZVWVkMh57xilvfsQXiRpj8UQvrgDxNnpKMxbD7MJln4 +72xLN1JbkdBjX0iGxdKfLs8o4DqLgbVFC4x6WJQCqmViY3i26oK8MVt6kwsTQFuY +heoqqj5SsCSQfCdnmS6UiVnzhIw4kJK5+U7upHt/gL+JzDQ3kRcXQ740upUK2PCA +hKtmquLgLxx6Tk2ksgYuT5TPBe92AcdBPirv/wSmM0Py92S02IWl26Rtjqm5GEVw +iqSpLb1gvj2Xzj2QxEFMA6TX5voUUw/9tzrBb+IeatRxd6jMh7i2FF1oQOEQAsgr +6swbWsOcduFoJRmv6nm0 +=k1/c +-----END PGP SIGNATURE----- +--=-=-=--