On Thu, Jun 20 2013, Mark Walters <markwalters1009 at gmail.com> wrote:
> I think we should decide before 0.16 whether to go with this patch and > patch 8/8 or for Peter's suggestion at > id:1368454815-1854-1-git-send-email-novalazy at gmail.com > > Now we have an enum for the NOTMUCH_EXCLUDE possibilities (rather than a > bool) I think it makes sense to implement NOTMUCH_EXCLUDE_FALSE properly > but I am happy with either choice. So, the choice here is to choose between id:"1368454815-1854-1-git-send-email-novalazy at gmail.com" and id:"87bo8edif8.fsf at qmul.ac.uk" (might be hard to catch from this thread -- was easier to take from http://nmbug.tethera.net/status/ ) Both apply cleanly to current master ( d0bd88f ) While Peter's version surely looks simpler (and may work, didn't test atm) the comments Mark presents makes sense (especially the "subtlety" thing ;) IMHO Mark's version makes future development "safer" and therefore my vote (or million of those) goes to id:"87bo8edif8.fsf at qmul.ac.uk" > Best wishes > > Mark Tomi > > > > On Mon, 13 May 2013, Mark Walters <markwalters1009 at gmail.com> wrote: >> Add NOTMUCH_EXCLUDE_FLAG to notmuch_exclude_t so that it can >> cover all four values of search --exclude in the cli. >> >> Previously the way to avoid any message being marked excluded was to >> pass in an empty list of excluded tags: since we now have an explicit >> option we might as well honour it. >> >> The enum is in a slightly strange order as the existing FALSE/TRUE >> options correspond to the new >> NOTMUCH_EXCLUDE_FLAG/NOTMUCH_EXCLUDE_TRUE options so this means we do >> not need to bump the version number. >> >> Indeed, an example of this is that the cli count and show still use >> FALSE/TRUE and still work. >> --- >> >> This is a new version of this single patch. In Peter's version >> NOTMUCH_EXCLUDE_FLAG and NOTMUCH_EXCLUDE_FALSE were treated as synonyms: >> this makes them behave in the obvious way and documents them. >> >> I think the only subtlety is the enum order mentioned in the commit >> message. Additionally it would be good to update cli count and show and, >> at for the latter, it would be good to add an option exclude=all too (so >> excluded messages are completely omitted). Those should both be simple >> but we need to decide whether to allow all four options >> (false/flag/true/all) always or not. Always allowing all 4 is nicely >> consistent but sometimes redundant. Additionally we would need some >> tests. >> >> I think this patch should go in with the main series as I think it is >> worth reserving the NOTMUCH_EXCLUDE_FALSE #define/value so that we don't >> break code in future if we do wish to add it. >> >> Best wishes >> >> Mark >> >> >> >> lib/notmuch.h | 18 ++++++++++++++++-- >> lib/query.cc | 8 +++++--- >> lib/thread.cc | 28 +++++++++++++++------------- >> notmuch-search.c | 2 +- >> 4 files changed, 37 insertions(+), 19 deletions(-) >> >> diff --git a/lib/notmuch.h b/lib/notmuch.h >> index 27b43ff..73c85a4 100644 >> --- a/lib/notmuch.h >> +++ b/lib/notmuch.h >> @@ -500,10 +500,15 @@ typedef enum { >> const char * >> notmuch_query_get_query_string (notmuch_query_t *query); >> >> -/* Exclude values for notmuch_query_set_omit_excluded */ >> +/* Exclude values for notmuch_query_set_omit_excluded. The strange >> + * order is to maintain backward compatibility: the old FALSE/TRUE >> + * options correspond to the new >> + * NOTMUCH_EXCLUDE_FLAG/NOTMUCH_EXCLUDE_TRUE options. >> + */ >> typedef enum { >> - NOTMUCH_EXCLUDE_FALSE, >> + NOTMUCH_EXCLUDE_FLAG, >> NOTMUCH_EXCLUDE_TRUE, >> + NOTMUCH_EXCLUDE_FALSE, >> NOTMUCH_EXCLUDE_ALL >> } notmuch_exclude_t; >> >> @@ -517,6 +522,15 @@ typedef enum { >> * match in at least one non-excluded message. Otherwise, if set to ALL, >> * notmuch_query_search_threads will omit excluded messages from all >> threads. >> * >> + * If set to FALSE or FLAG then both notmuch_query_search_messages and >> + * notmuch_query_search_threads will return all matching >> + * messages/threads regardless of exclude status. If set to FLAG then >> + * the exclude flag will be set for any excluded message that is >> + * returned by notmuch_query_search_messages, and the thread counts >> + * for threads returned by notmuch_query_search_threads will be the >> + * number of non-excluded messages/matches. Otherwise, if set to >> + * FALSE, then the exclude status is completely ignored. >> + * >> * The performance difference when calling >> * notmuch_query_search_messages should be relatively small (and both >> * should be very fast). However, in some cases, >> diff --git a/lib/query.cc b/lib/query.cc >> index 1cc768f..69668a4 100644 >> --- a/lib/query.cc >> +++ b/lib/query.cc >> @@ -218,13 +218,15 @@ notmuch_query_search_messages (notmuch_query_t *query) >> } >> messages->base.excluded_doc_ids = NULL; >> >> - if (query->exclude_terms) { >> + if ((query->omit_excluded != NOTMUCH_EXCLUDE_FALSE) && >> (query->exclude_terms)) { >> exclude_query = _notmuch_exclude_tags (query, final_query); >> >> - 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 { /* NOTMUCH_EXCLUDE_FLAG */ >> exclude_query = Xapian::Query (Xapian::Query::OP_AND, >> exclude_query, final_query); >> >> diff --git a/lib/thread.cc b/lib/thread.cc >> index bc07877..4dcf705 100644 >> --- a/lib/thread.cc >> +++ b/lib/thread.cc >> @@ -238,20 +238,22 @@ _thread_add_message (notmuch_thread_t *thread, >> char *clean_author; >> notmuch_bool_t message_excluded = FALSE; >> >> - for (tags = notmuch_message_get_tags (message); >> - notmuch_tags_valid (tags); >> - notmuch_tags_move_to_next (tags)) >> - { >> - tag = notmuch_tags_get (tags); >> - /* Is message excluded? */ >> - for (notmuch_string_node_t *term = exclude_terms->head; >> - term != NULL; >> - term = term->next) >> + if (omit_exclude != NOTMUCH_EXCLUDE_FALSE) { >> + for (tags = notmuch_message_get_tags (message); >> + notmuch_tags_valid (tags); >> + notmuch_tags_move_to_next (tags)) >> { >> - /* We ignore initial 'K'. */ >> - if (strcmp(tag, (term->string + 1)) == 0) { >> - message_excluded = TRUE; >> - break; >> + tag = notmuch_tags_get (tags); >> + /* Is message excluded? */ >> + for (notmuch_string_node_t *term = exclude_terms->head; >> + term != NULL; >> + term = term->next) >> + { >> + /* We ignore initial 'K'. */ >> + if (strcmp(tag, (term->string + 1)) == 0) { >> + message_excluded = TRUE; >> + break; >> + } >> } >> } >> } >> diff --git a/notmuch-search.c b/notmuch-search.c >> index 4323201..a20791a 100644 >> --- a/notmuch-search.c >> +++ b/notmuch-search.c >> @@ -411,7 +411,7 @@ notmuch_search_command (notmuch_config_t *config, int >> argc, char *argv[]) >> for (i = 0; i < search_exclude_tags_length; i++) >> notmuch_query_add_tag_exclude (query, search_exclude_tags[i]); >> if (exclude == EXCLUDE_FLAG) >> - notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FALSE); >> + notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_FLAG); >> if (exclude == EXCLUDE_ALL) >> notmuch_query_set_omit_excluded (query, NOTMUCH_EXCLUDE_ALL); >> } >> -- >> 1.7.9.1 > _______________________________________________ > notmuch mailing list > notmuch at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch