add_message: Propagate error status from notmuch_message_create_for_message_id

What a great feeling to remove an XXX comment.

database.cc

@@ -284,7 +284,7 @@ notmuch_database_find_message (notmuch_database_t *notmuch,
     if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND)
 	return NULL;
 
-    return _notmuch_message_create (notmuch, notmuch, doc_id);
+    return _notmuch_message_create (notmuch, notmuch, doc_id, NULL);
 }
 
 /* Return one or more thread_ids, (as a GPtrArray of strings), for the
@@ -762,17 +762,12 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 	 * database). */
 
 	/* Use NULL for owner since we want to free this locally. */
-
-	/* XXX: This call can fail by either out-of-memory or an
-	 * "impossible" Xapian exception. We should rewrite it to
-	 * allow us to propagate the error status. */
-	message = _notmuch_message_create_for_message_id (NULL, notmuch,
-							  message_id);
-	if (message == NULL) {
-	    fprintf (stderr, "Internal error. This shouldn't happen.\n\n");
-	    fprintf (stderr, "I mean, it's possible you ran out of memory, but then this code path is still an internal error since it should have detected that and propagated the status value up the stack.\n");
-	    exit (1);
-	}
+	message = _notmuch_message_create_for_message_id (NULL,
+							  notmuch,
+							  message_id,
+							  &ret);
+	if (message == NULL)
+	    goto DONE;
 
 	/* Has a message previously been added with the same ID? */
 	old_filename = notmuch_message_get_filename (message);

message.cc

@@ -80,23 +80,43 @@ _notmuch_message_destructor (notmuch_message_t *message)
  * caller *is* responsible for calling notmuch_message_destroy.
  *
  * If no document exists in the database with document ID of 'doc_id'
- * then this function returns NULL.
+ * then this function returns NULL and sets *status to
+ * NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND.
+ *
+ * This function can also fail to due lack of available memory,
+ * returning NULL and optionally setting *status to
+ * NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY. Caller can pass NULL
+ * for status if uninterested in distinguishing these two cases.
  */
 notmuch_message_t *
 _notmuch_message_create (const void *talloc_owner,
 			 notmuch_database_t *notmuch,
-			 unsigned int doc_id)
+			 unsigned int doc_id,
+			 notmuch_private_status_t *status)
 {
     notmuch_message_t *message;
 
+    if (status)
+	*status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
+
     message = talloc (talloc_owner, notmuch_message_t);
-    if (unlikely (message == NULL))
+    if (unlikely (message == NULL)) {
+	if (status)
+	    *status = NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY;
 	return NULL;
+    }
 
     message->notmuch = notmuch;
     message->doc_id = doc_id;
     message->message_id = NULL; /* lazily created */
     message->filename = NULL; /* lazily created */
+
+    /* This is C++'s creepy "placement new", which is really just an
+     * ugly way to call a constructor for a pre-allocated object. So
+     * it's really not an error to not be checking for OUT_OF_MEMORY
+     * here, since this "new" isn't actually allocating memory. This
+     * is language-design comedy of the wrong kind. */
+
     new (&message->doc) Xapian::Document;
 
     talloc_set_destructor (message, _notmuch_message_destructor);
@@ -105,6 +125,8 @@ _notmuch_message_create (const void *talloc_owner,
 	message->doc = notmuch->xapian_db->get_document (doc_id);
     } catch (const Xapian::DocNotFoundError &error) {
 	talloc_free (message);
+	if (status)
+	    *status = NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND;
 	return NULL;
     }
 
@@ -127,32 +149,61 @@ _notmuch_message_create (const void *talloc_owner,
  * If there is already a document with message ID 'message_id' in the
  * database, then the returned message can be used to query/modify the
  * document. Otherwise, a new document will be inserted into the
- * database before this function returns;
+ * database before this function returns.
+ *
+ * If an error occurs, this function will return NULL and *status
+ * will be set as appropriate. (The status pointer argument must
+ * not be NULL.)
  */
 notmuch_message_t *
 _notmuch_message_create_for_message_id (const void *talloc_owner,
 					notmuch_database_t *notmuch,
-					const char *message_id)
+					const char *message_id,
+					notmuch_status_t *status)
 {
+    notmuch_private_status_t private_status;
     notmuch_message_t *message;
     Xapian::Document doc;
     unsigned int doc_id;
     char *term;
 
+    *status = NOTMUCH_STATUS_SUCCESS;
+
     message = notmuch_database_find_message (notmuch, message_id);
     if (message)
 	return talloc_steal (talloc_owner, message);
 
     term = talloc_asprintf (NULL, "%s%s",
 			    _find_prefix ("id"), message_id);
-    doc.add_term (term);
-    talloc_free (term);
+    if (term == NULL) {
+	*status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	return NULL;
+    }
 
-    doc.add_value (NOTMUCH_VALUE_MESSAGE_ID, message_id);
+    try {
+	doc.add_term (term);
+	talloc_free (term);
 
-    doc_id = notmuch->xapian_db->add_document (doc);
+	doc.add_value (NOTMUCH_VALUE_MESSAGE_ID, message_id);
 
-    return _notmuch_message_create (talloc_owner, notmuch, doc_id);
+	doc_id = notmuch->xapian_db->add_document (doc);
+    } catch (const Xapian::Error &error) {
+	*status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	return NULL;
+    }
+
+    message = _notmuch_message_create (talloc_owner, notmuch,
+				       doc_id, &private_status);
+
+    if (private_status >= (notmuch_private_status_t) NOTMUCH_STATUS_LAST_STATUS)
+    {
+	fprintf (stderr, "Internal error: Failed to find document immediately after adding it.\n");
+	exit (1);
+    }
+
+    *status = (notmuch_status_t) private_status;
+
+    return message;
 }
 
 const char *

notmuch-private.h

@@ -83,6 +83,7 @@ typedef enum {
 typedef enum _notmuch_private_status {
     /* First, copy all the public status values. */
     NOTMUCH_PRIVATE_STATUS_SUCCESS = NOTMUCH_STATUS_SUCCESS,
+    NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY = NOTMUCH_STATUS_OUT_OF_MEMORY,
     NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION = NOTMUCH_STATUS_XAPIAN_EXCEPTION,
     NOTMUCH_PRIVATE_STATUS_FILE_NOT_EMAIL = NOTMUCH_STATUS_FILE_NOT_EMAIL,
     NOTMUCH_PRIVATE_STATUS_NULL_POINTER = NOTMUCH_STATUS_NULL_POINTER,
@@ -100,12 +101,14 @@ typedef enum _notmuch_private_status {
 notmuch_message_t *
 _notmuch_message_create (const void *talloc_owner,
 			 notmuch_database_t *notmuch,
-			 unsigned int doc_id);
+			 unsigned int doc_id,
+			 notmuch_private_status_t *status);
 
 notmuch_message_t *
 _notmuch_message_create_for_message_id (const void *talloc_owner,
 					notmuch_database_t *notmuch,
-					const char *message_id);
+					const char *message_id,
+					notmuch_status_t *status);
 
 /* Lookup a prefix value by name.
  *

notmuch.h

@@ -51,6 +51,8 @@ typedef int notmuch_bool_t;
  *
  * NOTMUCH_STATUS_SUCCESS: No error occurred.
  *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory
+ *
  * XXX: We don't really want to expose this lame XAPIAN_EXCEPTION
  * value. Instead we should map to things like DATABASE_LOCKED or
  * whatever.
@@ -78,6 +80,7 @@ typedef int notmuch_bool_t;
  */
 typedef enum _notmuch_status {
     NOTMUCH_STATUS_SUCCESS = 0,
+    NOTMUCH_STATUS_OUT_OF_MEMORY,
     NOTMUCH_STATUS_XAPIAN_EXCEPTION,
     NOTMUCH_STATUS_FILE_ERROR,
     NOTMUCH_STATUS_FILE_NOT_EMAIL,
@@ -267,8 +270,8 @@ notmuch_database_add_message (notmuch_database_t *database,
  * a new notmuch_message_t object is returned. The caller should call
  * notmuch_message_destroy when done with the message.
  *
- * If no message is found with the given message_id, this function
- * returns NULL.
+ * If no message is found with the given message_id or if an
+ * out-of-memory situation occurs, this function returns NULL.
  */
 notmuch_message_t *
 notmuch_database_find_message (notmuch_database_t *database,
@@ -376,6 +379,9 @@ notmuch_results_has_more (notmuch_results_t *results);
  *
  * See the documentation of notmuch_query_search for example code
  * showing how to iterate over a notmuch_results_t object.
+ *
+ * If an out-of-memory situation occurs, this function will return
+ * NULL.
  */
 notmuch_message_t *
 notmuch_results_get (notmuch_results_t *results);

query.cc

@@ -165,12 +165,24 @@ notmuch_results_has_more (notmuch_results_t *results)
 notmuch_message_t *
 notmuch_results_get (notmuch_results_t *results)
 {
+    notmuch_message_t *message;
     Xapian::docid doc_id;
+    notmuch_private_status_t status;
 
     doc_id = *results->iterator;
 
-    return _notmuch_message_create (results,
-				    results->notmuch, doc_id);
+    message = _notmuch_message_create (results,
+				       results->notmuch, doc_id,
+				       &status);
+
+    if (message == NULL &&
+	status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND)
+    {
+	fprintf (stderr, "Internal error: a results iterator contains a non-existent document ID.\n");
+	exit (1);
+    }
+
+    return message;
 }
 
 void