From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/3] lib: Add support for automatically excluding tags
	from queries
In-Reply-To: <1326258173-21163-3-git-send-email-amdragon@mit.edu>
References: <20120109043101.GH20796@mit.edu>
	<1326258173-21163-1-git-send-email-amdragon@mit.edu>
	<1326258173-21163-3-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.10.2+187~g43d4f26 (http://notmuchmail.org) Emacs/23.1.1
	(i686-pc-linux-gnu)
Date: Wed, 11 Jan 2012 10:11:33 +0000
Message-ID: <87ty42v99m.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Wed, 11 Jan 2012 00:02:52 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This is useful for tags like "deleted" and "spam" that people
> generally want to exclude from query results.  These exclusions will
> be overridden if a tag is explicitly mentioned in a query.
> ---
>  lib/notmuch.h |    6 ++++++
>  lib/query.cc  |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 9f23a10..0a3ae2b 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -457,6 +457,12 @@ notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort);
>  notmuch_sort_t
>  notmuch_query_get_sort (notmuch_query_t *query);
>  
> +/* Add a tag that will be excluded by default from the query results.
> + * This exclusion will be overridden if this tag appears explicitly in
> + * the query. */
> +void
> +notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag);
> +
>  /* Execute a query for threads, returning a notmuch_threads_t object
>   * which can be used to iterate over the results. The returned threads
>   * object is owned by the query and as such, will only be valid until
> diff --git a/lib/query.cc b/lib/query.cc
> index b6c0f12..716db1c 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -27,6 +27,7 @@ struct _notmuch_query {
>      notmuch_database_t *notmuch;
>      const char *query_string;
>      notmuch_sort_t sort;
> +    notmuch_string_list_t *exclude_terms;
>  };
>  
>  typedef struct _notmuch_mset_messages {
> @@ -76,6 +77,8 @@ notmuch_query_create (notmuch_database_t *notmuch,
>  
>      query->sort = NOTMUCH_SORT_NEWEST_FIRST;
>  
> +    query->exclude_terms = _notmuch_string_list_create (query);
> +
>      return query;
>  }
>  
> @@ -97,6 +100,13 @@ notmuch_query_get_sort (notmuch_query_t *query)
>      return query->sort;
>  }
>  
> +void
> +notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag)
> +{
> +    char *term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);
> +    _notmuch_string_list_append (query->exclude_terms, term);
> +}
> +
>  /* We end up having to call the destructors explicitly because we had
>   * to use "placement new" in order to initialize C++ objects within a
>   * block that we allocated with talloc. So C++ is making talloc
> @@ -112,6 +122,25 @@ _notmuch_messages_destructor (notmuch_mset_messages_t *messages)
>      return 0;
>  }
>  

I'd like to have a comment here, or inline in the code, explaining the
following function a little bit.

> +static Xapian::Query
> +_notmuch_exclude_tags (notmuch_query_t *query, Xapian::Query xquery)
> +{
> +    Xapian::TermIterator end = xquery.get_terms_end ();
> +
> +    for (notmuch_string_node_t *term = query->exclude_terms->head; term;
> +	 term = term->next) {
> +	Xapian::TermIterator it = xquery.get_terms_begin ();
> +	for (; it != end; it++) {
> +	    if (*it == term->string)

[This is a double reminder to me why I'm not that enthusiastic about
operator overloading in C++.]

> +		break;
> +	}
> +	if (it == end)
> +	    xquery = Xapian::Query (Xapian::Query::OP_AND_NOT,
> +				    xquery, Xapian::Query (term->string));

I briefly dug into Xapian documentation and source code, and became none
the wiser whether this copies the queries passed to it or not, i.e. does
this leak memory or not. I just presume you know what you're doing. ;)

I think the function fails if someone is stupid enough to exclude the
same tag twice. I'm not sure if you should care. If you think so, you
could just check double add in notmuch_query_add_tag_exclude().

Otherwise, looks good.

> +    }
> +    return xquery;
> +}
> +
>  notmuch_messages_t *
>  notmuch_query_search_messages (notmuch_query_t *query)
>  {
> @@ -157,6 +186,8 @@ notmuch_query_search_messages (notmuch_query_t *query)
>  					 mail_query, string_query);
>  	}
>  
> +	final_query = _notmuch_exclude_tags (query, final_query);
> +
>  	enquire.set_weighting_scheme (Xapian::BoolWeight());
>  
>  	switch (query->sort) {
> @@ -436,6 +467,8 @@ notmuch_query_count_messages (notmuch_query_t *query)
>  					 mail_query, string_query);
>  	}
>  
> +	final_query = _notmuch_exclude_tags (query, final_query);
> +
>  	enquire.set_weighting_scheme(Xapian::BoolWeight());
>  	enquire.set_docid_order(Xapian::Enquire::ASCENDING);
>  
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch