[Patch v2 0/3] Add test and fix exclude bug

[Patch v2 0/3] Add test and fix exclude bug

[Mark Walters]

The test in the previous patches
id:"1331551914-28323-1-git-send-email-markwalters1009@gmail.com"
triggered the bug accidentally. It accidentally set the exclude tags
to be "=" and "deleted" rather than just "deleted". The non-existent
"=" tag (i.e., the tag that does not occur anywhere in the Xapian
database) triggered the bug. First we correct the tests to just set
the "deleted" tag, then we add some tests including one with a
deliberately non-existent tag. Finally, we modify the exclusion code
to avoid the bug.

The modification also appears to fix a similar problem for jrollins
that I have not yet been able to reproduce.

Since the code is now more clearly correct, cleaner and we do
definitely avoid a Xapian bug I think these patches can be committed
even without knowing why it fixed jrollins' bug.

Finally these apply to current master (not to the --with-excluded series).

Best wishes

Mark

Mark Walters (3):
  test: the test for the exclude code mistakenly excludes the tag "="
  test: add tests for message only search
  lib: fix an exclude bug

 lib/query.cc |    5 +++--
 test/count   |    2 +-
 test/search  |   19 ++++++++++++++++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
1.7.9.1

[Patch v2 1/3] test: the test for the exclude code mistakenly excludes the tag "="

[Mark Walters]

The tests for the exclude code in search and count use the line
    notmuch config set search.exclude_tags = deleted
which actually sets the exclude tags to be "=" and "deleted". Remove
the "=" from this line.
---
 test/count  |    2 +-
 test/search |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/count b/test/count
index 976fff1..b97fc06 100755
--- a/test/count
+++ b/test/count
@@ -38,7 +38,7 @@ test_expect_equal \
     "`notmuch count --output=threads ${SEARCH}`"
 
 test_begin_subtest "count excluding \"deleted\" messages"
-notmuch config set search.exclude_tags = deleted
+notmuch config set search.exclude_tags deleted
 generate_message '[subject]="Not deleted"'
 generate_message '[subject]="Another not deleted"'
 generate_message '[subject]="Deleted"'
diff --git a/test/search b/test/search
index 081f60c..3e3a462 100755
--- a/test/search
+++ b/test/search
@@ -130,7 +130,7 @@ output=$(notmuch search "bödý" | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; utf8-message-body-subject (inbox unread)"
 
 test_begin_subtest "Exclude \"deleted\" messages from search"
-notmuch config set search.exclude_tags = deleted
+notmuch config set search.exclude_tags deleted
 generate_message '[subject]="Not deleted"'
 generate_message '[subject]="Deleted"'
 notmuch new > /dev/null
-- 
1.7.9.1

[Patch v2 2/3] test: add tests for message only search

[Mark Walters]

This adds three tests for --output=messages searches. One test is for
the case when one exclude tag does not occur in the Xapian
database. This triggers a Xapian bug in some cases and causes the
whole exclusion to fail. The next commit avoids this bug.
---
 test/search |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/test/search b/test/search
index 3e3a462..17af6a2 100755
--- a/test/search
+++ b/test/search
@@ -132,13 +132,30 @@ test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; u
 test_begin_subtest "Exclude \"deleted\" messages from search"
 notmuch config set search.exclude_tags deleted
 generate_message '[subject]="Not deleted"'
+not_deleted_id=$gen_msg_id
 generate_message '[subject]="Deleted"'
 notmuch new > /dev/null
 notmuch tag +deleted id:$gen_msg_id
+deleted_id=$gen_msg_id
 output=$(notmuch search subject:deleted | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Not deleted (inbox unread)
 thread:XXX   2001-01-05 [0/1] Notmuch Test Suite; Deleted (deleted inbox unread)"
 
+test_begin_subtest "Exclude \"deleted\" messages from message search"
+output=$(notmuch search --output=messages subject:deleted | notmuch_search_sanitize)
+test_expect_equal "$output" "id:$not_deleted_id"
+
+test_begin_subtest "Exclude \"deleted\" messages from message search (no-exclude)"
+output=$(notmuch search --no-exclude --output=messages subject:deleted | notmuch_search_sanitize)
+test_expect_equal "$output" "id:$not_deleted_id
+id:$deleted_id"
+
+test_begin_subtest "Exclude \"deleted\" messages from message search (non-existent exclude-tag)"
+notmuch config set search.exclude_tags deleted non_existent_tag
+output=$(notmuch search --output=messages subject:deleted | notmuch_search_sanitize)
+test_expect_equal "$output" "id:$not_deleted_id"
+notmuch config set search.exclude_tags deleted
+
 test_begin_subtest "Exclude \"deleted\" messages from search, overridden"
 output=$(notmuch search subject:deleted and tag:deleted | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Deleted (deleted inbox unread)"
-- 
1.7.9.1

[Patch v2 3/3] lib: fix an exclude bug

[Mark Walters]

When the exclude tags contain a tag that does not occur anywhere in
the Xapian database the exclusion fails. We modify the way the query
is constructed to `work around' this. (In fact the new code is cleaner
anyway.)

It also seems to fix another exclusion failure bug reported by
jrollins but we have not yet worked out why it helps in that case.
---
 lib/query.cc |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index ab18fbc..2b73d72 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -213,13 +213,14 @@ notmuch_query_search_messages (notmuch_query_t *query)
 
 	if (query->exclude_terms) {
 	    exclude_query = _notmuch_exclude_tags (query, final_query);
-	    exclude_query = Xapian::Query (Xapian::Query::OP_AND,
-					   exclude_query, final_query);
 
 	    if (query->omit_excluded_messages)
 		final_query = Xapian::Query (Xapian::Query::OP_AND_NOT,
 					     final_query, exclude_query);
 	    else {
+		exclude_query = Xapian::Query (Xapian::Query::OP_AND,
+					   exclude_query, final_query);
+
 		enquire.set_weighting_scheme (Xapian::BoolWeight());
 		enquire.set_query (exclude_query);
 
-- 
1.7.9.1

Re: [Patch v2 0/3] Add test and fix exclude bug

[Austin Clements]

Quoth Mark Walters on Mar 14 at 12:26 pm:
> The test in the previous patches
> id:"1331551914-28323-1-git-send-email-markwalters1009@gmail.com"
> triggered the bug accidentally. It accidentally set the exclude tags
> to be "=" and "deleted" rather than just "deleted". The non-existent
> "=" tag (i.e., the tag that does not occur anywhere in the Xapian
> database) triggered the bug. First we correct the tests to just set
> the "deleted" tag, then we add some tests including one with a
> deliberately non-existent tag. Finally, we modify the exclusion code
> to avoid the bug.
> 
> The modification also appears to fix a similar problem for jrollins
> that I have not yet been able to reproduce.
> 
> Since the code is now more clearly correct, cleaner and we do
> definitely avoid a Xapian bug I think these patches can be committed
> even without knowing why it fixed jrollins' bug.
> 
> Finally these apply to current master (not to the --with-excluded series).

LGTM.  It would be nice to have the broken tests marked broken in the
patch that introduces them, but it's fine.

Re: [Patch v2 0/3] Add test and fix exclude bug

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Wed, 14 Mar 2012 12:26:51 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> The test in the previous patches
> id:"1331551914-28323-1-git-send-email-markwalters1009@gmail.com"
> triggered the bug accidentally. It accidentally set the exclude tags
> to be "=" and "deleted" rather than just "deleted". The non-existent
> "=" tag (i.e., the tag that does not occur anywhere in the Xapian
> database) triggered the bug. First we correct the tests to just set
> the "deleted" tag, then we add some tests including one with a
> deliberately non-existent tag. Finally, we modify the exclusion code
> to avoid the bug.
> 
> The modification also appears to fix a similar problem for jrollins
> that I have not yet been able to reproduce.
> 
> Since the code is now more clearly correct, cleaner and we do
> definitely avoid a Xapian bug I think these patches can be committed
> even without knowing why it fixed jrollins' bug.
> 
> Finally these apply to current master (not to the --with-excluded series).

tested, and it works for me, so LGTM.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [Patch v2 0/3] Add test and fix exclude bug

[David Bremner]

On Wed, 14 Mar 2012 12:26:51 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> The test in the previous patches
> id:"1331551914-28323-1-git-send-email-markwalters1009@gmail.com"
> triggered the bug accidentally. It accidentally set the exclude tags
> to be "=" and "deleted" rather than just "deleted". The non-existent
> "=" tag (i.e., the tag that does not occur anywhere in the Xapian
> database) triggered the bug.

pushed,

d