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

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

On Thu, Jun 20 2013, Mark Walters <markwalters1009@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@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@gmail.com"
and
id:"87bo8edif8.fsf@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@qmul.ac.uk"

> Best wishes
>
> Mark

Tomi


>
>
>
> On Mon, 13 May 2013, Mark Walters <markwalters1009@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@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch