From: Jani Nikula <jani@nikula.org>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org,
	amdragon@MIT.EDU
Subject: Re: [PATCH v4 07/11] lib: added interface
	notmuch_thread_get_flag_messages
In-Reply-To: <1328204619-25046-7-git-send-email-markwalters1009@gmail.com>
References: <874nv9rv79.fsf@qmul.ac.uk>
	<1328204619-25046-7-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.11+139~g4340989 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Thu, 02 Feb 2012 23:55:33 +0200
Message-ID: <87k444yk6i.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi Mark -

This is my first look at any version of the series; apologies if I'm
clueless about some details... Please find some comments below.

BR,
Jani.


On Thu,  2 Feb 2012 17:43:35 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> The function is
> notmuch_thread_get_flag_messages
> (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags)
> 
> and returns the number of messages with the specified flags on flag_mask.

Is the purpose of this function to get the count of messages that have
certain flags set, certain flags not set, and certain flags don't-care?

At the very least, I think the documentation of the function should be
greatly improved.

I think the name of the function should be notmuch_thread_count_messages
which is like notmuch_query_count_messages, but for messages in threads
(and with some extra restrictions).

> 
> This generalises the existing function
> notmuch_thread_get_total_messages and
> notmuch_thread_get_matched_messages which are retained to maintain
> compatibility.
> ---
>  lib/message.cc |    6 +++---
>  lib/notmuch.h  |   15 +++++++++++++--
>  lib/thread.cc  |   39 ++++++++++++++++++++++++++-------------
>  3 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/message.cc b/lib/message.cc
> index 0075425..d60da83 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -746,7 +746,7 @@ notmuch_bool_t
>  notmuch_message_get_flag (notmuch_message_t *message,
>  			  notmuch_message_flag_t flag)
>  {
> -    return message->flags & (1 << flag);
> +    return message->flags & flag;
>  }
>  
>  void
> @@ -754,9 +754,9 @@ notmuch_message_set_flag (notmuch_message_t *message,
>  			  notmuch_message_flag_t flag, notmuch_bool_t enable)
>  {
>      if (enable)
> -	message->flags |= (1 << flag);
> +	message->flags |= flag;
>      else
> -	message->flags &= ~(1 << flag);
> +	message->flags &= ~flag;
>  }
>  
>  time_t
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index f75afae..c02e7f4 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -654,6 +654,16 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread);
>  int
>  notmuch_thread_get_total_messages (notmuch_thread_t *thread);
>  
> +/* Get the number of messages in 'thread' which have the specified
> + * flags on flag_mask.
> + *
> + * This is a more general interface than
> + * notmuch_thread_get_total_messages or
> + * notmuch_thread_get_matched_messages
> + */
> +int
> +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags);
> +
>  /* Get a notmuch_messages_t iterator for the top-level messages in
>   * 'thread'.
>   *
> @@ -902,8 +912,9 @@ notmuch_message_get_filenames (notmuch_message_t *message);
>  
>  /* Message flags */
>  typedef enum _notmuch_message_flag {
> -    NOTMUCH_MESSAGE_FLAG_MATCH,
> -    NOTMUCH_MESSAGE_FLAG_EXCLUDED
> +    NOTMUCH_MESSAGE_FLAG_MATCH = (1<<0),
> +    NOTMUCH_MESSAGE_FLAG_EXCLUDED = (1<<1),
> +    NOTMUCH_MESSAGE_FLAG_MAX  = (1<<2)

How are these used by the current lib users at the moment? How will they
break with this change?

Please align the assignments. 

>  } notmuch_message_flag_t;
>  
>  /* Get a value of a flag for the email corresponding to 'message'. */
> diff --git a/lib/thread.cc b/lib/thread.cc
> index e976d64..542f7f4 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -37,8 +37,7 @@ struct visible _notmuch_thread {
>  
>      notmuch_message_list_t *message_list;
>      GHashTable *message_hash;
> -    int total_messages;
> -    int matched_messages;
> +    int flag_count_messages[NOTMUCH_MESSAGE_FLAG_MAX];
>      time_t oldest;
>      time_t newest;
>  };
> @@ -226,7 +225,6 @@ _thread_add_message (notmuch_thread_t *thread,
>  
>      _notmuch_message_list_add_message (thread->message_list,
>  				       talloc_steal (thread, message));
> -    thread->total_messages++;
>  
>      g_hash_table_insert (thread->message_hash,
>  			 xstrdup (notmuch_message_get_message_id (message)),
> @@ -319,21 +317,18 @@ _thread_add_matched_message (notmuch_thread_t *thread,
>  
>      date = notmuch_message_get_date (message);
>  
> -    if (date < thread->oldest || ! thread->matched_messages) {
> +    if (date < thread->oldest || ! notmuch_thread_get_matched_messages (thread)) {
>  	thread->oldest = date;
>  	if (sort == NOTMUCH_SORT_OLDEST_FIRST)
>  	    _thread_set_subject_from_message (thread, message);
>      }
>  
> -    if (date > thread->newest || ! thread->matched_messages) {
> +    if (date > thread->newest || ! notmuch_thread_get_matched_messages (thread)) {
>  	thread->newest = date;
>  	if (sort != NOTMUCH_SORT_OLDEST_FIRST)
>  	    _thread_set_subject_from_message (thread, message);
>      }
>  
> -    if (!notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED))
> -	thread->matched_messages++;
> -
>      if (g_hash_table_lookup_extended (thread->message_hash,
>  			    notmuch_message_get_message_id (message), NULL,
>  			    (void **) &hashed_message)) {
> @@ -411,7 +406,7 @@ _notmuch_thread_create (void *ctx,
>      const char *thread_id;
>      char *thread_id_query_string;
>      notmuch_query_t *thread_id_query;
> -
> +    int i;
>      notmuch_messages_t *messages;
>      notmuch_message_t *message;
>  
> @@ -457,8 +452,8 @@ _notmuch_thread_create (void *ctx,
>      thread->message_hash = g_hash_table_new_full (g_str_hash, g_str_equal,
>  						  free, NULL);
>  
> -    thread->total_messages = 0;
> -    thread->matched_messages = 0;
> +    for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++)
> +	thread->flag_count_messages[i] = 0;

memset (thread->flag_count_messages, 0, sizeof(thread->flag_count_messages));

>      thread->oldest = 0;
>      thread->newest = 0;
>  
> @@ -473,6 +468,7 @@ _notmuch_thread_create (void *ctx,
>  	 notmuch_messages_move_to_next (messages))
>      {
>  	unsigned int doc_id;
> +	unsigned int message_flags;
>  
>  	message = notmuch_messages_get (messages);
>  	doc_id = _notmuch_message_get_doc_id (message);
> @@ -485,6 +481,10 @@ _notmuch_thread_create (void *ctx,
>  	    _notmuch_doc_id_set_remove (match_set, doc_id);
>  	    _thread_add_matched_message (thread, message, sort);
>  	}
> +	message_flags =
> +	    notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) |
> +	    notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED);
> +	thread->flag_count_messages[message_flags]++;

The first impression of using a set of flags as index is that there's a
bug. But this is to keep count of messages with certain flag sets rather
than total for each flag, right? I think this needs more comments, more
documentation. Already naming the field flag_set_message_counts or
similar would help greatly.

>  
>  	_notmuch_message_close (message);
>      }
> @@ -511,15 +511,28 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread)
>  }
>  
>  int
> +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags)
> +{
> +    unsigned int i;
> +    int count = 0;
> +    for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++)

ARRAY_SIZE (thread->flag_count_messages)

> +	if ((i & flag_mask) == (flags & flag_mask))
> +	    count += thread->flag_count_messages[i];
> +    return count;
> +}

