From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 3FE9C429E20 for ; Thu, 10 Mar 2011 19:48:57 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.99 X-Spam-Level: X-Spam-Status: No, score=-0.99 tagged_above=-999 required=5 tests=[ALL_TRUSTED=-1, 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 6qO5R2pstLKb; Thu, 10 Mar 2011 19:48:55 -0800 (PST) Received: from yoom.home.cworth.org (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 5CF39431FB5; Thu, 10 Mar 2011 19:48:55 -0800 (PST) Received: by yoom.home.cworth.org (Postfix, from userid 1000) id E505E54C0C4; Thu, 10 Mar 2011 19:48:54 -0800 (PST) From: Carl Worth To: Austin Clements , notmuch@notmuchmail.org Subject: Re: [PATCH 0/5] Fetch all message metadata in a single pass In-Reply-To: References: <1291928396-27937-1-git-send-email-amdragon@mit.edu> User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu) Date: Thu, 10 Mar 2011 19:48:54 -0800 Message-ID: <87hbba8ggp.fsf@yoom.home.cworth.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; 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: Fri, 11 Mar 2011 03:48:57 -0000 --=-=-= On Sun, 13 Feb 2011 15:25:48 -0500, Austin Clements wrote: > I've rebased this against current master and fixed a few merge > conflicts. The rebased patches are on the eager-metadata-v3 branch of > http://awakening.csail.mit.edu/git/notmuch.git Hi Austin, This looks like a great set of optimizations and cleanup. Here is some (long-overdue) review. First, the patch series looks excellent and my review here is quite nit-picky. I feel bad reviewing this so late and not just immediately merging it, but I will commit to picking up a refreshed series very quickly. One very minor thing is that the word "metadata" might be confused with Xapian metadata which we are using already such as: version_string = notmuch->xapian_db->get_metadata ("version"); So we might want to come up with a better name here. I don't have any concrete suggestion yet, so if you don't think of anything obvious, then don't worry about it. > + /* Get tags */ > + if (!message->tag_list) { > + message->tag_list = > + _notmuch_get_terms_with_prefix (message, i, end, tag_prefix); > + _notmuch_string_list_sort (message->tag_list); > + } Looks like the above case is missing the assert to ensure proper prefix ordering. > + if (!message->tag_list) > + _notmuch_message_ensure_metadata (message); > + > + /* We tell the iterator to add a talloc reference to the > + * underlying list, rather than stealing it. As a result, it's > + * possible to modify the message tags while iterating because > + * the iterator will keep the current list alive. */ > + return _notmuch_tags_create (message, message->tag_list, FALSE); The behavior here is great, but don't like Boolean function parameters being used to change the semantics. The problem with a Boolean argument is that it's impossible to know the semantic by just reading the calling code---you must also consult the implementation as well. For any case where it seems natural to implement something with a Boolean argument, I sometimes use an approach something like this: static void _foo_function_internal (foo_t *, ..., bool_t be_different) { ...; } void foo_function (foo_t *, ...) { return _foo_function_internal (foo, ..., FALSE); } void foo_function_different (foo_t *, ...) { return _foo_function_internal (foo, ..., TRUE); } That is, I'm willing to accept the Boolean parameter in the case of a static function defined immediately next to all callers. Here, for any non-static callers the semantics should be quite clear. (And perhaps the function calling with the FALSE case will need a clarifying suffix as well---one might have some_function_foo and some_function_bar for the two cases). Of course, my proscription against Boolean parameter doesn't apply to something like a function that is setting some Boolean feature to TRUE or FALSE. The important criterion is that the function semantics should be evident to someone reading code calling the function. So if the Boolean argument is obviously tied to some portion of the function name, then that can be adequate. Enough with the generalities, back to _notmuch_tags_create... The caller above is the exceptional caller. It's the only one using passing FALSE, and it also already has a large comment. So it seems that the right fix here is to have the extra talloc_reference happen here where there's a comment talking about adding a talloc reference. So it would be great to see something here like: tags = _notmuch_tags_create (message, message->tag_list); talloc_reference (message, message->tag_list); return tags; Of course, that would mean that _notmuch_tags_create can't have done the talloc_steal. So perhaps all the other callers should be calling: _notmuch_tags_create_with_steal (ctx, tag_list); What do you think? > -notmuch_tags_t * > -_notmuch_convert_tags (void *ctx, Xapian::TermIterator &i, > - Xapian::TermIterator &end); > +notmuch_string_list_t * > +_notmuch_get_terms_with_prefix (void *ctx, Xapian::TermIterator &i, > + Xapian::TermIterator &end, > + const char *prefix); I don't really like the fact that _notmuch_get_terms_with_prefix doesn't make a clear indication of what file it's defined in, (the old function _notmuch_convert_tags had the same problem). It could be named _notmuch_database_convert_tags since it's in database.cc, but that would be odd in not actually accepting a notmuch_database_t * as the first parameter. Any suggestion here? > index 0000000..d92a0ba > --- /dev/null > +++ b/lib/strings.c > @@ -0,0 +1,94 @@ > +/* strings.c - Iterator for a list of strings Similarly, this file might be better as string-list.c. > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see http://www.gnu.org/licenses/ . > + * > + * Author: Carl Worth Hey, wait a second, some of this code is mine, but I think the sort function is yours. Please do start annotating the Author tags on all files as appropriate. (There are probably lots of files missing your Author attribution---I just happened to notice this one here since you happened to have an Author line in the patch.) > -/* XXX: Should write some talloc-friendly list to avoid the need for > - * this. */ Hurrah! I love patches that get to remove XXX comments. > + /* Get thread */ > + if (!message->thread_id) > + message->thread_id = > + _notmuch_message_get_term (message, i, end, thread_prefix); > + > + /* Get id */ > + assert (strcmp (thread_prefix, id_prefix) < 0); > + if (!message->message_id) > + message->message_id = > + _notmuch_message_get_term (message, i, end, id_prefix); Missing asserts on the above two as well? -Carl --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) iD8DBQFNeZum6JDdNq8qSWgRAm0WAKCWSxswxte3awQvkHx121eP3euxHwCfa7As AeePsp1GmNL7pFoTzSdxptE= =u/mF -----END PGP SIGNATURE----- --=-=-=--