Anton Khirnov <anton@khirnov.net> writes:

>  	if (status)
>  	    *status = NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND;
>  	return NULL;
> +    } catch (const Xapian::DatabaseModifiedError &error) {
> +	if (status)
> +	    *status = NOTMUCH_PRIVATE_STATUS_OPERATION_INVALIDATED;
> +	return NULL;

I wonder if this status code (name) should refer directly to iterators.
Do you envision using it in non-iterator contexts?

> +    if (! messages->is_of_list_type)
> +	valid = _notmuch_mset_messages_valid (messages);
> +    else
> +	valid = messages->iterator != NULL;
> +
> +    return valid ?
> +	NOTMUCH_STATUS_SUCCESS : NOTMUCH_STATUS_ITERATOR_EXHAUSTED;

Is this better than two returns just above?

>  notmuch_bool_t
>  notmuch_messages_valid (notmuch_messages_t *messages)
>  {
[snip]
> +    return notmuch_messages_status(messages) == NOTMUCH_STATUS_SUCCESS;
>  }

I think it would be better to gradually migrate existing code to the new
function, since this code is eminently inlinable. That probably means
deprecating notmuch_messges_valid

>  bool
> @@ -142,7 +157,7 @@ notmuch_messages_move_to_next (notmuch_messages_t *messages)
>  	return;
>      }
>  
> -    if (messages->iterator == NULL)
> +    if (! notmuch_messages_valid(messages))
>  	return;
>  
>      messages->iterator = messages->iterator->next;
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index 367e23e6..f4702dd8 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h

>  notmuch_message_list_t *
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 937fa24e..c9923da2 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -232,6 +232,20 @@ typedef enum {
> +    /**
> +     * The iterator being examined has been exhausted and contains no more
> +     * items.
> +     */
> +    NOTMUCH_STATUS_ITERATOR_EXHAUSTED,
> +    /**
> +     * An operation that was being performed on the database has been
> +     * invalidated while in progress, and must be re-executed.
> +     *
> +     * This will typically happen while iterating over query results and the
> +     * underlying Xapian database is modified by another process so that the
> +     * currently open version cannot be read anymore.
> +     */
> +    NOTMUCH_STATUS_OPERATION_INVALIDATED,

Note that you should update notmuch_status_to_string when adding new
status values

> @@ -1516,10 +1530,35 @@ notmuch_thread_destroy (notmuch_thread_t *thread);
>   *
>   * See the documentation of notmuch_query_search_messages for example

>   * code showing how to iterate over a notmuch_messages_t object.
> + *
> + * Note that an iterator may become invalid either due to getting exhausted or
> + * due to a runtime error. Use notmuch_messages_status to distinguish
> + * between those cases.
>   */

I guess we need to decide if we really expect people to use both
functions in new code.

>  notmuch_bool_t
>  notmuch_messages_valid (notmuch_messages_t *messages);
>  
> +/**
> + *
> + * See the documentation of notmuch_query_search_messages for example
> + * code showing how to iterate over a notmuch_messages_t object.

It would be good if the referred to code actually used
notmuch_messages_status, to encourage good error handling. The idiom "!
status" as success is pretty entrenched, so I think
notmuch_messages_valid is not needed there.

 query = notmuch_query_create (database, query_string);

 if (notmuch_query_search_messages (query, &messages) != NOTMUCH_STATUS_SUCCESS)
     return EXIT_FAILURE;

 for (;
      ! notmuch_messages_status (messages);
      notmuch_messages_move_to_next (messages))
 {
     message = notmuch_messages_get (messages);
     ....
     notmuch_message_destroy (message);
 }

 if (notmuch_messages_status (messages) != NOTMUCH_STATUS_ITERATOR_EXHAUSTED)
     return EXIT_FAILURE;

 notmuch_query_destroy (query);


> +    if (message == NULL) {
> +	if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
> +	    INTERNAL_ERROR ("a messages iterator contains a non-existent
> document ID.\n");

As you know (because you sent this patch series!) we really want to
avoid the library aborting. Are we confident that this cannot arise from
concurrent access (I guess via Xapian snapshotting mechanisms?). Perhaps
a brief comment about _why_ this should never happen would be a good idea.

>      }
> +    status = notmuch_messages_status(messages_ro);
>  
> -    printf("SUCCESS\n");
> +    if (status == NOTMUCH_STATUS_ITERATOR_EXHAUSTED)
> +	printf("SUCCESS\n");
> +    else
> +	printf("status: %u\n", status);

It might be my mistake in rebasing, but this test fails for me with
"status: 26". Assuming you update notmuch_status_to_string, that can be
made less cryptic, and I'll double check

> +test_subtest_known_broken

It should not in fact be known broken after this commit, right?

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