[PATCH 0/8] search --exclude=all

[PATCH 0/8] search --exclude=all

[Peter Wang]

Hi,

This is Mark's change to add search --exclude=all.
I mainly just updated a comment and added some documentation.

Mark Walters (2):
  lib: add --exclude=all option
  cli: add --exclude=all option to notmuch-search.c

Peter Wang (6):
  test: add tests for search --exclude=all
  man: clarify search --exclude documentation
  man: clarify search --exclude=flag
  man: document search --exclude=all
  lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
  cli: use notmuch_exclude_t in option parser

 lib/notmuch-private.h     |    1 +
 lib/notmuch.h             |   23 ++++++++++++++++-------
 lib/query.cc              |   14 +++++++++-----
 lib/thread.cc             |   41 ++++++++++++++++++++++++++++++-----------
 man/man1/notmuch-search.1 |   28 ++++++++++++++++++++++------
 notmuch-search.c          |   24 +++++++++---------------
 test/excludes             |   22 ++++++++++++++++++++++
 7 files changed, 109 insertions(+), 44 deletions(-)

-- 
1.7.4.4

[PATCH 1/8] lib: add --exclude=all option

[Peter Wang]

From: Mark Walters <markwalters1009@gmail.com>

Adds a exclude all option to the lib which means that excluded
messages are completely ignored (as if they had actually been
deleted).
---
 lib/notmuch-private.h |    1 +
 lib/notmuch.h         |   22 +++++++++++++++-------
 lib/query.cc          |   10 ++++++----
 lib/thread.cc         |   41 ++++++++++++++++++++++++++++++-----------
 4 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index bfb4111..aa799b4 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -232,6 +232,7 @@ _notmuch_thread_create (void *ctx,
 			unsigned int seed_doc_id,
 			notmuch_doc_id_set_t *match_set,
 			notmuch_string_list_t *excluded_terms,
+			notmuch_exclude_t omit_exclude,
 			notmuch_sort_t sort);
 
 /* message.cc */
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3633bed..1280afd 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -500,14 +500,22 @@ typedef enum {
 const char *
 notmuch_query_get_query_string (notmuch_query_t *query);
 
+/* Exclude values for notmuch_query_set_omit_excluded */
+typedef enum {
+    NOTMUCH_EXCLUDE_FALSE,
+    NOTMUCH_EXCLUDE_TRUE,
+    NOTMUCH_EXCLUDE_ALL
+} notmuch_exclude_t;
+
 /* Specify whether to omit excluded results or simply flag them.  By
  * default, this is set to TRUE.
  *
- * If this is TRUE, notmuch_query_search_messages will omit excluded
- * messages from the results.  notmuch_query_search_threads will omit
- * threads that match only in excluded messages, but will include all
- * messages in threads that match in at least one non-excluded
- * message.
+ * If set to TRUE or ALL, notmuch_query_search_messages will omit excluded
+ * messages from the results, and notmuch_query_search_threads will omit
+ * threads that match only in excluded messages.  If set to TRUE,
+ * notmuch_query_search_threads will include all messages in threads that
+ * match in at least one non-excluded message.  Otherwise, if set to ALL,
+ * notmuch_query_search_threads will omit excluded messages from all threads.
  *
  * The performance difference when calling
  * notmuch_query_search_messages should be relatively small (and both
@@ -516,9 +524,9 @@ notmuch_query_get_query_string (notmuch_query_t *query);
  * excluded messages as it does not need to construct the threads that
  * only match in excluded messages.
  */
-
 void
-notmuch_query_set_omit_excluded (notmuch_query_t *query, notmuch_bool_t omit_excluded);
+notmuch_query_set_omit_excluded (notmuch_query_t *query,
+				 notmuch_exclude_t omit_excluded);
 
 /* Specify the sorting desired for this query. */
 void
diff --git a/lib/query.cc b/lib/query.cc
index e9c1a2d..f752452 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -28,7 +28,7 @@ struct _notmuch_query {
     const char *query_string;
     notmuch_sort_t sort;
     notmuch_string_list_t *exclude_terms;
-    notmuch_bool_t omit_excluded;
+    notmuch_exclude_t omit_excluded;
 };
 
 typedef struct _notmuch_mset_messages {
@@ -92,7 +92,7 @@ notmuch_query_create (notmuch_database_t *notmuch,
 
     query->exclude_terms = _notmuch_string_list_create (query);
 
-    query->omit_excluded = TRUE;
+    query->omit_excluded = NOTMUCH_EXCLUDE_TRUE;
 
     return query;
 }
@@ -104,7 +104,8 @@ notmuch_query_get_query_string (notmuch_query_t *query)
 }
 
 void
-notmuch_query_set_omit_excluded (notmuch_query_t *query, notmuch_bool_t omit_excluded)
+notmuch_query_set_omit_excluded (notmuch_query_t *query,
+				 notmuch_exclude_t omit_excluded)
 {
     query->omit_excluded = omit_excluded;
 }
@@ -220,7 +221,7 @@ notmuch_query_search_messages (notmuch_query_t *query)
 	if (query->exclude_terms) {
 	    exclude_query = _notmuch_exclude_tags (query, final_query);
 
-	    if (query->omit_excluded)
+	    if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
 		final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
 					     final_query, exclude_query);
 	    else {
@@ -486,6 +487,7 @@ notmuch_threads_get (notmuch_threads_t *threads)
 				   doc_id,
 				   &threads->match_set,
 				   threads->query->exclude_terms,
+				   threads->query->omit_excluded,
 				   threads->query->sort);
 }
 
diff --git a/lib/thread.cc b/lib/thread.cc
index e976d64..4cb0896 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -215,7 +215,8 @@ _thread_cleanup_author (notmuch_thread_t *thread,
 static void
 _thread_add_message (notmuch_thread_t *thread,
 		     notmuch_message_t *message,
-		     notmuch_string_list_t *exclude_terms)
+		     notmuch_string_list_t *exclude_terms,
+		     notmuch_exclude_t omit_exclude)
 {
     notmuch_tags_t *tags;
     const char *tag;
@@ -223,6 +224,28 @@ _thread_add_message (notmuch_thread_t *thread,
     InternetAddress *address;
     const char *from, *author;
     char *clean_author;
+    notmuch_bool_t message_excluded = FALSE;
+
+    for (tags = notmuch_message_get_tags (message);
+	 notmuch_tags_valid (tags);
+	 notmuch_tags_move_to_next (tags))
+    {
+	tag = notmuch_tags_get (tags);
+	/* Is message excluded? */
+	for (notmuch_string_node_t *term = exclude_terms->head;
+	     term != NULL;
+	     term = term->next)
+	{
+	    /* We ignore initial 'K'. */
+	    if (strcmp(tag, (term->string + 1)) == 0) {
+		message_excluded = TRUE;
+		break;
+	    }
+	}
+    }
+
+    if (message_excluded && omit_exclude == NOTMUCH_EXCLUDE_ALL)
+	return;
 
     _notmuch_message_list_add_message (thread->message_list,
 				       talloc_steal (thread, message));
@@ -263,17 +286,12 @@ _thread_add_message (notmuch_thread_t *thread,
 	 notmuch_tags_move_to_next (tags))
     {
 	tag = notmuch_tags_get (tags);
-	/* Mark excluded messages. */
-	for (notmuch_string_node_t *term = exclude_terms->head; term;
-	     term = term->next) {
-	    /* We ignore initial 'K'. */
-	    if (strcmp(tag, (term->string + 1)) == 0) {
-		notmuch_message_set_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED, TRUE);
-		break;
-	    }
-	}
 	g_hash_table_insert (thread->tags, xstrdup (tag), NULL);
     }
+
+    /* Mark excluded messages. */
+    if (message_excluded)
+	notmuch_message_set_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED, TRUE);
 }
 
 static void
