unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH 0/2] A bug in the exclude code
@ 2012-03-12 11:31 Mark Walters
  2012-03-12 11:31 ` [PATCH 1/2] test: add tests for message only search Mark Walters
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Mark Walters @ 2012-03-12 11:31 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] test: add tests for message only search
  2012-03-12 11:31 [PATCH 0/2] A bug in the exclude code Mark Walters
@ 2012-03-12 11:31 ` Mark Walters
  2012-03-12 20:06   ` Jameson Graef Rollins
  2012-03-12 11:31 ` [PATCH 2/2] lib: fix an exclude bug Mark Walters
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Mark Walters @ 2012-03-12 11:31 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] lib: fix an exclude bug
  2012-03-12 11:31 [PATCH 0/2] A bug in the exclude code Mark Walters
  2012-03-12 11:31 ` [PATCH 1/2] test: add tests for message only search Mark Walters
@ 2012-03-12 11:31 ` Mark Walters
  2012-03-12 20:03 ` [PATCH 0/2] A bug in the exclude code Jameson Graef Rollins
  2012-03-14  2:08 ` Austin Clements
  3 siblings, 0 replies; 9+ messages in thread
From: Mark Walters @ 2012-03-12 11:31 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] A bug in the exclude code
  2012-03-12 11:31 [PATCH 0/2] A bug in the exclude code Mark Walters
  2012-03-12 11:31 ` [PATCH 1/2] test: add tests for message only search Mark Walters
  2012-03-12 11:31 ` [PATCH 2/2] lib: fix an exclude bug Mark Walters
@ 2012-03-12 20:03 ` Jameson Graef Rollins
  2012-03-12 21:07   ` Mark Walters
  2012-03-14  2:08 ` Austin Clements
  3 siblings, 1 reply; 9+ messages in thread
From: Jameson Graef Rollins @ 2012-03-12 20:03 UTC (permalink / raw)
  To: Mark Walters, notmuch

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] test: add tests for message only search
  2012-03-12 11:31 ` [PATCH 1/2] test: add tests for message only search Mark Walters
@ 2012-03-12 20:06   ` Jameson Graef Rollins
  0 siblings, 0 replies; 9+ messages in thread
From: Jameson Graef Rollins @ 2012-03-12 20:06 UTC (permalink / raw)
  To: Mark Walters, notmuch

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] A bug in the exclude code
  2012-03-12 20:03 ` [PATCH 0/2] A bug in the exclude code Jameson Graef Rollins
@ 2012-03-12 21:07   ` Mark Walters
  2012-03-13 17:39     ` Jameson Graef Rollins
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Walters @ 2012-03-12 21:07 UTC (permalink / raw)
  To: Jameson Graef Rollins, notmuch


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] A bug in the exclude code
  2012-03-12 21:07   ` Mark Walters
@ 2012-03-13 17:39     ` Jameson Graef Rollins
  0 siblings, 0 replies; 9+ messages in thread
From: Jameson Graef Rollins @ 2012-03-13 17:39 UTC (permalink / raw)
  To: Mark Walters, notmuch

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] A bug in the exclude code
  2012-03-12 11:31 [PATCH 0/2] A bug in the exclude code Mark Walters
                   ` (2 preceding siblings ...)
  2012-03-12 20:03 ` [PATCH 0/2] A bug in the exclude code Jameson Graef Rollins
@ 2012-03-14  2:08 ` Austin Clements
  2012-03-14 11:12   ` Mark Walters
  3 siblings, 1 reply; 9+ messages in thread
From: Austin Clements @ 2012-03-14  2:08 UTC (permalink / raw)
  To: Mark Walters, notmuch

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] A bug in the exclude code
  2012-03-14  2:08 ` Austin Clements
@ 2012-03-14 11:12   ` Mark Walters
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Walters @ 2012-03-14 11:12 UTC (permalink / raw)
  To: Austin Clements, notmuch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-03-14 11:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-12 11:31 [PATCH 0/2] A bug in the exclude code Mark Walters
2012-03-12 11:31 ` [PATCH 1/2] test: add tests for message only search Mark Walters
2012-03-12 20:06   ` Jameson Graef Rollins
2012-03-12 11:31 ` [PATCH 2/2] lib: fix an exclude bug Mark Walters
2012-03-12 20:03 ` [PATCH 0/2] A bug in the exclude code Jameson Graef Rollins
2012-03-12 21:07   ` Mark Walters
2012-03-13 17:39     ` Jameson Graef Rollins
2012-03-14  2:08 ` Austin Clements
2012-03-14 11:12   ` Mark Walters

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).