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

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

Date: Fri, 19 Oct 2012 01:15:31 -0400

To: Peter Wang, notmuch@notmuchmail.org

Cc:

From: 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

Thread: