Re: [PATCH 3/3] lib: add notmuch_threads_status()

Subject: Re: [PATCH 3/3] lib: add notmuch_threads_status()

Date: Mon, 28 Jul 2025 16:26:45 +0200

To: notmuch@notmuchmail.org, David Bremner

Cc:

From: Anton Khirnov

Quoting David Bremner (2025-07-27 17:10:59)
> > +    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?

I am dropping it in the updated patch version, but there's no hiding
from the fact that only
* status == SUCCESS && thread != NULL
* status != SUCCESS && thread == NULL
are valid program states, whether we enforce it in the code explicitly
or not.

> >      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.

What's the point, when it's an internal function? Seems to me that it's
better to segfault when the calling code is so plainly broken, as that
is easier to debug.

> > -    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.

The code was actually returned, as status_p was a called-provided
pointer. But I've changed the signature as you requested, so now it
returns the status directly and the thread through a pointer.

> > @@ -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.

Right, both done in the new patch version.

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

Thread: