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

[Tomi Ollila <tomi.ollila@iki.fi>]

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