--- /dev/null
+Return-Path: <amdragon@gmail.com>\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 38EF1431FB6\r
+ for <notmuch@notmuchmail.org>; Sun, 20 Mar 2011 23:56:09 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.699\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.699 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, FREEMAIL_FROM=0.001,\r
+ RCVD_IN_DNSWL_LOW=-0.7] autolearn=unavailable\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 3o7rZmVIYAmr for <notmuch@notmuchmail.org>;\r
+ Sun, 20 Mar 2011 23:56:09 -0700 (PDT)\r
+Received: from mail-qy0-f174.google.com (mail-qy0-f174.google.com\r
+ [209.85.216.174]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id DB6BE431FB5\r
+ for <notmuch@notmuchmail.org>; Sun, 20 Mar 2011 23:56:08 -0700 (PDT)\r
+Received: by qyk7 with SMTP id 7so2228072qyk.5\r
+ for <notmuch@notmuchmail.org>; Sun, 20 Mar 2011 23:56:05 -0700 (PDT)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
+ h=domainkey-signature:mime-version:sender:in-reply-to:references:date\r
+ :x-google-sender-auth:message-id:subject:from:to:cc:content-type\r
+ :content-transfer-encoding;\r
+ bh=7RamGmwwsGCyK+eSUskw6piW/pYhxlgZHTXvR7c/0B0=;\r
+ b=kdtB0hlyuTRyiBZRuYnN9HaJt4j/4qbYAGfuLJW6SCdGUiQQsroEU7Bc+muKLx1Rbi\r
+ m4TTydHVihyGcBoqtjMKqrHWhnTRVJGjrM9NuBZil7tl5z16rZ3RTwUuY5RqzK0oARUJ\r
+ JhBi4laqSVSkD6c9IEjZfgZmDgu2P5rJCX7p8=\r
+DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;\r
+ h=mime-version:sender:in-reply-to:references:date\r
+ :x-google-sender-auth:message-id:subject:from:to:cc:content-type\r
+ :content-transfer-encoding;\r
+ b=xJCf3C6pAyEuO+3EWMAM3f+7pmvd9VpgGueGDVXc9nQ4eNmiztVJGhxMMwozfG6igJ\r
+ k27gtjLJnVqxQVOqE3TtJJfQ+lnNXtiXPEBiAtoMw66jjMbJPxY5cM4vsmNmyOx/NjLu\r
+ f2urq7ijqrUctbWNA7YMKoUAxOhUwWBiodUrY=\r
+MIME-Version: 1.0\r
+Received: by 10.229.102.209 with SMTP id h17mr2945734qco.102.1300690565502;\r
+ Sun, 20 Mar 2011 23:56:05 -0700 (PDT)\r
+Sender: amdragon@gmail.com\r
+Received: by 10.229.30.68 with HTTP; Sun, 20 Mar 2011 23:56:05 -0700 (PDT)\r
+In-Reply-To: <87hbba8ggp.fsf@yoom.home.cworth.org>\r
+References: <1291928396-27937-1-git-send-email-amdragon@mit.edu>\r
+ <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>\r
+ <87hbba8ggp.fsf@yoom.home.cworth.org>\r
+Date: Mon, 21 Mar 2011 02:56:05 -0400\r
+X-Google-Sender-Auth: GRcovGu3vALGYFkuVRjsY60hFyw\r
+Message-ID: <AANLkTi=uZXTacQFiUUk8k5h17NrV1wTo5OR7T=3UguE_@mail.gmail.com>\r
+Subject: Re: [PATCH 0/5] Fetch all message metadata in a single pass\r
+From: Austin Clements <amdragon@mit.edu>\r
+To: Carl Worth <cworth@cworth.org>\r
+Content-Type: text/plain; charset=ISO-8859-1\r
+Content-Transfer-Encoding: quoted-printable\r
+Cc: 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: Mon, 21 Mar 2011 06:56:09 -0000\r
+\r
+Thanks for the thorough review. My updated patches are on the\r
+eager-metadata-v4 branch (also, for-review/eager-metadata-v4) at\r
+ http://awakening.csail.mit.edu/git/notmuch.git\r
+Responses inline.\r
+\r
+On Thu, Mar 10, 2011 at 10:48 PM, Carl Worth <cworth@cworth.org> wrote:\r
+> On Sun, 13 Feb 2011 15:25:48 -0500, Austin Clements <amdragon@mit.edu> wr=\r
+ote:\r
+>> I've rebased this against current master and fixed a few merge\r
+>> conflicts. =A0The rebased patches are on the eager-metadata-v3 branch of\r
+>> =A0 http://awakening.csail.mit.edu/git/notmuch.git\r
+>\r
+> Hi Austin,\r
+>\r
+> This looks like a great set of optimizations and cleanup. Here is some\r
+> (long-overdue) review.\r
+>\r
+> First, the patch series looks excellent and my review here is quite\r
+> nit-picky. I feel bad reviewing this so late and not just immediately\r
+> merging it, but I will commit to picking up a refreshed series very\r
+> quickly.\r
+>\r
+> One very minor thing is that the word "metadata" might be confused with\r
+> Xapian metadata which we are using already such as:\r
+>\r
+> =A0 =A0version_string =3D notmuch->xapian_db->get_metadata ("version");\r
+>\r
+> So we might want to come up with a better name here. I don't have any\r
+> concrete suggestion yet, so if you don't think of anything obvious, then\r
+> don't worry about it.\r
+\r
+Hmm. I think this is okay because I always refer to "message\r
+metadata" (and I couldn't think of a better term).\r
+\r
+>> + =A0 =A0/* Get tags */\r
+>> + =A0 =A0if (!message->tag_list) {\r
+>> + =A0 =A0 message->tag_list =3D\r
+>> + =A0 =A0 =A0 =A0 _notmuch_get_terms_with_prefix (message, i, end, tag_p=\r
+refix);\r
+>> + =A0 =A0 _notmuch_string_list_sort (message->tag_list);\r
+>> + =A0 =A0}\r
+>\r
+> Looks like the above case is missing the assert to ensure proper prefix\r
+> ordering.\r
+\r
+Fixed.\r
+\r
+>> + =A0 =A0if (!message->tag_list)\r
+>> + =A0 =A0 _notmuch_message_ensure_metadata (message);\r
+>> +\r
+>> + =A0 =A0/* We tell the iterator to add a talloc reference to the\r
+>> + =A0 =A0 * underlying list, rather than stealing it. =A0As a result, it=\r
+'s\r
+>> + =A0 =A0 * possible to modify the message tags while iterating because\r
+>> + =A0 =A0 * the iterator will keep the current list alive. */\r
+>> + =A0 =A0return _notmuch_tags_create (message, message->tag_list, FALSE)=\r
+;\r
+>\r
+> The behavior here is great, but don't like Boolean function parameters\r
+> being used to change the semantics. The problem with a Boolean argument\r
+> is that it's impossible to know the semantic by just reading the calling\r
+> code---you must also consult the implementation as well.\r
+>\r
+> For any case where it seems natural to implement something with a\r
+> Boolean argument, I sometimes use an approach something like this:\r
+>\r
+> =A0 =A0 =A0 =A0static void\r
+> =A0 =A0 =A0 =A0_foo_function_internal (foo_t *, ..., bool_t be_different)\r
+> =A0 =A0 =A0 =A0{\r
+> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0...;\r
+> =A0 =A0 =A0 =A0}\r
+>\r
+> =A0 =A0 =A0 =A0void\r
+> =A0 =A0 =A0 =A0foo_function (foo_t *, ...)\r
+> =A0 =A0 =A0 =A0{\r
+> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return _foo_function_internal (foo, ..., F=\r
+ALSE);\r
+> =A0 =A0 =A0 =A0}\r
+>\r
+> =A0 =A0 =A0 =A0void\r
+> =A0 =A0 =A0 =A0foo_function_different (foo_t *, ...)\r
+> =A0 =A0 =A0 =A0{\r
+> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return _foo_function_internal (foo, ..., T=\r
+RUE);\r
+> =A0 =A0 =A0 =A0}\r
+>\r
+> That is, I'm willing to accept the Boolean parameter in the case of a\r
+> static function defined immediately next to all callers. Here, for any\r
+> non-static callers the semantics should be quite clear. (And perhaps the\r
+> function calling with the FALSE case will need a clarifying suffix as\r
+> well---one might have some_function_foo and some_function_bar for the\r
+> two cases).\r
+>\r
+> Of course, my proscription against Boolean parameter doesn't apply to\r
+> something like a function that is setting some Boolean feature to TRUE\r
+> or FALSE. The important criterion is that the function semantics should\r
+> be evident to someone reading code calling the function. So if the\r
+> Boolean argument is obviously tied to some portion of the function name,\r
+> then that can be adequate.\r
+>\r
+> Enough with the generalities, back to _notmuch_tags_create...\r
+>\r
+> The caller above is the exceptional caller. It's the only one using\r
+> passing FALSE, and it also already has a large comment. So it seems that\r
+> the right fix here is to have the extra talloc_reference happen here\r
+> where there's a comment talking about adding a talloc reference.\r
+>\r
+> So it would be great to see something here like:\r
+>\r
+> =A0 =A0 =A0 =A0tags =3D _notmuch_tags_create (message, message->tag_list)=\r
+;\r
+> =A0 =A0 =A0 =A0talloc_reference (message, message->tag_list);\r
+> =A0 =A0 =A0 =A0return tags;\r
+>\r
+> Of course, that would mean that _notmuch_tags_create can't have done the\r
+> talloc_steal. So perhaps all the other callers should be calling:\r
+>\r
+> =A0 =A0 =A0 =A0_notmuch_tags_create_with_steal (ctx, tag_list);\r
+>\r
+> What do you think?\r
+\r
+Your point about the boolean argument is well taken. In this case\r
+there's really no need for two difference versions, so I wound up\r
+making _notmuch_tags_create always steal the reference.\r
+\r
+I debated for a while whether it should add a reference, thus forcing\r
+most callers to talloc_unlink, or steal the reference, thus forcing\r
+one caller to talloc_reference. I ultimately decided it was much more\r
+likely that the caller would forget the talloc_unlink, resulting in a\r
+difficult-to-track memory leak (since the list would *eventually* be\r
+cleaned up), than that the steal would cause confusion. Also, there\r
+is some precedence for internal functions that steal an argument. So\r
+I made it steal the reference.\r
+\r
+This doesn't cause any problems in the one weird case that still needs\r
+the reference to the list. After the _notmuch_tags_create, the caller\r
+simply adds that reference.\r
+\r
+>> -notmuch_tags_t *\r
+>> -_notmuch_convert_tags (void *ctx, Xapian::TermIterator &i,\r
+>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Xapian::TermIterator &end);\r
+>> +notmuch_string_list_t *\r
+>> +_notmuch_get_terms_with_prefix (void *ctx, Xapian::TermIterator &i,\r
+>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 Xapian::TermIt=\r
+erator &end,\r
+>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const char *pr=\r
+efix);\r
+>\r
+> I don't really like the fact that _notmuch_get_terms_with_prefix doesn't\r
+> make a clear indication of what file it's defined in, (the old function\r
+> _notmuch_convert_tags had the same problem). It could be named\r
+> _notmuch_database_convert_tags since it's in database.cc, but that would\r
+> be odd in not actually accepting a notmuch_database_t * as the first\r
+> parameter. Any suggestion here?\r
+\r
+_notmuch_get_terms_with_prefix is weird because it captures a pattern\r
+that lies squarely in the Xapian world, being all about term\r
+iterators. Hence, I think the "right" solution (note, not the best\r
+solution), would be to hide the term iterators and make two copies of\r
+the function: one that takes a notmuch_database_t and always considers\r
+all database terms, and one private to message.cc that acts as a\r
+helper to what's now all bundled in _notmuch_message_ensure_metadata.\r
+\r
+But that bothers me more than a function that doesn't take a\r
+notmuch_database_t *. So I just added "database" to the name.\r
+\r
+>> index 0000000..d92a0ba\r
+>> --- /dev/null\r
+>> +++ b/lib/strings.c\r
+>> @@ -0,0 +1,94 @@\r
+>> +/* strings.c - Iterator for a list of strings\r
+>\r
+> Similarly, this file might be better as string-list.c.\r
+\r
+Done.\r
+\r
+>> + * You should have received a copy of the GNU General Public License\r
+>> + * along with this program. =A0If not, see http://www.gnu.org/licenses/=\r
+ .\r
+>> + *\r
+>> + * Author: Carl Worth <cworth@cworth.org>\r
+>\r
+> Hey, wait a second, some of this code is mine, but I think the sort\r
+> function is yours. Please do start annotating the Author tags on all\r
+> files as appropriate. (There are probably lots of files missing your\r
+> Author attribution---I just happened to notice this one here since you\r
+> happened to have an Author line in the patch.)\r
+\r
+Heh, fixed. I suppose I hadn't been thinking about it, since none of\r
+the source in lib/ has other authors listed.\r
+\r
+But it raises an interesting question. When is it kosher to add\r
+oneself to a file's author list? In this case I wrote about half of\r
+the code in that admittedly short file, so that makes sense Changing\r
+a few lines presumably isn't enough. Adding a few functions?\r
+\r
+>> -/* XXX: Should write some talloc-friendly list to avoid the need for\r
+>> - * this. */\r
+>\r
+> Hurrah! I love patches that get to remove XXX comments.\r
+\r
+]:--8)\r
+\r
+>> + =A0 =A0/* Get thread */\r
+>> + =A0 =A0if (!message->thread_id)\r
+>> + =A0 =A0 message->thread_id =3D\r
+>> + =A0 =A0 =A0 =A0 _notmuch_message_get_term (message, i, end, thread_pre=\r
+fix);\r
+>> +\r
+>> + =A0 =A0/* Get id */\r
+>> + =A0 =A0assert (strcmp (thread_prefix, id_prefix) < 0);\r
+>> + =A0 =A0if (!message->message_id)\r
+>> + =A0 =A0 message->message_id =3D\r
+>> + =A0 =A0 =A0 =A0 _notmuch_message_get_term (message, i, end, id_prefix)=\r
+;\r
+>\r
+> Missing asserts on the above two as well?\r
+\r
+Fixed. (Actually, I think there was only one assert missing in total,\r
+but it had propagated through the history.)\r
+\r
+> -Carl\r
projects / notmuch-archives.git / commitdiff