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