Fix count/search query destructiveness

Fix count/search query destructiveness

[David Bremner]

As discussed in id:147263183913.27784.12274024193186585889@mbp, the
current behaviour is destructive due to exclude handling. This makes
some natural code, including notmuch-reply, buggy.

I haven't updated the library prototypes for const correctness, but
that would probably make sense. Before I go through that, I wanted to
get some feedback on the breaking change to
notmuch_query_add_tag_exclude.  Another option would be to add a (take
a breath) notmuch_query_add_tag_exclude_st and wrap that in
notmuch_query_add_tag_exclude. I decided not to go that route in this
initial series since effectively code calling the function as void
impliment this same status ignoring. It depends how terrible we think
bumping the SONAME is. [1]

[1]: If we're going to break the ABI, I'd like to do it soon to give
     Debian a chance to do a transition before the next stable release.q

[PATCH 1/5] lib: eagerly parse queries

[David Bremner]

Rather than waiting for a call to count/search, parse the query string when the
notmuch_query_t is created. This is a small reduction in duplicated
code, and a potential efficiency improvement if many count/search
operations are called on the same query (although the latter sounds a
bit unusual). The main goal is to prepare the way for
non-destructive (or at least less destructive) exclude tag handling.

It does introduce a not-very-nice error path where running out of memory
is not easily distinguishable from a query syntax error.
---
 lib/query.cc | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index 53efd4e..098ed8f 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -29,6 +29,7 @@ struct _notmuch_query {
     notmuch_sort_t sort;
     notmuch_string_list_t *exclude_terms;
     notmuch_exclude_t omit_excluded;
+    Xapian::Query xapian_query;
 };
 
 typedef struct _notmuch_mset_messages {
@@ -71,6 +72,13 @@ _debug_query (void)
     return (env && strcmp (env, "") != 0);
 }
 
+/* Explicit destructor call for placement new */
+static int
+_notmuch_query_destructor (notmuch_query_t *query) {
+    query->xapian_query.~Query();
+    return 0;
+}
+
 notmuch_query_t *
 notmuch_query_create (notmuch_database_t *notmuch,
 		      const char *query_string)
