Date: Wed, 11 Jan 2012 13:48:38 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 2/3] lib: Add support for automatically excluding tags
	from queries
Message-ID: <20120111184838.GT20796@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>
	<87ty42v99m.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87ty42v99m.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Jan 11 at 10:11 am:
> 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.

Done.

> > +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++.]

Actually, that's a good point.  I had originally done this to prevent
std::string from performing any internal allocation or copying (which
calling c_str could do), but since it took me some digging to figure
out if I had *actually* avoided this (turns out there's an overloaded
string==char*, so I had), I've switched to using string.compare, which
I think makes the behavior and performance more obvious.

> > +		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. ;)

Since my code isn't doing any memory allocation, it can't leak memory.
It could do a lot of copying, but it turns out that isn't a problem
either because Xapian objects are internally reference-counted
handles.  So, in other words, if you write the obvious thing, it will
do the right thing, which is totally contrary to what C++ has
conditioned us to expect.  ]:--8)

> 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().

It handles this correctly for two reasons.  Suppose tag x is excluded
twice.  At worst, if tag:x doesn't appear in the query, the returned
query will get two AND NOT tag:x's, but that doesn't affect the
results.  It turns out it doesn't even get doubled up for a slightly
subtle reason: when the loop reaches the second x in the exclude list,
it will iterate over the new query, which already has the AND NOT
tag:x in it, so it will see the tag:x as if it were in the original
query and not further modify the query.

However, this did point out a bug.  I was using the end iterator from
the original query even when I started iterating over the modified
query.

I'll send out an updated series after someone looks at the third
patch.

> 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);
> >  
>