[PATCH 0/2] A bug in the exclude code

[PATCH 0/2] A bug in the exclude code

[Mark Walters]

There is a bug in the exclude code (found by jrollins in the
--with-excluded series) but also present in master.  None of the
current tests were finding it so the first patch adds two tests.

The bug (and test failure) do not appear in all configuations: on my
main test machine (an oldish debian testing 32bit userspace with a
64bit kernel and xapian 1.2.7) all tests pass. On my laptop (a recent
debian testing 64bit userspace and xapian 1.2.8) one of the new tests
fails.

The second patch fixes the behaviour for me but I don't see why it
should make a difference: searches for A and not B should give the
same results as A and not (A and B). It could be a bug in xapian, it
could be that I am not allowed to reuse queries as I do (is query1 =
query1 and query2 allowed?) or it could be some memory use bug on my
part.

Anyway the "fix" is small which should help narrow down the actual
cause.

Best wishes

Mark 

Mark Walters (2):
  test: add tests for message only search
  lib: fix an exclude bug

 lib/query.cc |    5 +++--
 test/search  |   11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
1.7.9.1

[PATCH 1/2] test: add tests for message only search

[Mark Walters]

This adds two tests for --output=messages searches one of which fails
in some configurations.
---
 test/search |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/test/search b/test/search
index 081f60c..276271b 100755
--- a/test/search
+++ b/test/search
@@ -132,13 +132,24 @@ 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 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

Re: [PATCH 1/2] test: add tests for message only search

[Jameson Graef Rollins]

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

On Mon, 12 Mar 2012 11:31:53 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> +test_begin_subtest "Exclude \"deleted\" messages from message search (no-exclude)"
> +output=$(notmuch search --no-exclude --output=messages subject:deleted | notmuch_search_sanitize)

This should be 's/no-exclude/with-excludes/'.  With this change all
tests pass when the subsequent patch is applied.

jamie.

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

[PATCH 2/2] lib: fix an exclude bug

[Mark Walters]

One test using the new exclude code was failing in some
configurations.  This patch makes it work for me. It may be a "fix"
but I do not see why it fixes it.
---
 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 0/2] A bug in the exclude code

[Jameson Graef Rollins]

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

On Mon, 12 Mar 2012 11:31:52 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> There is a bug in the exclude code (found by jrollins in the
> --with-excluded series) but also present in master.  None of the
> current tests were finding it so the first patch adds two tests.

Hey, Mark.  Thanks so much for looking in to this.  It look like this
patch fixes the issue for me too!  Very excited to see this all coming
together.

> The bug (and test failure) do not appear in all configuations: on my
> main test machine (an oldish debian testing 32bit userspace with a
> 64bit kernel and xapian 1.2.7) all tests pass. On my laptop (a recent
> debian testing 64bit userspace and xapian 1.2.8) one of the new tests
> fails.

It was failing for me too, but looking closer at the test I actually
found a bug: you accidentally used the old style --no-exclude option,
instead of the new --with-excludes.  When you fix the call all the tests
pass fine.

> The second patch fixes the behaviour for me but I don't see why it
> should make a difference: searches for A and not B should give the
> same results as A and not (A and B). It could be a bug in xapian, it
> could be that I am not allowed to reuse queries as I do (is query1 =
> query1 and query2 allowed?) or it could be some memory use bug on my
> part.

I can't explain it either, but there's certainly a lot about xapian that
I don't understand.  Maybe one of the xapian gurus will have some ideas
(Olly?  Austin?).

Anyway, thanks again for pushing on all of this, Mark.

jamie.

PS. Not a big deal, but it would have been nice for this patch set to
have been sent in-reply-to the original series it fixes, just to keep
everything together.

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

Re: [PATCH 0/2] A bug in the exclude code

[Mark Walters]

On Mon, 12 Mar 2012 13:03:12 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Mon, 12 Mar 2012 11:31:52 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> > There is a bug in the exclude code (found by jrollins in the
> > --with-excluded series) but also present in master.  None of the
> > current tests were finding it so the first patch adds two tests.
> 
> Hey, Mark.  Thanks so much for looking in to this.  It look like this
> patch fixes the issue for me too!  Very excited to see this all coming
> together.
> 
> > The bug (and test failure) do not appear in all configuations: on my
> > main test machine (an oldish debian testing 32bit userspace with a
> > 64bit kernel and xapian 1.2.7) all tests pass. On my laptop (a recent
> > debian testing 64bit userspace and xapian 1.2.8) one of the new tests
> > fails.
> 
> It was failing for me too, but looking closer at the test I actually
> found a bug: you accidentally used the old style --no-exclude option,
> instead of the new --with-excludes.  When you fix the call all the tests
> pass fine.

That's great.

> > The second patch fixes the behaviour for me but I don't see why it
> > should make a difference: searches for A and not B should give the
> > same results as A and not (A and B). It could be a bug in xapian, it
> > could be that I am not allowed to reuse queries as I do (is query1 =
> > query1 and query2 allowed?) or it could be some memory use bug on my
> > part.
> 
> I can't explain it either, but there's certainly a lot about xapian that
> I don't understand.  Maybe one of the xapian gurus will have some ideas
> (Olly?  Austin?).
> 
> Anyway, thanks again for pushing on all of this, Mark.
> 
> jamie.
> 
> PS. Not a big deal, but it would have been nice for this patch set to
> have been sent in-reply-to the original series it fixes, just to keep
> everything together.

Just to emphasise the bug is already present in current master (just
better hidden because of the defaults). Hence this pair of patches
(unlike the first one I sent privately) are to current master rather
than to the exclude the series (though they apply there to to modulo the
minor change you mention).

Best wishes

Mark

Re: [PATCH 0/2] A bug in the exclude code

[Jameson Graef Rollins]

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

On Mon, 12 Mar 2012 21:07:49 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> Just to emphasise the bug is already present in current master (just
> better hidden because of the defaults). Hence this pair of patches
> (unlike the first one I sent privately) are to current master rather
> than to the exclude the series (though they apply there to to modulo the
> minor change you mention).

Ok, I see.  I think we really need to see the rest of that patch series
applied to master, though, so if you could send a version compatible
with [0], and in-reply-to [0], that would awesome.

Thanks again, Mark.

jamie.

[0] id:"1330779918-28024-1-git-send-email-markwalters1009@gmail.com"

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

Re: [PATCH 0/2] A bug in the exclude code

[Austin Clements]

On Mon, 12 Mar 2012 11:31:52 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> There is a bug in the exclude code (found by jrollins in the
> --with-excluded series) but also present in master.  None of the
> current tests were finding it so the first patch adds two tests.
> 
> The bug (and test failure) do not appear in all configuations: on my
> main test machine (an oldish debian testing 32bit userspace with a
> 64bit kernel and xapian 1.2.7) all tests pass. On my laptop (a recent
> debian testing 64bit userspace and xapian 1.2.8) one of the new tests
> fails.
> 
> The second patch fixes the behaviour for me but I don't see why it
> should make a difference: searches for A and not B should give the
> same results as A and not (A and B). It could be a bug in xapian, it
> could be that I am not allowed to reuse queries as I do (is query1 =
> query1 and query2 allowed?) or it could be some memory use bug on my
> part.
> 
> Anyway the "fix" is small which should help narrow down the actual
> cause.

LGTM.  Even if we don't totally understand the root cause here, this
change is the right thing to do anyway.

I think it's fine to go ahead and push this ahead of the other exclude
updates, though obviously those will need a little rebasing on top of
this.

Re: [PATCH 0/2] A bug in the exclude code

[Mark Walters]

On Tue, 13 Mar 2012 22:08:24 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> On Mon, 12 Mar 2012 11:31:52 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> > There is a bug in the exclude code (found by jrollins in the
> > --with-excluded series) but also present in master.  None of the
> > current tests were finding it so the first patch adds two tests.
> > 
> > The bug (and test failure) do not appear in all configuations: on my
> > main test machine (an oldish debian testing 32bit userspace with a
> > 64bit kernel and xapian 1.2.7) all tests pass. On my laptop (a recent
> > debian testing 64bit userspace and xapian 1.2.8) one of the new tests
> > fails.
> > 
> > The second patch fixes the behaviour for me but I don't see why it
> > should make a difference: searches for A and not B should give the
> > same results as A and not (A and B). It could be a bug in xapian, it
> > could be that I am not allowed to reuse queries as I do (is query1 =
> > query1 and query2 allowed?) or it could be some memory use bug on my
> > part.
> > 
> > Anyway the "fix" is small which should help narrow down the actual
> > cause.
> 
> LGTM.  Even if we don't totally understand the root cause here, this
> change is the right thing to do anyway.
> 
> I think it's fine to go ahead and push this ahead of the other exclude
> updates, though obviously those will need a little rebasing on top of
> this.

Please don't push exactly this one: I will try and send a version with
a better commit message and a test that shows what is failing better
later today. (The functional part will be unchanged.)

Best wishes

Mark