From: Carl Worth <cworth@cworth.org>
To: Austin Clements <amdragon@mit.edu>, notmuch@notmuchmail.org
Subject: Re: [PATCH 0/5] Fetch all message metadata in a single pass
In-Reply-To: <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>
References: <1291928396-27937-1-git-send-email-amdragon@mit.edu>
	<AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>
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"
Precedence: list

--=-=-=

On Sun, 13 Feb 2011 15:25:48 -0500, Austin Clements <amdragon@mit.edu> 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 <cworth@cworth.org>

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-----
--=-=-=--