@@ -84,6 +92,10 @@ notmuch_query_create (notmuch_database_t *notmuch,
     if (unlikely (query == NULL))
 	return NULL;
 
+    new (&query->xapian_query) Xapian::Query ();
+
+    talloc_set_destructor (query, _notmuch_query_destructor);
+
     query->notmuch = notmuch;
 
     query->query_string = talloc_strdup (query, query_string);
@@ -94,6 +106,22 @@ notmuch_query_create (notmuch_database_t *notmuch,
 
     query->omit_excluded = NOTMUCH_EXCLUDE_TRUE;
 
+    try {
+	new (&query->xapian_query) Xapian::Query ();
+	query->xapian_query =
+	    notmuch->query_parser->parse_query (query_string, NOTMUCH_QUERY_PARSER_FLAGS);
+    } catch (const Xapian::Error &error) {
+	_notmuch_database_log (notmuch,
+			       "A Xapian exception occured parsing query: %s\n",
+			       error.get_msg().c_str());
+	_notmuch_database_log_append (notmuch,
+			       "Query string was: %s\n",
+			       query->query_string);
+
+	talloc_free (query);
+	query = NULL;
+    }
+
     return query;
 }
 
@@ -217,7 +245,7 @@ _notmuch_query_search_documents (notmuch_query_t *query,
 	Xapian::Query mail_query (talloc_asprintf (query, "%s%s",
 						   _find_prefix ("type"),
 						   type));
-	Xapian::Query string_query, final_query, exclude_query;
+	Xapian::Query final_query, exclude_query;
 	Xapian::MSet mset;
 	Xapian::MSetIterator iterator;
 
@@ -226,10 +254,8 @@ _notmuch_query_search_documents (notmuch_query_t *query,
 	{
 	    final_query = mail_query;
 	} else {
-	    string_query = notmuch->query_parser->
-		parse_query (query_string, NOTMUCH_QUERY_PARSER_FLAGS);
 	    final_query = Xapian::Query (Xapian::Query::OP_AND,
-					 mail_query, string_query);
+					 mail_query, query->xapian_query);
 	}
 	messages->base.excluded_doc_ids = NULL;
 
@@ -572,7 +598,7 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
 	Xapian::Query mail_query (talloc_asprintf (query, "%s%s",
 						   _find_prefix ("type"),
 						   type));
-	Xapian::Query string_query, final_query, exclude_query;
+	Xapian::Query final_query, exclude_query;
 	Xapian::MSet mset;
 
 	if (strcmp (query_string, "") == 0 ||
@@ -580,10 +606,8 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
 	{
 	    final_query = mail_query;
 	} else {
-	    string_query = notmuch->query_parser->
-		parse_query (query_string, NOTMUCH_QUERY_PARSER_FLAGS);
 	    final_query = Xapian::Query (Xapian::Query::OP_AND,
-					 mail_query, string_query);
+					 mail_query, query->xapian_query);
 	}
 
 	exclude_query = _notmuch_exclude_tags (query, final_query);
-- 
2.9.3

[PATCH 2/5] lib/query: make query parsing lazy again, keep centralized.

[David Bremner]

This is mainly to fix the nasty error path introduced in the last
commit, by moving the parsing into functions where the API is already
set up to return error status.  It preserves the feature of having a
pre-parsed query available for further processing.
---
 lib/query.cc | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index 098ed8f..d7cf28f 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -29,6 +29,7 @@ struct _notmuch_query {
     notmuch_sort_t sort;
     notmuch_string_list_t *exclude_terms;
     notmuch_exclude_t omit_excluded;
+    notmuch_bool_t parsed;
     Xapian::Query xapian_query;
 };
 
@@ -93,6 +94,7 @@ notmuch_query_create (notmuch_database_t *notmuch,
 	return NULL;
 
     new (&query->xapian_query) Xapian::Query ();
+    query->parsed = FALSE;
 
     talloc_set_destructor (query, _notmuch_query_destructor);
 
@@ -106,23 +108,30 @@ notmuch_query_create (notmuch_database_t *notmuch,
 
     query->omit_excluded = NOTMUCH_EXCLUDE_TRUE;
 
+    return query;
+}
+
+static notmuch_status_t
+_notmuch_query_ensure_parsed (notmuch_query_t *query)
+{
+    if (query->parsed)
+	return NOTMUCH_STATUS_SUCCESS;
+
     try {
-	new (&query->xapian_query) Xapian::Query ();
 	query->xapian_query =
-	    notmuch->query_parser->parse_query (query_string, NOTMUCH_QUERY_PARSER_FLAGS);
+	    query->notmuch->query_parser->
+		parse_query (query->query_string, NOTMUCH_QUERY_PARSER_FLAGS);
     } catch (const Xapian::Error &error) {
-	_notmuch_database_log (notmuch,
+	_notmuch_database_log (query->notmuch,
 			       "A Xapian exception occured parsing query: %s\n",
 			       error.get_msg().c_str());
-	_notmuch_database_log_append (notmuch,
+	_notmuch_database_log_append (query->notmuch,
 			       "Query string was: %s\n",
 			       query->query_string);
 
-	talloc_free (query);
-	query = NULL;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
-
-    return query;
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 const char *
@@ -226,6 +235,11 @@ _notmuch_query_search_documents (notmuch_query_t *query,
     notmuch_database_t *notmuch = query->notmuch;
     const char *query_string = query->query_string;
     notmuch_mset_messages_t *messages;
+    notmuch_status_t status;
+
+    status = _notmuch_query_ensure_parsed (query);
+    if (status)
+	return status;
 
     messages = talloc (query, notmuch_mset_messages_t);
     if (unlikely (messages == NULL))
@@ -592,6 +606,11 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
     notmuch_database_t *notmuch = query->notmuch;
     const char *query_string = query->query_string;
     Xapian::doccount count = 0;
+    notmuch_status_t status;
+
+    status = _notmuch_query_ensure_parsed (query);
+    if (status)
+	return status;
 
     try {
 	Xapian::Enquire enquire (*notmuch->xapian_db);
-- 
2.9.3

[PATCH 3/5] test: add known broken test for nondestructiveness of count

[David Bremner]

Thanks to Lucas (id:147263183913.27784.12274024193186585889@mbp) for the
bug report and the test case.

I decided to use the python version because the python bindings could
use more exercise.
---
 test/T060-count.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/test/T060-count.sh b/test/T060-count.sh
index d6933a7..69ab591 100755
--- a/test/T060-count.sh
+++ b/test/T060-count.sh
@@ -126,4 +126,32 @@ sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
 test_expect_equal_file EXPECTED OUTPUT.clean
 restore_database
 
+test_begin_subtest "count library function is non-destructive"
+test_subtest_known_broken
+cat<<EOF > EXPECTED
+1: 52 messages
+2: 52 messages
+Exclude 'spam'
+3: 52 messages
+4: 52 messages
+EOF
+test_python <<EOF
+import sys
+import notmuch
+
+query_string = 'tag:inbox or tag:spam'
+tag_string = 'spam'
+
+database = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)
+query = notmuch.Query(database, query_string)
+
+print("1: {} messages".format(query.count_messages()))
+print("2: {} messages".format(query.count_messages()))
+print("Exclude '{}'".format(tag_string))
+query.exclude_tag(tag_string)
+print("3: {} messages".format(query.count_messages()))
+print("4: {} messages".format(query.count_messages()))
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.9.3

[PATCH 4/5] lib: query make exclude handling non-destructive

[David Bremner]

We filter added exclude at add time, rather than modifying the query by
count search. As noted in the comments, there are several ignored
conditions here. Returning proper status is split into a separate commit
because it is ABI breaking.
---
 lib/query.cc       | 46 ++++++++++++++++++++++++++++------------------
 test/T060-count.sh |  1 -
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index d7cf28f..81e3e01 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -31,6 +31,7 @@ struct _notmuch_query {
     notmuch_exclude_t omit_excluded;
     notmuch_bool_t parsed;
     Xapian::Query xapian_query;
+    std::set<std::string> terms;
 };
 
 typedef struct _notmuch_mset_messages {
@@ -77,6 +78,7 @@ _debug_query (void)
 static int
 _notmuch_query_destructor (notmuch_query_t *query) {
     query->xapian_query.~Query();
+    query->terms.~set<std::string>();
     return 0;
 }
 
@@ -94,6 +96,7 @@ notmuch_query_create (notmuch_database_t *notmuch,
 	return NULL;
 
     new (&query->xapian_query) Xapian::Query ();
+    new (&query->terms) std::set<std::string> ();
     query->parsed = FALSE;
 
     talloc_set_destructor (query, _notmuch_query_destructor);
@@ -121,6 +124,14 @@ _notmuch_query_ensure_parsed (notmuch_query_t *query)
 	query->xapian_query =
 	    query->notmuch->query_parser->
 		parse_query (query->query_string, NOTMUCH_QUERY_PARSER_FLAGS);
+
+	/* apparently Xapian doesn't support skip_to on terms from a query,
+	   so cache a copy of all terms in something searchable */
+
+	for (Xapian::TermIterator t = query->xapian_query.get_terms_begin();
+	     t != query->xapian_query.get_terms_end(); ++t)
+	    query->terms.insert(*t);
+
     } catch (const Xapian::Error &error) {
 	_notmuch_database_log (query->notmuch,
 			       "A Xapian exception occured parsing query: %s\n",
@@ -162,7 +173,17 @@ notmuch_query_get_sort (const notmuch_query_t *query)
 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_status_t status;
+    char *term;
+
+    status = _notmuch_query_ensure_parsed (query);
+    if (status)
+	return; /* XXX report error */
+
+    term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);
+    if (query->terms.count(term) != 0)
+	return; /* XXX report ignoring exclude? */
+
     _notmuch_string_list_append (query->exclude_terms, term);
 }
 
@@ -182,28 +203,17 @@ _notmuch_messages_destructor (notmuch_mset_messages_t *messages)
 }
 
 /* Return a query that matches messages with the excluded tags
- * registered with query.  Any tags that explicitly appear in xquery
- * will not be excluded, and will be removed from the list of exclude
- * tags.  The caller of this function has to combine the returned
+ * registered with query. The caller of this function has to combine the returned
  * query appropriately.*/
 static Xapian::Query
-_notmuch_exclude_tags (notmuch_query_t *query, Xapian::Query xquery)
+_notmuch_exclude_tags (notmuch_query_t *query)
 {
     Xapian::Query exclude_query = Xapian::Query::MatchNothing;
 
     for (notmuch_string_node_t *term = query->exclude_terms->head; term;
 	 term = term->next) {
-	Xapian::TermIterator it = xquery.get_terms_begin ();
-	Xapian::TermIterator end = xquery.get_terms_end ();
-	for (; it != end; it++) {
-	    if ((*it).compare (term->string) == 0)
-		break;
-	}
-	if (it == end)
-	    exclude_query = Xapian::Query (Xapian::Query::OP_OR,
-				    exclude_query, Xapian::Query (term->string));
-	else
-	    term->string = talloc_strdup (query, "");
+	exclude_query = Xapian::Query (Xapian::Query::OP_OR,
+				       exclude_query, Xapian::Query (term->string));
     }
     return exclude_query;
 }
@@ -274,7 +284,7 @@ _notmuch_query_search_documents (notmuch_query_t *query,
 	messages->base.excluded_doc_ids = NULL;
 
 	if ((query->omit_excluded != NOTMUCH_EXCLUDE_FALSE) && (query->exclude_terms)) {
-	    exclude_query = _notmuch_exclude_tags (query, final_query);
+	    exclude_query = _notmuch_exclude_tags (query);
 
 	    if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
 		query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
@@ -629,7 +639,7 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
 					 mail_query, query->xapian_query);
 	}
 
-	exclude_query = _notmuch_exclude_tags (query, final_query);
+	exclude_query = _notmuch_exclude_tags (query);
 
 	final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
 					 final_query, exclude_query);
diff --git a/test/T060-count.sh b/test/T060-count.sh
index 69ab591..b82583b 100755
--- a/test/T060-count.sh
+++ b/test/T060-count.sh
@@ -127,7 +127,6 @@ test_expect_equal_file EXPECTED OUTPUT.clean
 restore_database
 
 test_begin_subtest "count library function is non-destructive"
-test_subtest_known_broken
 cat<<EOF > EXPECTED
 1: 52 messages
 2: 52 messages
-- 
2.9.3

Re: [PATCH 4/5] lib: query make exclude handling non-destructive

[David Bremner]

David Bremner <david@tethera.net> writes:

> +
> +	/* apparently Xapian doesn't support skip_to on terms from a query,
> +	   so cache a copy of all terms in something searchable */

The terms are not sorted, but rather returned in occurence order, so
this really is needed; the comment should be updated to not suggest a
bug in Xapian ;).

> +
> +	for (Xapian::TermIterator t = query->xapian_query.get_terms_begin();
> +	     t != query->xapian_query.get_terms_end(); ++t)
> +	    query->terms.insert(*t);

  we ought to insert

        query->parsed = TRUE

  to prevent the query from being parsed over and over again.

[PATCH 5/5] lib: make notmuch_query_add_tag_exclude return a status value

[David Bremner]

Since this is an ABI breaking change, bump the SONAME.
---
 lib/notmuch.h    | 19 +++++++++++++++++--
 lib/query.cc     |  7 ++++---
 notmuch-count.c  |  9 +++++++--
 notmuch-search.c | 12 ++++++++++--
 notmuch-show.c   | 13 +++++++++++--
 5 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 2faa146..2396aa1 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -180,6 +180,11 @@ typedef enum _notmuch_status {
      */
     NOTMUCH_STATUS_PATH_ERROR,
     /**
+     * The requested operation was ignored. Depending on the function,
+     * this may not be an actual error.
+     */
+    NOTMUCH_STATUS_IGNORED,
+    /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
      */
@@ -807,10 +812,20 @@ notmuch_query_get_sort (const notmuch_query_t *query);
 
 /**
  * Add a tag that will be excluded from the query results by default.
- * This exclusion will be overridden if this tag appears explicitly in
+ * This exclusion will be ignored if this tag appears explicitly in
  * the query.
+ *
+ * @returns
+ *
+ * NOTMUCH_STATUS_SUCCESS: excluded was added successfully.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured.
+ *      Most likely a problem lazily parsing the query string.
+ *
+ * NOTMUCH_STATUS_IGNORED: tag is explicitely present in the query, so
+ *		not excluded.
  */
-void
+notmuch_status_t
 notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag);
 
 /**
diff --git a/lib/query.cc b/lib/query.cc
index 81e3e01..2918543 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -170,7 +170,7 @@ notmuch_query_get_sort (const notmuch_query_t *query)
     return query->sort;
 }
 
-void
+notmuch_status_t
 notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag)
 {
     notmuch_status_t status;
@@ -178,13 +178,14 @@ notmuch_query_add_tag_exclude (notmuch_query_t *query, const char *tag)
 
     status = _notmuch_query_ensure_parsed (query);
     if (status)
-	return; /* XXX report error */
+	return status;
 
     term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);
     if (query->terms.count(term) != 0)
-	return; /* XXX report ignoring exclude? */
+	return NOTMUCH_STATUS_IGNORED;
 
     _notmuch_string_list_append (query->exclude_terms, term);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 /* We end up having to call the destructors explicitly because we had
diff --git a/notmuch-count.c b/notmuch-count.c
index 35a2aa7..3207c01 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -87,8 +87,13 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
 	return -1;
     }
 
-    for (i = 0; i < exclude_tags_length; i++)
-	notmuch_query_add_tag_exclude (query, exclude_tags[i]);
+    for (i = 0; i < exclude_tags_length; i++) {
+	status = notmuch_query_add_tag_exclude (query, exclude_tags[i]);
+	if (status && status != NOTMUCH_STATUS_IGNORED) {
+	    print_status_query ("notmuch count", query, status);
+	    return -1;
+	}
+    }
 
     switch (output) {
     case OUTPUT_MESSAGES:
diff --git a/notmuch-search.c b/notmuch-search.c
index 8c65d5a..64a9811 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -735,11 +735,19 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
     if (ctx->exclude != NOTMUCH_EXCLUDE_FALSE) {
 	const char **search_exclude_tags;
 	size_t search_exclude_tags_length;
+	notmuch_status_t status;
 
 	search_exclude_tags = notmuch_config_get_search_exclude_tags
 	    (config, &search_exclude_tags_length);
-	for (i = 0; i < search_exclude_tags_length; i++)
-	    notmuch_query_add_tag_exclude (ctx->query, search_exclude_tags[i]);
+
+	for (i = 0; i < search_exclude_tags_length; i++) {
+	    status = notmuch_query_add_tag_exclude (ctx->query, search_exclude_tags[i]);
+	    if (status && status != NOTMUCH_STATUS_IGNORED) {
+		print_status_query ("notmuch search", ctx->query, status);
+		return EXIT_FAILURE;
+	    }
+	}
+
 	notmuch_query_set_omit_excluded (ctx->query, ctx->exclude);
     }
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 22fa655..4c63959 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1157,11 +1157,19 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	const char **search_exclude_tags;
 	size_t search_exclude_tags_length;
 	unsigned int i;
+	notmuch_status_t status;
 
 	search_exclude_tags = notmuch_config_get_search_exclude_tags
 	    (config, &search_exclude_tags_length);
-	for (i = 0; i < search_exclude_tags_length; i++)
-	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
+
+	for (i = 0; i < search_exclude_tags_length; i++) {
+	    status = notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
+	    if (status && status != NOTMUCH_STATUS_IGNORED) {
+		print_status_query ("notmuch show", query, status);
+		ret = -1;
+		goto DONE;
+	    }
+	}
 
 	if (exclude == EXCLUDE_FALSE) {
 	    notmuch_query_set_omit_excluded (query, FALSE);
@@ -1171,6 +1179,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	ret = do_show (config, query, format, sprinter, &params);
     }
 
+ DONE:
     notmuch_crypto_cleanup (&params.crypto);
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
-- 
2.9.3

Re: [PATCH 5/5] lib: make notmuch_query_add_tag_exclude return a status value

[David Bremner]

David Bremner <david@tethera.net> writes:

> Since this is an ABI breaking change, bump the SONAME.
> ---
>  lib/notmuch.h    | 19 +++++++++++++++++--
>  lib/query.cc     |  7 ++++---
>  notmuch-count.c  |  9 +++++++--
>  notmuch-search.c | 12 ++++++++++--
>  notmuch-show.c   | 13 +++++++++++--
>  5 files changed, 49 insertions(+), 11 deletions(-)

And I managed to stage, but not commit the following

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 2396aa1..312127c 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -55,8 +55,8 @@ NOTMUCH_BEGIN_DECLS
  * The library version number.  This must agree with the soname
  * version in Makefile.local.
  */
-#define LIBNOTMUCH_MAJOR_VERSION       4
-#define LIBNOTMUCH_MINOR_VERSION       3
+#define LIBNOTMUCH_MAJOR_VERSION       5
+#define LIBNOTMUCH_MINOR_VERSION       0
 #define LIBNOTMUCH_MICRO_VERSION       0
 
yay me.

Re: Fix count/search query destructiveness

[David Bremner]

David Bremner <david@tethera.net> writes:

> As discussed in id:147263183913.27784.12274024193186585889@mbp, the
> current behaviour is destructive due to exclude handling. This makes
> some natural code, including notmuch-reply, buggy.

Mark points out on IRC that notmuch-reply doesn't use excludes, so it's
not buggy just confusing. So the current documentation should be
something like "the query functions are non-destructive, unless you use
excludes".  That makes these changes somewhat less urgent; I still think
they're probably a good idea, since the current is not very nice, and a
bit hard to document/understand.

V2 non-destructive excludes

[David Bremner]

This mainly rebases against master, and fixes the two minor problems
already pointed out.

I would like feedback on breaking the API for this.

[PATCH 4/5] lib: query make exclude handling non-destructive

[David Bremner]

We filter added exclude at add time, rather than modifying the query by
count search. As noted in the comments, there are several ignored
conditions here. Returning proper status is split into a separate commit
because it is ABI breaking.
---
 lib/query.cc       | 48 ++++++++++++++++++++++++++++++------------------
 test/T060-count.sh |  1 -
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index ba5465d..afbabb6 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -31,6 +31,7 @@ struct _notmuch_query {
     notmuch_exclude_t omit_excluded;
     notmuch_bool_t parsed;
     Xapian::Query xapian_query;
+    std::set<std::string> terms;
 };
 
 typedef struct _notmuch_mset_messages {
@@ -77,6 +78,7 @@ _debug_query (void)
 static int
 _notmuch_query_destructor (notmuch_query_t *query) {
     query->xapian_query.~Query();
+    query->terms.~set<std::string>();
     return 0;
 }
 
@@ -94,6 +96,7 @@ notmuch_query_create (notmuch_database_t *notmuch,
 	return NULL;
 
     new (&query->xapian_query) Xapian::Query ();
+    new (&query->terms) std::set<std::string> ();
     query->parsed = FALSE;
 
     talloc_set_destructor (query, _notmuch_query_destructor);
@@ -121,6 +124,16 @@ _notmuch_query_ensure_parsed (notmuch_query_t *query)
 	query->xapian_query =
 	    query->notmuch->query_parser->
 		parse_query (query->query_string, NOTMUCH_QUERY_PARSER_FLAGS);
+
+	/* Xapian doesn't support skip_to on terms from a query since
+	 *  they are unordered, so cache a copy of all terms in
+	 *  something searchable.
+	 */
+
+	for (Xapian::TermIterator t = query->xapian_query.get_terms_begin();
+	     t != query->xapian_query.get_terms_end(); ++t)
+	    query->terms.insert(*t);
+
 	query->parsed = TRUE;
     } catch (const Xapian::Error &error) {
 	_notmuch_database_log (query->notmuch,
@@ -163,7 +176,17 @@ notmuch_query_get_sort (const notmuch_query_t *query)
 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_status_t status;
+    char *term;
+
+    status = _notmuch_query_ensure_parsed (query);
+    if (status)
+	return; /* XXX report error */
+
+    term = talloc_asprintf (query, "%s%s", _find_prefix ("tag"), tag);
+    if (query->terms.count(term) != 0)
+	return; /* XXX report ignoring exclude? */
+
     _notmuch_string_list_append (query->exclude_terms, term);
 }
 
@@ -183,28 +206,17 @@ _notmuch_messages_destructor (notmuch_mset_messages_t *messages)
 }
 
 /* Return a query that matches messages with the excluded tags
- * registered with query.  Any tags that explicitly appear in xquery
- * will not be excluded, and will be removed from the list of exclude
- * tags.  The caller of this function has to combine the returned
+ * registered with query. The caller of this function has to combine the returned
  * query appropriately.*/
 static Xapian::Query
-_notmuch_exclude_tags (notmuch_query_t *query, Xapian::Query xquery)
+_notmuch_exclude_tags (notmuch_query_t *query)
 {
     Xapian::Query exclude_query = Xapian::Query::MatchNothing;
 
     for (notmuch_string_node_t *term = query->exclude_terms->head; term;
 	 term = term->next) {
-	Xapian::TermIterator it = xquery.get_terms_begin ();
-	Xapian::TermIterator end = xquery.get_terms_end ();
-	for (; it != end; it++) {
-	    if ((*it).compare (term->string) == 0)
-		break;
-	}
-	if (it == end)
-	    exclude_query = Xapian::Query (Xapian::Query::OP_OR,
-				    exclude_query, Xapian::Query (term->string));
-	else
-	    term->string = talloc_strdup (query, "");
+	exclude_query = Xapian::Query (Xapian::Query::OP_OR,
+				       exclude_query, Xapian::Query (term->string));
     }
     return exclude_query;
 }
@@ -275,7 +287,7 @@ _notmuch_query_search_documents (notmuch_query_t *query,
 	messages->base.excluded_doc_ids = NULL;
 
 	if ((query->omit_excluded != NOTMUCH_EXCLUDE_FALSE) && (query->exclude_terms)) {
-	    exclude_query = _notmuch_exclude_tags (query, final_query);
+	    exclude_query = _notmuch_exclude_tags (query);
 
 	    if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
 		query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
@@ -630,7 +642,7 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
 					 mail_query, query->xapian_query);
 	}
 
-	exclude_query = _notmuch_exclude_tags (query, final_query);
+	exclude_query = _notmuch_exclude_tags (query);
 
 	final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
 					 final_query, exclude_query);
diff --git a/test/T060-count.sh b/test/T060-count.sh
index 69ab591..b82583b 100755
--- a/test/T060-count.sh
+++ b/test/T060-count.sh
@@ -127,7 +127,6 @@ test_expect_equal_file EXPECTED OUTPUT.clean
 restore_database
 
 test_begin_subtest "count library function is non-destructive"
-test_subtest_known_broken
 cat<<EOF > EXPECTED
 1: 52 messages
 2: 52 messages
-- 
2.9.3