Date: Sun, 21 Oct 2012 13:04:27 +1100
Message-ID: <20121021130427.GA4820@hili.localdomain>
From: Peter Wang <novalazy@gmail.com>
To: notmuch@notmuchmail.org
Subject: Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
In-Reply-To: <87txtr6o0c.fsf@betacantrips.com>
References: <1340198947-29370-1-git-send-email-novalazy@gmail.com>
	<1340198947-29370-8-git-send-email-novalazy@gmail.com>
	<87txtr6o0c.fsf@betacantrips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Precedence: list

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