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); > > >