Re: [PATCH v4 13/16] add indexopts to notmuch python bindings.

[notmuch-archives.git] / 55 / c262d201289d6eb84291e3961543ce474759e7

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