Anton Khirnov <anton@khirnov.net> writes:

>
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index db49e77d..ebb38a5c 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -714,7 +714,8 @@ _notmuch_thread_create (void *ctx,
>  			notmuch_doc_id_set_t *match_set,
>  			notmuch_string_list_t *excluded_terms,
>  			notmuch_exclude_t omit_exclude,
> -			notmuch_sort_t sort);
> +			notmuch_sort_t sort,
> +			notmuch_private_status_t *status);

In general it results in cleaner code to return a status code and use
the output parameter for a pointer to whatever is being created. On the
other hand the existing codebase is not very consistent about this. This
function is only used in one place, so the usual worry about the amount
of change needed at callsites does not apply.

> +/**
> + * Get the status of the given 'threads' iterator.
> + *
> + * Return value:
> + *
> + * NOTMUCH_STATUS_SUCCESS: The iterator is valid; notmuch_threads_get will
> + *     return a valid object
> + *
> + * NOTMUCH_STATUS_ITERATOR_EXHAUSTED: All items have been read
> + *
> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Iteration failed to allocate memory
> + *
> + * NOTMUCH_STATUS_OPERATION_INVALIDATED: Iteration was invalidated by the
> + *     database. Re-open the database and try again.
> + *
> + * See the documentation of notmuch_query_search_threads for example
> + * code showing how to iterate over a notmuch_threads_t object.
> + */
> +notmuch_status_t
> +notmuch_threads_status (notmuch_threads_t *threads);

I realized that for new API we should use the doxygen style of
e.g. notmuch_database_open_with_config. 

>  
> +
Extra whitespace, not a big deal but may as well clean up.

>  notmuch_thread_t *
>  notmuch_threads_get (notmuch_threads_t *threads)
>  {
> +    notmuch_thread_t *thread;
>      unsigned int doc_id;
> +    notmuch_private_status_t status;
>  
>      if (! notmuch_threads_valid (threads))
>  	return NULL;
>  
>      doc_id = g_array_index (threads->doc_ids, unsigned int,
>  			    threads->doc_id_pos);

> +    thread = _notmuch_thread_create (threads->query,
> +				     threads->query->notmuch,
> +				     doc_id,
> +				     &threads->match_set,
> +				     threads->query->exclude_terms,
> +				     threads->query->omit_excluded,
> +				     threads->query->sort,
> +				     &status);
> +    if (thread == NULL) {
> +	if (! status)
> +	    INTERNAL_ERROR ("NULL thread with no error code");
> +	threads->status = COERCE_STATUS (status, "error creating a thread");
> +    }
> +
> +    return thread;

This bit would be about the same length with my proposal to return the
status, but maybe you could eliminate the INTERNAL_ERROR?

>      notmuch_thread_t *thread = NULL;
> @@ -566,9 +567,9 @@ _notmuch_thread_create (void *ctx,
>      notmuch_message_t *message;
>      notmuch_status_t status;

Here you'd have to check the output threads parameter, and return
NOTMUCH_STATUS_NULL_POINTER if it is NULL.

>  
> -    seed_message = _notmuch_message_create (local, notmuch, seed_doc_id, NULL);
> -    if (! seed_message)
> -	INTERNAL_ERROR ("Thread seed message %u does not exist", seed_doc_id);
> +    seed_message = _notmuch_message_create (local, notmuch, seed_doc_id, status_p);
> +    if (*status_p)
> +	return NULL;

Won't this trigger an INTERNAL_ERROR in the caller? It seems better to return a
sensible error code, and handling that explicitly in the caller.

>      thread_id = notmuch_message_get_thread_id (seed_message);
>      thread_id_query_string = talloc_asprintf (local, "thread:%s", thread_id);
> @@ -659,6 +660,7 @@ _notmuch_thread_create (void *ctx,
>  
>    DONE:
>      talloc_free (local);
> +    *status_p = NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY;
>      return thread;
>  }
>  
> diff --git a/test/T642-database-modified-threads.sh b/test/T642-database-modified-threads.sh
> index 1717cd39..cba6aaa0 100755
> --- a/test/T642-database-modified-threads.sh
> +++ b/test/T642-database-modified-threads.sh
> @@ -11,7 +11,6 @@ add_email_corpus
>  cp -a $NOTMUCH_SRCDIR/test/corpora/lkml ${MAIL_DIR}/
>  
>  test_begin_subtest "handling NOTMUCH_STATUS_OPERATION_INVALIDATED in _notmuch_thread_create"
> -test_subtest_known_broken
>  
>  test_C ${MAIL_DIR} <<EOF
>  #include <notmuch-test.h>
> @@ -27,6 +26,7 @@ main (int argc, char **argv)
>      notmuch_query_t *query_ro, *query_rw;
>      notmuch_status_t status;
>      char* msg = NULL;
> +    unsigned try;
>  
>      EXPECT0 (notmuch_database_open_with_config (argv[1],
>  					        NOTMUCH_DATABASE_MODE_READ_ONLY,
> @@ -59,13 +59,34 @@ main (int argc, char **argv)
>  
>      notmuch_database_close (rw_db);
>  
> -    for (;
> -	 notmuch_threads_valid (threads);
> -	 notmuch_threads_move_to_next (threads)) {
> -	notmuch_thread_t *thread = notmuch_threads_get (threads);
> +    // try iterating over the query up to twice, we expect a Xapian
> +    // DatabaseModifiedError (mapped to NOTMUCH_STATUS_OPERATION_INVALIDATED)
> +    // on the first try
> +    for (try = 0; try < 2; try++) {
> +	for (;
> +	     notmuch_threads_valid (threads);
> +	     notmuch_threads_move_to_next (threads)) {
> +	    notmuch_thread_t *thread = notmuch_threads_get (threads);
> +	}

I think the intent will be clearer if as much of this change as possible
is in the original test, even if it doesn't do anything yet.

A minor point, it would be good to use notmuch_threads_status in the
updated version of the test instead of notmuch_threads_valid.

_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org