@@ -404,6 +422,7 @@ _notmuch_thread_create (void *ctx,
 			unsigned int seed_doc_id,
 			notmuch_doc_id_set_t *match_set,
 			notmuch_string_list_t *exclude_terms,
+			notmuch_exclude_t omit_excluded,
 			notmuch_sort_t sort)
 {
     notmuch_thread_t *thread;
@@ -479,7 +498,7 @@ _notmuch_thread_create (void *ctx,
 	if (doc_id == seed_doc_id)
 	    message = seed_message;
 
-	_thread_add_message (thread, message, exclude_terms);
+	_thread_add_message (thread, message, exclude_terms, omit_excluded);
 
 	if ( _notmuch_doc_id_set_contains (match_set, doc_id)) {
 	    _notmuch_doc_id_set_remove (match_set, doc_id);
-- 
1.7.4.4

[PATCH 2/8] cli: add --exclude=all option to notmuch-search.c

[Peter Wang]

From: Mark Walters <markwalters1009@gmail.com>

Add a --exclude=all option to notmuch search.
---
 notmuch-search.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 3be296d..89b5bf9 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -420,6 +420,7 @@ enum {
     EXCLUDE_TRUE,
     EXCLUDE_FALSE,
     EXCLUDE_FLAG,
+    EXCLUDE_ALL
 };
 
 int
@@ -461,6 +462,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
           (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
                                   { "false", EXCLUDE_FALSE },
                                   { "flag", EXCLUDE_FLAG },
+                                  { "all", EXCLUDE_ALL },
                                   { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
 	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
@@ -516,7 +518,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	exclude = EXCLUDE_FALSE;
     }
 
-    if (exclude == EXCLUDE_TRUE || exclude == EXCLUDE_FLAG) {
+    if (exclude != EXCLUDE_FALSE) {
 	const char **search_exclude_tags;
 	size_t search_exclude_tags_length;
 
@@ -525,7 +527,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	for (i = 0; i < search_exclude_tags_length; i++)
 	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
 	if (exclude == EXCLUDE_FLAG)
-	    notmuch_query_set_omit_excluded (query, FALSE);
+	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FALSE);
+	if (exclude == EXCLUDE_ALL)
+	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_ALL);
     }
 
     switch (output) {
-- 
1.7.4.4

[PATCH 3/8] test: add tests for search --exclude=all

[Peter Wang]

Test the new search --exclude=all option.
---
 test/excludes |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/test/excludes b/test/excludes
index 24d653e..f1ae9ea 100755
--- a/test/excludes
+++ b/test/excludes
@@ -166,6 +166,16 @@ ${matching_message_ids[3]}
 ${matching_message_ids[4]}
 ${matching_message_ids[5]}"
 
+test_begin_subtest "Search, exclude=all (thread summary)"
+output=$(notmuch search --exclude=all tag:test | notmuch_search_sanitize)
+test_expect_equal "$output" "thread:XXX   2001-01-05 [1/5] Notmuch Test Suite; Some messages excluded: single non-excluded match: reply 4 (inbox test unread)
+thread:XXX   2001-01-05 [1/6] Notmuch Test Suite; No messages excluded: single match: reply 3 (inbox test unread)"
+
+test_begin_subtest "Search, exclude=all (messages)"
+output=$(notmuch search --exclude=all --output=messages tag:test | notmuch_search_sanitize)
+test_expect_equal "$output" "${matching_message_ids[4]}
+${matching_message_ids[5]}"
+
 test_begin_subtest "Search, default exclusion: tag in query (thread summary)"
 output=$(notmuch search tag:test and tag:deleted | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2001-01-05 [1/6] Notmuch Test Suite; All messages excluded: single match: reply 2 (deleted inbox test unread)
@@ -218,6 +228,18 @@ ${matching_message_ids[1]}
 ${matching_message_ids[2]}
 ${matching_message_ids[3]}"
 
+test_begin_subtest "Search, exclude=all: tag in query (thread summary)"
+output=$(notmuch search --exclude=all tag:test and tag:deleted | notmuch_search_sanitize)
+test_expect_equal "$output" "thread:XXX   2001-01-05 [1/6] Notmuch Test Suite; All messages excluded: single match: reply 2 (deleted inbox test unread)
+thread:XXX   2001-01-05 [2/6] Notmuch Test Suite; All messages excluded: double match: reply 2 (deleted inbox test unread)
+thread:XXX   2001-01-05 [1/6] Notmuch Test Suite; Some messages excluded: single excluded match: reply 3 (deleted inbox test unread)"
+
+test_begin_subtest "Search, exclude=all: tag in query (messages)"
+output=$(notmuch search --exclude=all --output=messages tag:test and tag:deleted | notmuch_search_sanitize)
+test_expect_equal "$output" "${matching_message_ids[0]}
+${matching_message_ids[1]}
+${matching_message_ids[2]}
+${matching_message_ids[3]}"
 
 #########################################################
 # Notmuch count tests
-- 
1.7.4.4

[PATCH 4/8] man: clarify search --exclude documentation

[Peter Wang]

Highlight "excluded messages" as a piece of jargon with
a meaning that may not be obvious.

Be explicit about the effects of search --exclude=true and
--exclude=false.
---
 man/man1/notmuch-search.1 |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/man/man1/notmuch-search.1 b/man/man1/notmuch-search.1
index 5c72c4b..003b742 100644
--- a/man/man1/notmuch-search.1
+++ b/man/man1/notmuch-search.1
@@ -116,8 +116,18 @@ Limit the number of displayed results to N.
 .TP 4
 .BR \-\-exclude=(true|false|flag)
 
-Specify whether to omit messages matching search.tag_exclude from the
-search results (the default) or not. The extra option
+Messages matching search.tag_exclude are called "excluded messages".
+This option specifies whether to omit excluded messages in the search
+process.
+
+The default value,
+.BR true ,
+prevents excluded messages from matching the search terms.
+
+.B false
+allows excluded messages to match search terms and appear in displayed
+results. Excluded messages are still marked in the relevant outputs.
+
 .B flag
 only has an effect when
 .B --output=summary
-- 
1.7.4.4

[PATCH 5/8] man: clarify search --exclude=flag

[Peter Wang]

Improve the description of the search --exclude=flag option,
using text taken from the commit that introduced the option.
---
 man/man1/notmuch-search.1 |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/man/man1/notmuch-search.1 b/man/man1/notmuch-search.1
index 003b742..e23ca30 100644
--- a/man/man1/notmuch-search.1
+++ b/man/man1/notmuch-search.1
@@ -130,9 +130,11 @@ results. Excluded messages are still marked in the relevant outputs.
 
 .B flag
 only has an effect when
-.B --output=summary
-In this case all matching threads are returned but the "match count"
-is the number of matching non-excluded messages in the thread.
+.BR --output=summary .
+The output is almost identical to
+.BR false ,
+but the "match count" is the number of matching non-excluded messages in the
+thread, rather than the number of matching messages.
 .RE
 
 .SH SEE ALSO
-- 
1.7.4.4

[PATCH 6/8] man: document search --exclude=all

[Peter Wang]

Document the new search --exclude=all option.
---
 man/man1/notmuch-search.1 |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/man/man1/notmuch-search.1 b/man/man1/notmuch-search.1
index e23ca30..1d8d57a 100644
--- a/man/man1/notmuch-search.1
+++ b/man/man1/notmuch-search.1
@@ -114,7 +114,7 @@ Limit the number of displayed results to N.
 
 .RS 4
 .TP 4
-.BR \-\-exclude=(true|false|flag)
+.BR \-\-exclude=(true|false|all|flag)
 
 Messages matching search.tag_exclude are called "excluded messages".
 This option specifies whether to omit excluded messages in the search
@@ -124,6 +124,10 @@ The default value,
 .BR true ,
 prevents excluded messages from matching the search terms.
 
+.B all
+additionally prevents excluded messages from appearing in displayed
+results, in effect behaving as though the excluded messages do not exist.
+
 .B false
 allows excluded messages to match search terms and appear in displayed
 results. Excluded messages are still marked in the relevant outputs.
-- 
1.7.4.4

[PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Peter Wang]

Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can
cover all four values of search --exclude in the cli.
---
 lib/notmuch.h    |    1 +
 lib/query.cc     |    6 ++++--
 notmuch-search.c |    2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 1280afd..f4381ae 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -504,6 +504,7 @@ notmuch_query_get_query_string (notmuch_query_t *query);
 typedef enum {
     NOTMUCH_EXCLUDE_FALSE,
     NOTMUCH_EXCLUDE_TRUE,
+    NOTMUCH_EXCLUDE_FLAG,
     NOTMUCH_EXCLUDE_ALL
 } notmuch_exclude_t;
 
diff --git a/lib/query.cc b/lib/query.cc
index f752452..1e0f292 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -221,10 +221,12 @@ notmuch_query_search_messages (notmuch_query_t *query)
 	if (query->exclude_terms) {
 	    exclude_query = _notmuch_exclude_tags (query, final_query);
 
-	    if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
+	    if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
+		query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
+	    {
 		final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
 					     final_query, exclude_query);
-	    else {
+	    } else {
 		exclude_query = Xapian::Query (Xapian::Query::OP_AND,
 					   exclude_query, final_query);
 
diff --git a/notmuch-search.c b/notmuch-search.c
index 89b5bf9..027923d 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -527,7 +527,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	for (i = 0; i < search_exclude_tags_length; i++)
 	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
 	if (exclude == EXCLUDE_FLAG)
-	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FALSE);
+	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FLAG);
 	if (exclude == EXCLUDE_ALL)
 	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_ALL);
     }
-- 
1.7.4.4

[PATCH 8/8] cli: use notmuch_exclude_t in option parser

[Peter Wang]

Use notmuch_exclude_t constants directly instead of a redundant
enumeration while parsing search --exclude keyword arguments.
---
 notmuch-search.c |   28 +++++++++-------------------
 1 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 027923d..23cf342 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -416,13 +416,6 @@ do_search_tags (notmuch_database_t *notmuch,
     return 0;
 }
 
-enum {
-    EXCLUDE_TRUE,
-    EXCLUDE_FALSE,
-    EXCLUDE_FLAG,
-    EXCLUDE_ALL
-};
-
 int
 notmuch_search_command (void *ctx, int argc, char *argv[])
 {
@@ -436,7 +429,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     output_t output = OUTPUT_SUMMARY;
     int offset = 0;
     int limit = -1; /* unlimited */
-    int exclude = EXCLUDE_TRUE;
+    notmuch_exclude_t exclude = NOTMUCH_EXCLUDE_TRUE;
     unsigned int i;
 
     enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
@@ -459,10 +452,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 				  { "tags", OUTPUT_TAGS },
 				  { 0, 0 } } },
         { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
-          (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
-                                  { "false", EXCLUDE_FALSE },
-                                  { "flag", EXCLUDE_FLAG },
-                                  { "all", EXCLUDE_ALL },
+          (notmuch_keyword_t []){ { "true", NOTMUCH_EXCLUDE_TRUE },
+                                  { "false", NOTMUCH_EXCLUDE_FALSE },
+                                  { "flag", NOTMUCH_EXCLUDE_FLAG },
+                                  { "all", NOTMUCH_EXCLUDE_ALL },
                                   { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
 	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
@@ -510,15 +503,15 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 
     notmuch_query_set_sort (query, sort);
 
-    if (exclude == EXCLUDE_FLAG && output != OUTPUT_SUMMARY) {
+    if (exclude == NOTMUCH_EXCLUDE_FLAG && output != OUTPUT_SUMMARY) {
 	/* If we are not doing summary output there is nowhere to
 	 * print the excluded flag so fall back on including the
 	 * excluded messages. */
 	fprintf (stderr, "Warning: this output format cannot flag excluded messages.\n");
-	exclude = EXCLUDE_FALSE;
+	exclude = NOTMUCH_EXCLUDE_FALSE;
     }
 
-    if (exclude != EXCLUDE_FALSE) {
+    if (exclude != NOTMUCH_EXCLUDE_FALSE) {
 	const char **search_exclude_tags;
 	size_t search_exclude_tags_length;
 
@@ -526,10 +519,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	    (config, &search_exclude_tags_length);
 	for (i = 0; i < search_exclude_tags_length; i++)
 	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
-	if (exclude == EXCLUDE_FLAG)
-	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FLAG);
-	if (exclude == EXCLUDE_ALL)
-	    notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_ALL);
+	notmuch_query_set_omit_excluded (query, exclude);
     }
 
     switch (output) {
-- 
1.7.4.4

Re: [PATCH 4/8] man: clarify search --exclude documentation

[Mark Walters]

I have reviewed all the new parts of this series (judged as being
patches 3-8) and the changes made to my two patches and they are all
fine (with one small comment below). Patch 1/8 does need a proper review
though as it ended up more intrusive than I would have liked.

> +Messages matching search.tag_exclude are called "excluded messages".

My one comment is that this is not quite true if the corresponding tag
is in the query. Since you are defining the term it would be nice to
mention that, but I can't see a clean wording.

This should not hold up the series as it your wording is clearly an
improvement on the current wording.

Best wishes

Mark

Re: [PATCH 4/8] man: clarify search --exclude documentation

[Peter Wang]

On Wed, 20 Jun 2012 21:08:05 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> 
> I have reviewed all the new parts of this series (judged as being
> patches 3-8) and the changes made to my two patches and they are all
> fine (with one small comment below). Patch 1/8 does need a proper review
> though as it ended up more intrusive than I would have liked.
> 
> > +Messages matching search.tag_exclude are called "excluded messages".
> 
> My one comment is that this is not quite true if the corresponding tag
> is in the query. Since you are defining the term it would be nice to
> mention that, but I can't see a clean wording.

How about:

    A message is called "excluded" if it matches at least one tag
    in search.tag_exclude that does not appear explicitly in the search
    terms.

Or less densely:

    Let "excluded tags" be the set of tags listed in search.tag_exclude,
    minus any tags which appear explicitly in the search terms.
    A message is an "excluded message" if it matches one or more
    excluded tags.

Peter

Re: [PATCH 4/8] man: clarify search --exclude documentation

[Mark Walters]

Peter Wang <novalazy@gmail.com> writes:

> On Wed, 20 Jun 2012 21:08:05 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> 
>> I have reviewed all the new parts of this series (judged as being
>> patches 3-8) and the changes made to my two patches and they are all
>> fine (with one small comment below). Patch 1/8 does need a proper review
>> though as it ended up more intrusive than I would have liked.
>> 
>> > +Messages matching search.tag_exclude are called "excluded messages".
>> 
>> My one comment is that this is not quite true if the corresponding tag
>> is in the query. Since you are defining the term it would be nice to
>> mention that, but I can't see a clean wording.
>
> How about:
>
>     A message is called "excluded" if it matches at least one tag
>     in search.tag_exclude that does not appear explicitly in the search
>     terms.

I think this wording is excellent (and prefer it to the less dense wording).

Best wishes

Mark

> Or less densely:
>
>     Let "excluded tags" be the set of tags listed in search.tag_exclude,
>     minus any tags which appear explicitly in the search terms.
>     A message is an "excluded message" if it matches one or more
>     excluded tags.
>
> Peter

[PATCH v2 4/8] man: clarify search --exclude documentation

[Peter Wang]

Highlight "excluded messages" as a term with a meaning that
may not be obvious.

Be explicit about the effects of search --exclude=true and
--exclude=false.
---
 man/man1/notmuch-search.1 |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/man/man1/notmuch-search.1 b/man/man1/notmuch-search.1
index 5c72c4b..52782c7 100644
--- a/man/man1/notmuch-search.1
+++ b/man/man1/notmuch-search.1
@@ -116,8 +116,19 @@ Limit the number of displayed results to N.
 .TP 4
 .BR \-\-exclude=(true|false|flag)
 
-Specify whether to omit messages matching search.tag_exclude from the
-search results (the default) or not. The extra option
+A message is called "excluded" if it matches at least one tag in
+search.tag_exclude that does not appear explicitly in the search terms.
+This option specifies whether to omit excluded messages in the search
+process.
+
+The default value,
+.BR true ,
+prevents excluded messages from matching the search terms.
+
+.B false
+allows excluded messages to match search terms and appear in displayed
+results. Excluded messages are still marked in the relevant outputs.
+
 .B flag
 only has an effect when
 .B --output=summary
-- 
1.7.4.4

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Ethan Glasser-Camp]

Peter Wang <novalazy@gmail.com> writes:

> Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can
> cover all four values of search --exclude in the cli.

This series looks good to me. It's a nice clean up and a nice new
feature. Patches all apply.

However, I'm getting test failures like:

 FAIL   Search, exclude "deleted" messages from message search --exclude=false
        --- excludes.3.expected 2012-10-19 04:45:06.900518377 +0000
        +++ excludes.3.output   2012-10-19 04:45:06.900518377 +0000
        @@ -1,2 +1,2 @@
        -id:msg-001@notmuch-test-suite
         id:msg-002@notmuch-test-suite
        +id:msg-001@notmuch-test-suite

 FAIL   Search, don't exclude "deleted" messages when --exclude=flag specified
        --- excludes.7.expected 2012-10-19 04:45:07.004518378 +0000
        +++ excludes.7.output   2012-10-19 04:45:07.004518378 +0000
        @@ -1,2 +1,2 @@
        -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
         thread:XXX   2001-01-05 [1/2] Notmuch Test Suite; Not deleted reply (deleted inbox unread)
        +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)

 FAIL   Search, don't exclude "deleted" messages from search if not configured
        --- excludes.8.expected 2012-10-19 04:45:07.028518377 +0000
        +++ excludes.8.output   2012-10-19 04:45:07.028518377 +0000
        @@ -1,2 +1,2 @@
        -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
         thread:XXX   2001-01-05 [2/2] Notmuch Test Suite; Deleted (deleted inbox unread)
        +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)

In other words, threads and messages are coming up out of order. I'm not
sure of the right way to fix this. If you would like me to try sticking
"| sort" here and there in the tests I will do so. I'm not sure if the
test suite is guaranteed to scan messages in a certain order.

Mark Walters wrote in
id:"1340198947-29370-5-git-send-email-novalazy@gmail.com" that he
thought patch 1/8 seemed more intrusive than he liked. Maybe I just have
a higher standard for "intrusive" than he does ;) but I thought it was
fine.

It looks like you have better wording for patch 4/8 so I'd like to see
you resend it.

> -         if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
> +         if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
> +             query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
> +         {
>               final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
>                                            final_query, exclude_query);
> -         else {
> +         } else {

"House style" is to not put braces around one-line then-clauses. This is
the only place where you did that.

I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit!

Ethan

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Ethan Glasser-Camp]

Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> writes:

> It looks like you have better wording for patch 4/8 so I'd like to see
> you resend it.
>
> I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit!

It turns out that patch 4 already has a v2 in the thread, but I didn't
see it due to some kind of selective blindness. It would be nice if
nmbug had grouped it as part of the same patch series. 

I'm marking the old patch 4/8 obsolete. Only patch 3 and 7 remain.

Ethan

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Tomi Ollila]

On Sat, Oct 20 2012, Ethan Glasser-Camp wrote:

> Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> writes:
>
>> It looks like you have better wording for patch 4/8 so I'd like to see
>> you resend it.
>>
>> I'm marking patches 3, 4, and 7 as moreinfo. Please resubmit!
>
> It turns out that patch 4 already has a v2 in the thread, but I didn't
> see it due to some kind of selective blindness. It would be nice if
> nmbug had grouped it as part of the same patch series. 

I noticed the same today when I tagged your NEWS patch.

By looking contrib/nmbug/nmbug-status the reason seems obvious:
it iterates over messages and resets thread when thread id changes
(and message ordering is not based on threads).

There is 2 ways to "fix" this: 

1) Iterate over threads and then messges over these threads. I looked
the bindings and this didn't seem so easy as one could imagine..

2) Iterate over messages like now, but cache the content to an objects
in hash where thread-id is the key.
 
