Tomi Ollila <tomi.ollila at iki.fi> writes: > 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"
I think if we go with the latter then it would make sense to also do id:1368301809-12532-9-git-send-email-markwalters1009 at gmail.com (or something similar) Best wishes Mark > > (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