Re: [PATCH 0/5] Fetch all message metadata in a single pass

diff --git a/6c/b011fd7ac6c04c4b2dcfa8de93248249a00cf8 b/6c/b011fd7ac6c04c4b2dcfa8de93248249a00cf8
new file mode 100644 (file)
index 0000000..3df9207
--- /dev/null
+++ b/6c/b011fd7ac6c04c4b2dcfa8de93248249a00cf8
@@ -0,0 +1,217 @@
+Return-Path: <cworth@cworth.org>\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 3FE9C429E20\r
+       for <notmuch@notmuchmail.org>; Thu, 10 Mar 2011 19:48:57 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.99\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.99 tagged_above=-999 required=5\r
+       tests=[ALL_TRUSTED=-1, T_MIME_NO_TEXT=0.01] 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 6qO5R2pstLKb; Thu, 10 Mar 2011 19:48:55 -0800 (PST)\r
+Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 5CF39431FB5;\r
+       Thu, 10 Mar 2011 19:48:55 -0800 (PST)\r
+Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
+       id E505E54C0C4; Thu, 10 Mar 2011 19:48:54 -0800 (PST)\r
+From: Carl Worth <cworth@cworth.org>\r
+To: Austin Clements <amdragon@mit.edu>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH 0/5] Fetch all message metadata in a single pass\r
+In-Reply-To: <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>\r
+References: <1291928396-27937-1-git-send-email-amdragon@mit.edu>\r
+       <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>\r
+User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.2.1\r
+       (i486-pc-linux-gnu)\r
+Date: Thu, 10 Mar 2011 19:48:54 -0800\r
+Message-ID: <87hbba8ggp.fsf@yoom.home.cworth.org>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/signed; boundary="=-=-=";\r
+       micalg=pgp-sha1; protocol="application/pgp-signature"\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: Fri, 11 Mar 2011 03:48:57 -0000\r
+\r
+--=-=-=\r
+\r
+On Sun, 13 Feb 2011 15:25:48 -0500, Austin Clements <amdragon@mit.edu> wrote:\r
+> I've rebased this against current master and fixed a few merge\r
+> conflicts.  The rebased patches are on the eager-metadata-v3 branch of\r
+>   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
+    version_string = 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
+> +    /* Get tags */\r
+> +    if (!message->tag_list) {\r
+> +    message->tag_list =\r
+> +        _notmuch_get_terms_with_prefix (message, i, end, tag_prefix);\r
+> +    _notmuch_string_list_sort (message->tag_list);\r
+> +    }\r
+\r
+Looks like the above case is missing the assert to ensure proper prefix\r
+ordering.\r
+\r
+> +    if (!message->tag_list)\r
+> +    _notmuch_message_ensure_metadata (message);\r
+> +\r
+> +    /* We tell the iterator to add a talloc reference to the\r
+> +     * underlying list, rather than stealing it.  As a result, it's\r
+> +     * possible to modify the message tags while iterating because\r
+> +     * the iterator will keep the current list alive. */\r
+> +    return _notmuch_tags_create (message, message->tag_list, FALSE);\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
+       static void\r
+       _foo_function_internal (foo_t *, ..., bool_t be_different)\r
+       {\r
+               ...;\r
+       }\r
+\r
+        void\r
+       foo_function (foo_t *, ...)\r
+       {\r
+               return _foo_function_internal (foo, ..., FALSE);\r
+       }\r
+\r
+        void\r
+       foo_function_different (foo_t *, ...)\r
+       {\r
+               return _foo_function_internal (foo, ..., TRUE);\r
+       }\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
+       tags = _notmuch_tags_create (message, message->tag_list);\r
+        talloc_reference (message, message->tag_list);\r
+        return 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
+       _notmuch_tags_create_with_steal (ctx, tag_list);\r
+\r
+What do you think?\r
+\r
+> -notmuch_tags_t *\r
+> -_notmuch_convert_tags (void *ctx, Xapian::TermIterator &i,\r
+> -                   Xapian::TermIterator &end);\r
+> +notmuch_string_list_t *\r
+> +_notmuch_get_terms_with_prefix (void *ctx, Xapian::TermIterator &i,\r
+> +                            Xapian::TermIterator &end,\r
+> +                            const char *prefix);\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
+> 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
+> + * You should have received a copy of the GNU General Public License\r
+> + * along with this program.  If not, see http://www.gnu.org/licenses/ .\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
+> -/* 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
+> +    /* Get thread */\r
+> +    if (!message->thread_id)\r
+> +    message->thread_id =\r
+> +        _notmuch_message_get_term (message, i, end, thread_prefix);\r
+> +\r
+> +    /* Get id */\r
+> +    assert (strcmp (thread_prefix, id_prefix) < 0);\r
+> +    if (!message->message_id)\r
+> +    message->message_id =\r
+> +        _notmuch_message_get_term (message, i, end, id_prefix);\r
+\r
+Missing asserts on the above two as well?\r
+\r
+-Carl\r
+\r
+--=-=-=\r
+Content-Type: application/pgp-signature\r
+\r
+-----BEGIN PGP SIGNATURE-----\r
+Version: GnuPG v1.4.10 (GNU/Linux)\r
+\r
+iD8DBQFNeZum6JDdNq8qSWgRAm0WAKCWSxswxte3awQvkHx121eP3euxHwCfa7As\r
+AeePsp1GmNL7pFoTzSdxptE=\r
+=u/mF\r
+-----END PGP SIGNATURE-----\r
+--=-=-=--\r