If no-one points me an easy way to do (1), I'll attempt (2) (in a distant
future ;)

>
> I'm marking the old patch 4/8 obsolete. Only patch 3 and 7 remain.
>
> Ethan

Tomi

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Peter Wang]

On Fri, 19 Oct 2012 01:15:31 -0400, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
> Peter Wang <novalazy@gmail.com> writes:
> 
> > Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can
> > cover all four values of search --exclude in the cli.
> 
> This series looks good to me. It's a nice clean up and a nice new
> feature. Patches all apply.

Thanks for the review.

> However, I'm getting test failures like:
> 
>  FAIL   Search, exclude "deleted" messages from message search --exclude=false
>         --- excludes.3.expected 2012-10-19 04:45:06.900518377 +0000
>         +++ excludes.3.output   2012-10-19 04:45:06.900518377 +0000
>         @@ -1,2 +1,2 @@
>         -id:msg-001@notmuch-test-suite
>          id:msg-002@notmuch-test-suite
>         +id:msg-001@notmuch-test-suite
> 
>  FAIL   Search, don't exclude "deleted" messages when --exclude=flag specified
>         --- excludes.7.expected 2012-10-19 04:45:07.004518378 +0000
>         +++ excludes.7.output   2012-10-19 04:45:07.004518378 +0000
>         @@ -1,2 +1,2 @@
>         -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>          thread:XXX   2001-01-05 [1/2] Notmuch Test Suite; Not deleted reply (deleted inbox unread)
>         +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
> 
>  FAIL   Search, don't exclude "deleted" messages from search if not configured
>         --- excludes.8.expected 2012-10-19 04:45:07.028518377 +0000
>         +++ excludes.8.output   2012-10-19 04:45:07.028518377 +0000
>         @@ -1,2 +1,2 @@
>         -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>          thread:XXX   2001-01-05 [2/2] Notmuch Test Suite; Deleted (deleted inbox unread)
>         +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
> 
> In other words, threads and messages are coming up out of order. I'm not
> sure of the right way to fix this. If you would like me to try sticking
> "| sort" here and there in the tests I will do so. I'm not sure if the
> test suite is guaranteed to scan messages in a certain order.

Does it help if you add a "sleep 1" before the second generate_message
call, i.e. on line 35?

> > -         if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
> > +         if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
> > +             query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
> > +         {
> >               final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
> >                                            final_query, exclude_query);
> > -         else {
> > +         } else {
> 
> "House style" is to not put braces around one-line then-clauses. This is
> the only place where you did that.

I have to disagree.  The condition is wrapped over two lines.  The then
part is wrapped over two lines.  The else part already has braces.
All suggest braces around the then part.

Peter

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Tomi Ollila]

On Sun, Oct 21 2012, Peter Wang wrote:

> On Fri, 19 Oct 2012 01:15:31 -0400, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
>> Peter Wang <novalazy@gmail.com> writes:
>> 
>> > Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can
>> > cover all four values of search --exclude in the cli.
>> 
>> This series looks good to me. It's a nice clean up and a nice new
>> feature. Patches all apply.
>
> Thanks for the review.
>
>> However, I'm getting test failures like:
>> 
>>  FAIL   Search, exclude "deleted" messages from message search --exclude=false
>>         --- excludes.3.expected 2012-10-19 04:45:06.900518377 +0000
>>         +++ excludes.3.output   2012-10-19 04:45:06.900518377 +0000
>>         @@ -1,2 +1,2 @@
>>         -id:msg-001@notmuch-test-suite
>>          id:msg-002@notmuch-test-suite
>>         +id:msg-001@notmuch-test-suite
>> 
>>  FAIL   Search, don't exclude "deleted" messages when --exclude=flag specified
>>         --- excludes.7.expected 2012-10-19 04:45:07.004518378 +0000
>>         +++ excludes.7.output   2012-10-19 04:45:07.004518378 +0000
>>         @@ -1,2 +1,2 @@
>>         -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>>          thread:XXX   2001-01-05 [1/2] Notmuch Test Suite; Not deleted reply (deleted inbox unread)
>>         +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>> 
>>  FAIL   Search, don't exclude "deleted" messages from search if not configured
>>         --- excludes.8.expected 2012-10-19 04:45:07.028518377 +0000
>>         +++ excludes.8.output   2012-10-19 04:45:07.028518377 +0000
>>         @@ -1,2 +1,2 @@
>>         -thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>>          thread:XXX   2001-01-05 [2/2] Notmuch Test Suite; Deleted (deleted inbox unread)
>>         +thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
>> 
>> In other words, threads and messages are coming up out of order. I'm not
>> sure of the right way to fix this. If you would like me to try sticking
>> "| sort" here and there in the tests I will do so. I'm not sure if the
>> test suite is guaranteed to scan messages in a certain order.
>
> Does it help if you add a "sleep 1" before the second generate_message
> call, i.e. on line 35?
>
>> > -         if (query->omit_excluded != NOTMUCH_EXCLUDE_FALSE)
>> > +         if (query->omit_excluded == NOTMUCH_EXCLUDE_TRUE ||
>> > +             query->omit_excluded == NOTMUCH_EXCLUDE_ALL)
>> > +         {
>> >               final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
>> >                                            final_query, exclude_query);
>> > -         else {
>> > +         } else {
>> 
>> "House style" is to not put braces around one-line then-clauses. This is
>> the only place where you did that.
>
> I have to disagree.  The condition is wrapped over two lines.  The then
> part is wrapped over two lines.  The else part already has braces.
> All suggest braces around the then part.

Well, I personally would count none of these as convincing suggestions ;),
but IMHO the braces are OK here (I don't start judging which I'd like more).

> Peter

Tomi

Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t

[Ethan Glasser-Camp]

Peter Wang <novalazy@gmail.com> writes:

> Does it help if you add a "sleep 1" before the second generate_message
> call, i.e. on line 35?

It turns out that this test failure is sporadic (perhaps due to the fact
that I'm running on tmpfs) and exists even before this series. Doing
"sleep 1" makes it go away, but that "fix" is unrelated to this series
and ought to be fixed elsewhere.

> I have to disagree.  The condition is wrapped over two lines.  The then
> part is wrapped over two lines.  The else part already has braces.
> All suggest braces around the then part.

If Tomi is OK with it, then I guess it's fine. And it's true that there
are a couple of places where braces are used with long conditions and
then-parts.

This series has my +1.

Ethan

Re: [PATCH 1/8] lib: add --exclude=all option

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> From: Mark Walters <markwalters1009@gmail.com>
>
> Adds a exclude all option to the lib which means that excluded
> messages are completely ignored (as if they had actually been
> deleted).

Hi Peter, Mark;
 
This patch doesn't apply anymore, which makes it hard to test the
series.  At least by eye the series looks OK to me. Unless
somebody objects, I'd be inclined to push a rebased version of this
series.

d