I wonder if the same could be accomplished by using two flag mask
parameters, include_flag_mask and exclude_flag_mask. I'm thinking of the
usage, would it be easier to use:

notmuch_query_count_messages (thread, NOTMUCH_MESSAGE_FLAG_MATCH, NOTMUCH_MESSAGE_FLAG_EXCLUDED);

to get number of messages that have MATCH but not EXCLUDED? 0 as
include_flag_mask could still be special for "all", and you could use:

notmuch_query_count_messages (thread, 0, NOTMUCH_MESSAGE_FLAG_EXCLUDED);

Note the name change according to my earlier suggestion. It might be
wise to not export the function before the API is chrystal clear if
there is no pressing need to do so.

> +
> +int
>  notmuch_thread_get_total_messages (notmuch_thread_t *thread)
>  {
> -    return thread->total_messages;
> +    return notmuch_thread_get_flag_messages (thread, 0, 0);
>  }
>  
>  int
>  notmuch_thread_get_matched_messages (notmuch_thread_t *thread)
>  {
> -    return thread->matched_messages;
> +    return notmuch_thread_get_flag_messages (thread,
> +					     NOTMUCH_MESSAGE_FLAG_MATCH | NOTMUCH_MESSAGE_FLAG_EXCLUDED,
> +					     NOTMUCH_MESSAGE_FLAG_MATCH);
>  }
>  
>  const char *
> -- 
> 1.7.2.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch