From: Jani Nikula <jani@nikula.org>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/4] lib: add versions of	notmuch_query_count_{message,
	threads} with status return
In-Reply-To: <1425717797-19990-2-git-send-email-david@tethera.net>
References: <1419971380-10307-6-git-send-email-david@tethera.net>
	<1425717797-19990-1-git-send-email-david@tethera.net>
	<1425717797-19990-2-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.19+74~g6aeef2e (http://notmuchmail.org) Emacs/24.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 07 Mar 2015 15:57:08 +0200
Message-ID: <87vbicj4gb.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sat, 07 Mar 2015, David Bremner <david@tethera.net> wrote:
> Although I think it's a pretty bad idea to continue using the old API,
> this allows both a more gentle transition for clients of the library,
> and allows us to break one monolithic change into a series

We should probably bump LIBNOTMUCH_MINOR_VERSION for this.

See nitpicks inline below, up to you if you decide if they're worthwhile
changes, generally LGTM.

> ---
>  lib/database.cc |  8 +++++++-
>  lib/notmuch.h   | 34 ++++++++++++++++++++++++++++++----
>  lib/query.cc    | 36 +++++++++++++++++++++++++++++-------
>  3 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3974e2e..d92eec0 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1275,7 +1275,13 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>      if (new_features &
>  	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
>  	notmuch_query_t *query = notmuch_query_create (notmuch, "");
> -	total += notmuch_query_count_messages (query);
> +	unsigned msg_count;

Personal preference, I always want to specify "unsigned int" instead of
just "unsigned". Same all around patches 1 and 2.

> +
> +	status = notmuch_query_count_messages_st (query, &msg_count);
> +	if (status)
> +	    goto DONE;
> +
> +	total += msg_count;
>  	notmuch_query_destroy (query);
>      }
>      if (new_features & NOTMUCH_FEATURE_DIRECTORY_DOCS) {
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 3e326bf..a0fc276 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -914,12 +914,23 @@ notmuch_threads_destroy (notmuch_threads_t *threads);
>   * This function performs a search and returns Xapian's best
>   * guess as to number of matching messages.
>   *
> - * If a Xapian exception occurs, this function may return 0 (after
> - * printing a message).
> + * The version returning the count directly instead of a status value 
> + * is deprecated.
> + *
> + * Return value (of the _st) version:
> + *
> + * NOTMUCH_STATUS_SUCCESS: query complete successfully.
> + *
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured. The
> + *      value of *count is not defined.
>   */

I'd really like to have separate doc comments for both the
functions. The original and now deprecated version can be be a brief
one, with a reference to the _st version documentation. If you move the
_st version declaration here, above the original declaration, the diff
should remain small.

Perhaps reference the library version number when this was added?

> +
>  unsigned
>  notmuch_query_count_messages (notmuch_query_t *query);
>  
> +notmuch_status_t
> +notmuch_query_count_messages_st (notmuch_query_t *query, unsigned *count);
> +
>  /**
>   * Return the number of threads matching a search.
>   *
> @@ -928,13 +939,28 @@ notmuch_query_count_messages (notmuch_query_t *query);
>   * search.
>   *
>   * Note that this is a significantly heavier operation than
> - * notmuch_query_count_messages().
> + * notmuch_query_count_messages{_st}().
> + *
> + * The version returning the count directly instead of a status value 
> + * is deprecated.
>   *
> - * If an error occurs, this function may return 0.
> + * Return values (of the _st version):
> + *
> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Memory allocation failed. The value
> + *      of *count is not defined
> +
> + * NOTMUCH_STATUS_SUCCESS: query complete successfully.
> + *
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured. The
> + *      value of *count is not defined.
>   */

Same as above.

> +
>  unsigned
>  notmuch_query_count_threads (notmuch_query_t *query);
>  
> +notmuch_status_t
> +notmuch_query_count_threads_st (notmuch_query_t *query, unsigned *count);
> +
>  /**
>   * Get the thread ID of 'thread'.
>   *
> diff --git a/lib/query.cc b/lib/query.cc
> index 9279915..883d128 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -541,6 +541,16 @@ notmuch_threads_destroy (notmuch_threads_t *threads)
>  unsigned
>  notmuch_query_count_messages (notmuch_query_t *query)
>  {
> +    notmuch_status_t status;
> +    unsigned count;
> +
> +    status = notmuch_query_count_messages_st (query, &count);
> +    return status ? 0 : count;
> +}
> +
> +notmuch_status_t
> +notmuch_query_count_messages_st (notmuch_query_t *query, unsigned *count_out)
> +{
>      notmuch_database_t *notmuch = query->notmuch;
>      const char *query_string = query->query_string;
>      Xapian::doccount count = 0;
> @@ -595,30 +605,42 @@ notmuch_query_count_messages (notmuch_query_t *query)
>  	fprintf (stderr, "A Xapian exception occurred: %s\n",
>  		 error.get_msg().c_str());
>  	fprintf (stderr, "Query string was: %s\n", query->query_string);
> +	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>      }
>  
> -    return count;
> +    *count_out=count;

Spaces around "="?

> +    return NOTMUCH_STATUS_SUCCESS;
>  }
>  
>  unsigned
>  notmuch_query_count_threads (notmuch_query_t *query)
>  {
> +    notmuch_status_t status;
> +    unsigned count;
> +
> +    status = notmuch_query_count_threads_st (query, &count);
> +    return status ? 0 : count;
> +}
> +    
> +notmuch_status_t
> +notmuch_query_count_threads_st (notmuch_query_t *query, unsigned *count)
> +{
>      notmuch_messages_t *messages;
>      GHashTable *hash;
> -    unsigned int count;
>      notmuch_sort_t sort;
> +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
>  
>      sort = query->sort;
>      query->sort = NOTMUCH_SORT_UNSORTED;
>      messages = notmuch_query_search_messages (query);
>      query->sort = sort;
>      if (messages == NULL)
> -	return 0;
> +	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  
>      hash = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, NULL);
>      if (hash == NULL) {
>  	talloc_free (messages);
> -	return 0;
> +	return NOTMUCH_STATUS_OUT_OF_MEMORY;
>      }
>  
>      while (notmuch_messages_valid (messages)) {
> @@ -627,7 +649,7 @@ notmuch_query_count_threads (notmuch_query_t *query)
>  	char *thread_id_copy = talloc_strdup (messages, thread_id);
>  	if (unlikely (thread_id_copy == NULL)) {
>  	    notmuch_message_destroy (message);
> -	    count = 0;
> +	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
>  	    goto DONE;
>  	}
>  	g_hash_table_insert (hash, thread_id_copy, NULL);
> @@ -635,11 +657,11 @@ notmuch_query_count_threads (notmuch_query_t *query)
>  	notmuch_messages_move_to_next (messages);
>      }
>  
> -    count = g_hash_table_size (hash);
> +    *count = g_hash_table_size (hash);
>  
>    DONE:
>      g_hash_table_unref (hash);
>      talloc_free (messages);
>  
> -    return count;
> +    return ret;
>  }
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch