From: Tomi Ollila <tomi.ollila@iki.fi>
To: Peter Wang <novalazy@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 7/8] lib: add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t
In-Reply-To: <20121021130427.GA4820@hili.localdomain>
References: <1340198947-29370-1-git-send-email-novalazy@gmail.com>
	<1340198947-29370-8-git-send-email-novalazy@gmail.com>
	<87txtr6o0c.fsf@betacantrips.com>
	<20121021130427.GA4820@hili.localdomain>
User-Agent: Notmuch/0.14+51~g62cd13b (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-unknown-linux-gnu)
Date: Sun, 21 Oct 2012 15:10:40 +0300
Message-ID: <m24nlo2fgf.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sun, Oct 21 2012, Peter Wang wrote:

> 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.

Well, I personally would count none of these as convincing suggestions ;),
but IMHO the braces are OK here (I don't start judging which I'd like more).

> Peter

Tomi