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

[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

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 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 | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index 098ed8f..ba5465d 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,31 @@ 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);
+	query->parsed = TRUE;
     } 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 +236,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 +607,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       | 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

[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    | 23 +++++++++++++++++++----
 lib/query.cc     |  7 ++++---
 notmuch-count.c  |  9 +++++++--
 notmuch-search.c | 12 ++++++++++--
 notmuch-show.c   | 13 +++++++++++--
 5 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 251c29b..3b4a3aa 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	4
+#define LIBNOTMUCH_MAJOR_VERSION	5
+#define LIBNOTMUCH_MINOR_VERSION	0
 #define LIBNOTMUCH_MICRO_VERSION	0
 
 
@@ -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,
+    /**
      * One of the arguments violates the preconditions for the
      * function, in a way not covered by a more specific argument.
      */
@@ -812,10 +817,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 afbabb6..d8cbc7b 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -173,7 +173,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;
@@ -181,13 +181,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 3/5] test: add known broken test for nondestructiveness of count

[David Bremner]

David Bremner <david@tethera.net> writes:

> 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.

pushed this one patch to master.

d