add status value to _notmuch_message_ensure_metadata

add status value to _notmuch_message_ensure_metadata

[David Bremner]

In id:1487339566.mz8acpov1j.astroid@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.

This patch series catches any Xapian exceptions in
_notmuch_message_ensure_metadata and converts them into status returns
(this could be refined in the future to different codes if someone(TM)
finds the time).  The rest of the series is either trivial cleanup, or
propagating that status through the API. In particular the following
changes to the API docs are included.

Some of the NULL returns were there already, just not
documented. However message_get_message_id is a genuine change.

The change to message_get_flag is a bit annoying, the new API is
notably less friendly, and the proposed solution for the CLI is the
wrap_message_get_flag function. Neither being inline nor the name is
crucial.  Perhaps we should start a file of convenenience functions
that succeed or exit in the CLI.

I haven't tested against Gaute's test case (needs more boost than I
have handy).

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 16da8be9..ac588922 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1286,9 +1286,7 @@ notmuch_messages_collect_tags (notmuch_messages_t *messages);
  * message is valid, (which is until the query from which it derived
  * is destroyed).
  *
- * This function will not return NULL since Notmuch ensures that every
- * message has a unique message ID, (Notmuch will generate an ID for a
- * message if the original file does not contain one).
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_message_id (notmuch_message_t *message);
@@ -1302,8 +1300,7 @@ notmuch_message_get_message_id (notmuch_message_t *message);
  * notmuch_message_destroy on 'message' or until a query from which it
  * derived is destroyed).
  *
- * This function will not return NULL since Notmuch ensures that every
- * message belongs to a single thread.
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_thread_id (notmuch_message_t *message);
@@ -1344,6 +1341,8 @@ notmuch_message_get_replies (notmuch_message_t *message);
  * this function will arbitrarily return a single one of those
  * filenames. See notmuch_message_get_filenames for returning the
  * complete list of filenames.
+ *
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_filename (notmuch_message_t *message);
@@ -1357,6 +1356,8 @@ notmuch_message_get_filename (notmuch_message_t *message);
  *
  * Each filename in the iterator is an absolute filename, (the initial
  * component will match notmuch_database_get_path() ).
+ *
+ * The function returns NULL on error.
  */
 notmuch_filenames_t *
 notmuch_message_get_filenames (notmuch_message_t *message);
@@ -1378,10 +1379,16 @@ typedef enum _notmuch_message_flag {
 
 /**
  * Get a value of a flag for the email corresponding to 'message'.
+ * @returns
+ * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: an error occured reading message metadata from disk
+ * - NOTMUCH_STATUS_NULL_POINTER: Neither *key* nor *value* may be NULL.
+ * - NOTMUCH_STATUS_SUCCESS: No error occured.
+ * @since libnotmuch 5 (notmuch 0.24)
  */
-notmuch_bool_t
+notmuch_status_t
 notmuch_message_get_flag (notmuch_message_t *message,
-			  notmuch_message_flag_t flag);
+			  notmuch_message_flag_t flag,
+			  notmuch_bool_t *value);
 
 /**
  * Set a value of a flag for the email corresponding to 'message'.
@@ -1449,6 +1456,8 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header);
  * notmuch_tags_t object. (For consistency, we do provide a
  * notmuch_tags_destroy function, but there's no good reason to call
  * it if the message is about to be destroyed).
+ *
+ * The function returns NULL on error.
  */
 notmuch_tags_t *
 notmuch_message_get_tags (notmuch_message_t *message);
@@ -1659,6 +1668,12 @@ void
 notmuch_message_destroy (notmuch_message_t *message);
 
 /**
+ * Return the notmuch database of this message.
+ */
+notmuch_database_t *
+notmuch_message_get_database (notmuch_message_t *message);
+
+/**
  * @name Message Properties
  *
  * This interface provides the ability to attach arbitrary (key,value)
@@ -1750,7 +1765,7 @@ typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
  *     notmuch_message_properties_t *list;
  *
  *     for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
- *          notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
+ *          list && notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
  *        printf("%s\n", notmuch_message_properties_value(list));
  *     }
  *
@@ -1758,9 +1773,11 @@ typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
  *
  * Note that there's no explicit destructor needed for the
  * notmuch_message_properties_t object. (For consistency, we do
- * provide a notmuch_message_properities_destroy function, but there's
+ * provide a notmuch_message_properties_destroy function, but there's
  * no good reason to call it if the message is about to be destroyed).
  *
+ * @return The function returns NULL on error
+ *
  * @since libnotmuch 4.4 (notmuch 0.23)
  */
 notmuch_message_properties_t *

[PATCH 1/8] lib: make _notmuch_message_ensure_metadata static

[David Bremner]

It's not called anywhere outside message.cc.
---
 lib/message.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/message.cc b/lib/message.cc
index 9d3e8071..9455b11d 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -310,7 +310,7 @@ _notmuch_message_get_term (notmuch_message_t *message,
     return value;
 }
 
-void
+static void
 _notmuch_message_ensure_metadata (notmuch_message_t *message)
 {
     Xapian::TermIterator i, end;
-- 
2.11.0

[PATCH 2/8] lib: add status return to _notmuch_message_ensure_metadata

[David Bremner]

This is currently silently ignored everywhere, but that's not much
worse than crashing with an uncaught (and maybe uncatchable)
exception.
---
 lib/message.cc | 135 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 72 insertions(+), 63 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 9455b11d..916dd441 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -310,7 +310,7 @@ _notmuch_message_get_term (notmuch_message_t *message,
     return value;
 }
 
-static void
+static notmuch_private_status_t
 _notmuch_message_ensure_metadata (notmuch_message_t *message)
 {
     Xapian::TermIterator i, end;
@@ -328,72 +328,81 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
      * one field of the message object is actually used, it's a huge
      * win as more fields are used. */
 
-    i = message->doc.termlist_begin ();
-    end = message->doc.termlist_end ();
-
-    /* Get thread */
-    if (!message->thread_id)
-	message->thread_id =
-	    _notmuch_message_get_term (message, i, end, thread_prefix);
-
-    /* Get tags */
-    assert (strcmp (thread_prefix, tag_prefix) < 0);
-    if (!message->tag_list) {
-	message->tag_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     tag_prefix);
-	_notmuch_string_list_sort (message->tag_list);
-    }
+    try {
+	i = message->doc.termlist_begin ();
+	end = message->doc.termlist_end ();
+
+	/* Get thread */
+	if (!message->thread_id)
+	    message->thread_id =
+		_notmuch_message_get_term (message, i, end, thread_prefix);
+
+	/* Get tags */
+	assert (strcmp (thread_prefix, tag_prefix) < 0);
+	if (!message->tag_list) {
+	    message->tag_list =
+		_notmuch_database_get_terms_with_prefix (message, i, end,
+							 tag_prefix);
+	    _notmuch_string_list_sort (message->tag_list);
+	}
 
-    /* Get id */
-    assert (strcmp (tag_prefix, id_prefix) < 0);
-    if (!message->message_id)
-	message->message_id =
-	    _notmuch_message_get_term (message, i, end, id_prefix);
-
-    /* Get document type */
-    assert (strcmp (id_prefix, type_prefix) < 0);
-    if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
-	i.skip_to (type_prefix);
-	/* "T" is the prefix "type" fields.  See
+	/* Get id */
+	assert (strcmp (tag_prefix, id_prefix) < 0);
+	if (!message->message_id)
+	    message->message_id =
+		_notmuch_message_get_term (message, i, end, id_prefix);
+
+	/* Get document type */
+	assert (strcmp (id_prefix, type_prefix) < 0);
+	if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
+	    i.skip_to (type_prefix);
+	    /* "T" is the prefix "type" fields.  See
 	 * BOOLEAN_PREFIX_INTERNAL. */
-	if (*i == "Tmail")
-	    NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-	else if (*i == "Tghost")
-	    NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-	else
-	    INTERNAL_ERROR ("Message without type term");
-	NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-    }
-
-    /* Get filename list.  Here we get only the terms.  We lazily
-     * expand them to full file names when needed in
-     * _notmuch_message_ensure_filename_list. */
-    assert (strcmp (type_prefix, filename_prefix) < 0);
-    if (!message->filename_term_list && !message->filename_list)
-	message->filename_term_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     filename_prefix);
-
-
-    /* Get property terms. Mimic the setup with filenames above */
-    assert (strcmp (filename_prefix, property_prefix) < 0);
-    if (!message->property_map && !message->property_term_list)
-	message->property_term_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     property_prefix);
-
-    /* Get reply to */
-    assert (strcmp (property_prefix, replyto_prefix) < 0);
-    if (!message->in_reply_to)
-	message->in_reply_to =
-	    _notmuch_message_get_term (message, i, end, replyto_prefix);
+	    if (*i == "Tmail")
+		NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+	    else if (*i == "Tghost")
+		NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+	    else
+		INTERNAL_ERROR ("Message without type term");
+	    NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+	}
 
+	/* Get filename list.  Here we get only the terms.  We lazily
+	 * expand them to full file names when needed in
+	 * _notmuch_message_ensure_filename_list. */
+	assert (strcmp (type_prefix, filename_prefix) < 0);
+	if (!message->filename_term_list && !message->filename_list)
+	    message->filename_term_list =
+		_notmuch_database_get_terms_with_prefix (message, i, end,
+							 filename_prefix);
+
+
+	/* Get property terms. Mimic the setup with filenames above */
+	assert (strcmp (filename_prefix, property_prefix) < 0);
+	if (!message->property_map && !message->property_term_list)
+	    message->property_term_list =
+		_notmuch_database_get_terms_with_prefix (message, i, end,
+							 property_prefix);
+
+	/* Get reply to */
+	assert (strcmp (property_prefix, replyto_prefix) < 0);
+	if (!message->in_reply_to)
+	    message->in_reply_to =
+		_notmuch_message_get_term (message, i, end, replyto_prefix);
+
+
+	/* It's perfectly valid for a message to have no In-Reply-To
+	 * header. For these cases, we return an empty string. */
+	if (!message->in_reply_to)
+	    message->in_reply_to = talloc_strdup (message, "");
+    } catch (const Xapian::Error &error) {
+	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred fetching message metadata\n",
+		 error.get_msg().c_str());
+	message->notmuch->exception_reported = TRUE;
+	return NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
+    }
 
-    /* It's perfectly valid for a message to have no In-Reply-To
-     * header. For these cases, we return an empty string. */
-    if (!message->in_reply_to)
-	message->in_reply_to = talloc_strdup (message, "");
+    return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
 void
-- 
2.11.0

[PATCH 3/8] lib: propagate error return from some calls to _n_m_e_metadata

[David Bremner]

In this commit we take care of the easy cases, where _n_m_e_metadata
is called from a non-void function. Three tricker cases are left for
future commits.
---
 lib/message.cc | 15 +++++++++++----
 lib/notmuch.h  |  9 ++++-----
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 916dd441..16ea980c 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -458,7 +458,9 @@ const char *
 notmuch_message_get_message_id (notmuch_message_t *message)
 {
     if (!message->message_id)
-	_notmuch_message_ensure_metadata (message);
+	if (_notmuch_message_ensure_metadata (message))
+	    return NULL;
+
     if (!message->message_id)
 	INTERNAL_ERROR ("Message with document ID of %u has no message ID.\n",
 			message->doc_id);
@@ -534,7 +536,9 @@ const char *
 _notmuch_message_get_in_reply_to (notmuch_message_t *message)
 {
     if (!message->in_reply_to)
-	_notmuch_message_ensure_metadata (message);
+	if (_notmuch_message_ensure_metadata (message))
+	    return NULL;
+
     return message->in_reply_to;
 }
 
@@ -542,7 +546,9 @@ const char *
 notmuch_message_get_thread_id (notmuch_message_t *message)
 {
     if (!message->thread_id)
-	_notmuch_message_ensure_metadata (message);
+	if (_notmuch_message_ensure_metadata (message))
+	    return NULL;
+
     if (!message->thread_id)
 	INTERNAL_ERROR ("Message with document ID of %u has no thread ID.\n",
 			message->doc_id);
@@ -982,7 +988,8 @@ notmuch_message_get_tags (notmuch_message_t *message)
     notmuch_tags_t *tags;
 
     if (!message->tag_list)
-	_notmuch_message_ensure_metadata (message);
+	if (_notmuch_message_ensure_metadata (message))
+	    return NULL;
 
     tags = _notmuch_tags_create (message, message->tag_list);
     /* _notmuch_tags_create steals the reference to the tag_list, but
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 16da8be9..62866e5a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1286,9 +1286,7 @@ notmuch_messages_collect_tags (notmuch_messages_t *messages);
  * message is valid, (which is until the query from which it derived
  * is destroyed).
  *
- * This function will not return NULL since Notmuch ensures that every
- * message has a unique message ID, (Notmuch will generate an ID for a
- * message if the original file does not contain one).
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_message_id (notmuch_message_t *message);
@@ -1302,8 +1300,7 @@ notmuch_message_get_message_id (notmuch_message_t *message);
  * notmuch_message_destroy on 'message' or until a query from which it
  * derived is destroyed).
  *
- * This function will not return NULL since Notmuch ensures that every
- * message belongs to a single thread.
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_thread_id (notmuch_message_t *message);
@@ -1449,6 +1446,8 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header);
  * notmuch_tags_t object. (For consistency, we do provide a
  * notmuch_tags_destroy function, but there's no good reason to call
  * it if the message is about to be destroyed).
+ *
+ * The function returns NULL on error.
  */
 notmuch_tags_t *
 notmuch_message_get_tags (notmuch_message_t *message);
-- 
2.11.0

[PATCH 4/8] lib: push error from nme_metadata through nme_filename_list

[David Bremner]

The NULL returns documented here are not actually API changing, they
were just previously undocumented.
---
 lib/message.cc | 20 +++++++++++++-------
 lib/notmuch.h  |  4 ++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 16ea980c..f55dd9fa 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -843,16 +843,19 @@ _notmuch_message_clear_data (notmuch_message_t *message)
     message->modified = TRUE;
 }
 
-static void
+static notmuch_private_status_t
 _notmuch_message_ensure_filename_list (notmuch_message_t *message)
 {
     notmuch_string_node_t *node;
 
     if (message->filename_list)
-	return;
+	return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 
-    if (!message->filename_term_list)
-	_notmuch_message_ensure_metadata (message);
+    if (!message->filename_term_list) {
+	notmuch_private_status_t status = _notmuch_message_ensure_metadata (message);
+	if (status)
+	    return status;
+    }
 
     message->filename_list = _notmuch_string_list_create (message);
     node = message->filename_term_list->head;
@@ -873,7 +876,7 @@ _notmuch_message_ensure_filename_list (notmuch_message_t *message)
 
 	_notmuch_string_list_append (message->filename_list, data);
 
-	return;
+	return NOTMUCH_PRIVATE_STATUS_SUCCESS;
     }
 
     for (; node; node = node->next) {
@@ -913,12 +916,14 @@ _notmuch_message_ensure_filename_list (notmuch_message_t *message)
 
     talloc_free (message->filename_term_list);
     message->filename_term_list = NULL;
+    return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
 const char *
 notmuch_message_get_filename (notmuch_message_t *message)
 {
-    _notmuch_message_ensure_filename_list (message);
+    if (_notmuch_message_ensure_filename_list (message))
+	return NULL;
 
     if (message->filename_list == NULL)
 	return NULL;
@@ -935,7 +940,8 @@ notmuch_message_get_filename (notmuch_message_t *message)
 notmuch_filenames_t *
 notmuch_message_get_filenames (notmuch_message_t *message)
 {
-    _notmuch_message_ensure_filename_list (message);
+    if (_notmuch_message_ensure_filename_list (message))
+	return NULL;
 
     return _notmuch_filenames_create (message, message->filename_list);
 }
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 62866e5a..4e25958c 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1341,6 +1341,8 @@ notmuch_message_get_replies (notmuch_message_t *message);
  * this function will arbitrarily return a single one of those
  * filenames. See notmuch_message_get_filenames for returning the
  * complete list of filenames.
+ *
+ * The function returns NULL on error.
  */
 const char *
 notmuch_message_get_filename (notmuch_message_t *message);
@@ -1354,6 +1356,8 @@ notmuch_message_get_filename (notmuch_message_t *message);
  *
  * Each filename in the iterator is an absolute filename, (the initial
  * component will match notmuch_database_get_path() ).
+ *
+ * The function returns NULL on error.
  */
 notmuch_filenames_t *
 notmuch_message_get_filenames (notmuch_message_t *message);
-- 
2.11.0

[PATCH 5/8] lib: make _notmuch_message_ensure_property_map static

[David Bremner]

It's not called outside message.cc
---
 lib/message.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/message.cc b/lib/message.cc
index f55dd9fa..80fbd5ab 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1823,7 +1823,7 @@ _notmuch_message_database (notmuch_message_t *message)
     return message->notmuch;
 }
 
-void
+static void
 _notmuch_message_ensure_property_map (notmuch_message_t *message)
 {
     notmuch_string_node_t *node;
-- 
2.11.0

[PATCH 6/8] lib: propagate errors from nme_metadata through properties API

[David Bremner]

The NULL return here is not API breaking, since the NULL from
_string_map_create could previously be returned.
---
 lib/message-private.h   |  4 ++--
 lib/message-property.cc | 17 +++++++++++++++--
 lib/message.cc          | 28 ++++++++++++++++++++--------
 lib/notmuch.h           |  6 ++++--
 4 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/lib/message-private.h b/lib/message-private.h
index 74199256..2cfc95a7 100644
--- a/lib/message-private.h
+++ b/lib/message-private.h
@@ -1,8 +1,8 @@
 #ifndef MESSAGE_PRIVATE_H
 #define MESSAGE_PRIVATE_H
 
-notmuch_string_map_t *
-_notmuch_message_property_map (notmuch_message_t *message);
+notmuch_private_status_t
+_notmuch_message_property_map (notmuch_message_t *message, notmuch_string_map_t **map);
 
 notmuch_bool_t
 _notmuch_message_frozen (notmuch_message_t *message);
diff --git a/lib/message-property.cc b/lib/message-property.cc
index 0b13cac3..e81843d3 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -28,10 +28,18 @@
 notmuch_status_t
 notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
 {
+    notmuch_private_status_t private_status;
+    notmuch_string_map_t *map;
+
     if (! value)
 	return NOTMUCH_STATUS_NULL_POINTER;
 
-    *value = _notmuch_string_map_get (_notmuch_message_property_map (message), key);
+    private_status = _notmuch_message_property_map (message, &map);
+    if (private_status)
+	return COERCE_STATUS (private_status,
+			      "Unhandled error reading message property");
+
+    *value = _notmuch_string_map_get (map, key);
 
     return NOTMUCH_STATUS_SUCCESS;
 }
@@ -110,8 +118,13 @@ notmuch_message_remove_all_properties (notmuch_message_t *message, const char *k
 notmuch_message_properties_t *
 notmuch_message_get_properties (notmuch_message_t *message, const char *key, notmuch_bool_t exact)
 {
+    notmuch_private_status_t private_status;
     notmuch_string_map_t *map;
-    map = _notmuch_message_property_map (message);
+
+    private_status = _notmuch_message_property_map (message, &map);
+    if (private_status)
+	return NULL;
+
     return _notmuch_string_map_iterator_create (map, key, exact);
 }
 
diff --git a/lib/message.cc b/lib/message.cc
index 80fbd5ab..9ba24e8c 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1823,16 +1823,19 @@ _notmuch_message_database (notmuch_message_t *message)
     return message->notmuch;
 }
 
-static void
+static notmuch_private_status_t
 _notmuch_message_ensure_property_map (notmuch_message_t *message)
 {
     notmuch_string_node_t *node;
 
     if (message->property_map)
-	return;
+	return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 
-    if (!message->property_term_list)
-	_notmuch_message_ensure_metadata (message);
+    if (!message->property_term_list) {
+	notmuch_private_status_t status = _notmuch_message_ensure_metadata (message);
+	if (status)
+	    return status;
+    }
 
     message->property_map = _notmuch_string_map_create (message);
 
@@ -1854,14 +1857,23 @@ _notmuch_message_ensure_property_map (notmuch_message_t *message)
 
     talloc_free (message->property_term_list);
     message->property_term_list = NULL;
+    return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
-notmuch_string_map_t *
-_notmuch_message_property_map (notmuch_message_t *message)
+notmuch_private_status_t
+_notmuch_message_property_map (notmuch_message_t *message, notmuch_string_map_t **map)
 {
-    _notmuch_message_ensure_property_map (message);
+    notmuch_private_status_t status;
+
+    if (!map)
+	return NOTMUCH_PRIVATE_STATUS_NULL_POINTER;
 
-    return message->property_map;
+    status = _notmuch_message_ensure_property_map (message);
+    if (status)
+	return status;
+
+    *map = message->property_map;
+    return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
 notmuch_bool_t
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 4e25958c..2e8ccb05 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1753,7 +1753,7 @@ typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
  *     notmuch_message_properties_t *list;
  *
  *     for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
- *          notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
+ *          list && notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
  *        printf("%s\n", notmuch_message_properties_value(list));
  *     }
  *
@@ -1761,9 +1761,11 @@ typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
  *
  * Note that there's no explicit destructor needed for the
  * notmuch_message_properties_t object. (For consistency, we do
- * provide a notmuch_message_properities_destroy function, but there's
+ * provide a notmuch_message_properties_destroy function, but there's
  * no good reason to call it if the message is about to be destroyed).
  *
+ * @return The function returns NULL on error
+ *
  * @since libnotmuch 4.4 (notmuch 0.23)
  */
 notmuch_message_properties_t *
-- 
2.11.0

[PATCH 7/8] lib: add notmuch_message_get_database to public API

[David Bremner]

This is to facilitate error handling, specifically retrieval of log
messages from the notmuch database struct.
---
 lib/index.cc            |  6 +++---
 lib/message-property.cc |  4 ++--
 lib/message.cc          | 12 ++++++------
 lib/notmuch-private.h   |  2 --
 lib/notmuch.h           |  6 ++++++
 5 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index 8c145540..709b2ac4 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -314,7 +314,7 @@ _index_mime_part (notmuch_message_t *message,
     const char *charset;
 
     if (! part) {
-	_notmuch_database_log (_notmuch_message_database (message),
+	_notmuch_database_log (notmuch_message_get_database (message),
 			      "Warning: Not indexing empty mime part.\n");
 	return;
     }
@@ -345,7 +345,7 @@ _index_mime_part (notmuch_message_t *message,
 		if (i == 1)
 		    continue;
 		if (i > 1)
-		    _notmuch_database_log (_notmuch_message_database (message),
+		    _notmuch_database_log (notmuch_message_get_database (message),
 					  "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
 	    }
 	    if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
@@ -369,7 +369,7 @@ _index_mime_part (notmuch_message_t *message,
     }
 
     if (! (GMIME_IS_PART (part))) {
-	_notmuch_database_log (_notmuch_message_database (message),
+	_notmuch_database_log (notmuch_message_get_database (message),
 			      "Warning: Not indexing unknown mime part: %s.\n",
 			      g_type_name (G_OBJECT_TYPE (part)));
 	return;
diff --git a/lib/message-property.cc b/lib/message-property.cc
index e81843d3..54918be7 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -52,7 +52,7 @@ _notmuch_message_modify_property (notmuch_message_t *message, const char *key, c
     notmuch_status_t status;
     char *term = NULL;
 
-    status = _notmuch_database_ensure_writable (_notmuch_message_database (message));
+    status = _notmuch_database_ensure_writable (notmuch_message_get_database (message));
     if (status)
 	return status;
 
@@ -99,7 +99,7 @@ notmuch_message_remove_all_properties (notmuch_message_t *message, const char *k
     notmuch_status_t status;
     const char * term_prefix;
 
-    status = _notmuch_database_ensure_writable (_notmuch_message_database (message));
+    status = _notmuch_database_ensure_writable (notmuch_message_get_database (message));
     if (status)
 	return status;
 
diff --git a/lib/message.cc b/lib/message.cc
index 9ba24e8c..fee8fc24 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -260,7 +260,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch,
 
 	doc_id = _notmuch_database_generate_doc_id (notmuch);
     } catch (const Xapian::Error &error) {
-	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred creating message: %s\n",
+	_notmuch_database_log(notmuch_message_get_database (message), "A Xapian exception occurred creating message: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	*status_ret = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
@@ -396,7 +396,7 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
 	if (!message->in_reply_to)
 	    message->in_reply_to = talloc_strdup (message, "");
     } catch (const Xapian::Error &error) {
-	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred fetching message metadata\n",
+	_notmuch_database_log(notmuch_message_get_database (message), "A Xapian exception occurred fetching message metadata\n",
 		 error.get_msg().c_str());
 	message->notmuch->exception_reported = TRUE;
 	return NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
@@ -480,7 +480,7 @@ _notmuch_message_ensure_message_file (notmuch_message_t *message)
 	return;
 
     message->message_file = _notmuch_message_file_open_ctx (
-	_notmuch_message_database (message), message, filename);
+	notmuch_message_get_database (message), message, filename);
 }
 
 const char *
@@ -510,7 +510,7 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header)
 		return talloc_strdup (message, value.c_str ());
 
 	} catch (Xapian::Error &error) {
-	    _notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred when reading header: %s\n",
+	    _notmuch_database_log(notmuch_message_get_database (message), "A Xapian exception occurred when reading header: %s\n",
 		     error.get_msg().c_str());
 	    message->notmuch->exception_reported = TRUE;
 	    return NULL;
@@ -976,7 +976,7 @@ notmuch_message_get_date (notmuch_message_t *message)
     try {
 	value = message->doc.get_value (NOTMUCH_VALUE_TIMESTAMP);
     } catch (Xapian::Error &error) {
-	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred when reading date: %s\n",
+	_notmuch_database_log(notmuch_message_get_database (message), "A Xapian exception occurred when reading date: %s\n",
 		 error.get_msg().c_str());
 	message->notmuch->exception_reported = TRUE;
 	return 0;
@@ -1818,7 +1818,7 @@ notmuch_message_destroy (notmuch_message_t *message)
 }
 
 notmuch_database_t *
-_notmuch_message_database (notmuch_message_t *message)
+notmuch_message_get_database (notmuch_message_t *message)
 {
     return message->notmuch;
 }
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 7b35fc5b..2cad0795 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -503,8 +503,6 @@ _notmuch_query_count_documents (notmuch_query_t *query,
 void
 _notmuch_message_add_reply (notmuch_message_t *message,
 			    notmuch_message_t *reply);
-notmuch_database_t *
-_notmuch_message_database (notmuch_message_t *message);
 
 /* sha1.c */
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 2e8ccb05..5fd85105 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1662,6 +1662,12 @@ void
 notmuch_message_destroy (notmuch_message_t *message);
 
 /**
+ * Return the notmuch database of this message.
+ */
+notmuch_database_t *
+notmuch_message_get_database (notmuch_message_t *message);
+
+/**
  * @name Message Properties
  *
  * This interface provides the ability to attach arbitrary (key,value)
-- 
2.11.0

[PATCH 8/8] lib: add status return to notmuch_message_get_flag

[David Bremner]

This is needed to propagate errors from
notmuch_message_ensure_metadata. We make all the changes needed to
keep the code compiling in atomically in this commit.
---
 lib/database.cc  |  9 ++++++---
 lib/message.cc   | 21 ++++++++++++++++-----
 lib/notmuch.h    | 10 ++++++++--
 lib/thread.cc    | 16 +++++++++++++---
 notmuch-client.h | 15 +++++++++++++++
 notmuch-search.c |  2 +-
 notmuch-show.c   | 12 ++++++------
 7 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 2d19f20c..b60d1f23 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -2519,9 +2519,12 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 	 * message?  We have to be slightly careful: if this is a
 	 * blank message, it's not safe to call
 	 * notmuch_message_get_flag yet. */
-	if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND ||
-	    (is_ghost = notmuch_message_get_flag (
-		message, NOTMUCH_MESSAGE_FLAG_GHOST))) {
+	if (private_status != NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+	    ret = notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_GHOST, &is_ghost);
+	    if (ret)
+		goto DONE;
+	}
+	if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND || is_ghost) {
 	    _notmuch_message_add_term (message, "type", "mail");
 	    if (is_ghost)
 		/* Convert ghost message to a regular message */
diff --git a/lib/message.cc b/lib/message.cc
index fee8fc24..a319fde7 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -946,15 +946,26 @@ notmuch_message_get_filenames (notmuch_message_t *message)
     return _notmuch_filenames_create (message, message->filename_list);
 }
 
-notmuch_bool_t
+notmuch_status_t
 notmuch_message_get_flag (notmuch_message_t *message,
-			  notmuch_message_flag_t flag)
+			  notmuch_message_flag_t flag,
+			  notmuch_bool_t *value)
 {
+    notmuch_private_status_t private_status;
+
+    if (value == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
     if (flag == NOTMUCH_MESSAGE_FLAG_GHOST &&
-	! NOTMUCH_TEST_BIT (message->lazy_flags, flag))
-	_notmuch_message_ensure_metadata (message);
+	! NOTMUCH_TEST_BIT (message->lazy_flags, flag)) {
+	private_status = _notmuch_message_ensure_metadata (message);
+	if (private_status)
+	    return COERCE_STATUS (private_status,
+				  "Unhandled error reading message metadata");
+    }
 
-    return NOTMUCH_TEST_BIT (message->flags, flag);
+    *value =  NOTMUCH_TEST_BIT (message->flags, flag);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 void
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 5fd85105..ac588922 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1379,10 +1379,16 @@ typedef enum _notmuch_message_flag {
 
 /**
  * Get a value of a flag for the email corresponding to 'message'.
+ * @returns
+ * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: an error occured reading message metadata from disk
+ * - NOTMUCH_STATUS_NULL_POINTER: Neither *key* nor *value* may be NULL.
+ * - NOTMUCH_STATUS_SUCCESS: No error occured.
+ * @since libnotmuch 5 (notmuch 0.24)
  */
-notmuch_bool_t
+notmuch_status_t
 notmuch_message_get_flag (notmuch_message_t *message,
-			  notmuch_message_flag_t flag);
+			  notmuch_message_flag_t flag,
+			  notmuch_bool_t *value);
 
 /**
  * Set a value of a flag for the email corresponding to 'message'.
diff --git a/lib/thread.cc b/lib/thread.cc
index 84ee5298..56c9ea81 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -346,13 +346,15 @@ _thread_set_subject_from_message (notmuch_thread_t *thread,
  * search specification. The 'sort' parameter controls whether the
  * oldest or newest matching subject is applied to the thread as a
  * whole. */
-static void
+static notmuch_status_t
 _thread_add_matched_message (notmuch_thread_t *thread,
 			     notmuch_message_t *message,
 			     notmuch_sort_t sort)
 {
     time_t date;
     notmuch_message_t *hashed_message;
+    notmuch_status_t status;
+    notmuch_bool_t is_excluded;
 
     date = notmuch_message_get_date (message);
 
@@ -369,7 +371,11 @@ _thread_add_matched_message (notmuch_thread_t *thread,
 	    _thread_set_subject_from_message (thread, message);
     }
 
-    if (!notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED))
+    status = notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED, &is_excluded);
+    if (status)
+	return status;
+
+    if (! is_excluded)
 	thread->matched_messages++;
 
     if (g_hash_table_lookup_extended (thread->message_hash,
@@ -380,6 +386,7 @@ _thread_add_matched_message (notmuch_thread_t *thread,
     }
 
     _thread_add_matched_author (thread, _notmuch_message_get_author (hashed_message));
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static void
@@ -524,7 +531,10 @@ _notmuch_thread_create (void *ctx,
 
 	if ( _notmuch_doc_id_set_contains (match_set, doc_id)) {
 	    _notmuch_doc_id_set_remove (match_set, doc_id);
-	    _thread_add_matched_message (thread, message, sort);
+	    if (_thread_add_matched_message (thread, message, sort)) {
+		thread = NULL;
+		goto DONE;
+	    }
 	}
 
 	_notmuch_message_close (message);
diff --git a/notmuch-client.h b/notmuch-client.h
index d026e600..108b892b 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -492,6 +492,21 @@ print_status_database (const char *loc,
 int
 status_to_exit (notmuch_status_t status);
 
+inline notmuch_bool_t
+wrap_message_get_flag (notmuch_message_t *message,
+				   notmuch_message_flag_t flag) {
+    notmuch_bool_t value;
+    notmuch_status_t status;
+
+    status = print_status_database ("notmuch_message_get_flag",
+				    notmuch_message_get_database (message),
+				    notmuch_message_get_flag (message, flag, &value));
+    if (status)
+	exit (status_to_exit (status));
+
+    return value;
+}
+
 #include "command-line-arguments.h"
 
 extern char *notmuch_requested_db_uuid;
diff --git a/notmuch-search.c b/notmuch-search.c
index 8c65d5ad..899eb09c 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -91,7 +91,7 @@ get_thread_query (notmuch_thread_t *thread,
 	notmuch_message_t *message = notmuch_messages_get (messages);
 	const char *mid = notmuch_message_get_message_id (message);
 	/* Determine which query buffer to extend */
-	char **buf = notmuch_message_get_flag (
+	char **buf = wrap_message_get_flag (
 	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmatched_out;
 	/* Add this message's id: query.  Since "id" is an exclusive
 	 * prefix, it is implicitly 'or'd together, so we only need to
diff --git a/notmuch-show.c b/notmuch-show.c
index 22fa655a..398b084e 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -127,10 +127,10 @@ format_message_sprinter (sprinter_t *sp, notmuch_message_t *message)
     sp->string (sp, notmuch_message_get_message_id (message));
 
     sp->map_key (sp, "match");
-    sp->boolean (sp, notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH));
+    sp->boolean (sp, wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH));
 
     sp->map_key (sp, "excluded");
-    sp->boolean (sp, notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED));
+    sp->boolean (sp, wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED));
 
     sp->map_key (sp, "filename");
     sp->string (sp, notmuch_message_get_filename (message));
@@ -446,8 +446,8 @@ format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node,
 		part_type,
 		notmuch_message_get_message_id (message),
 		indent,
-		notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) ? 1 : 0,
-		notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED) ? 1 : 0,
+		wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) ? 1 : 0,
+		wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED) ? 1 : 0,
 		notmuch_message_get_filename (message));
     } else {
 	GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (meta);
@@ -854,8 +854,8 @@ show_messages (void *ctx,
 
 	message = notmuch_messages_get (messages);
 
-	match = notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH);
-	excluded = notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED);
+	match = wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH);
+	excluded = wrap_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED);
 
 	next_indent = indent;
 
-- 
2.11.0

Re: add status value to _notmuch_message_ensure_metadata

[Gaute Hope]

David Bremner writes on februar 18, 2017 15:45:
> In id:1487339566.mz8acpov1j.astroid@strange.none , Gaute provided a
> traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
> this is simple, but somewhat intrusive.
>
> [...]
>
> I haven't tested against Gaute's test case (needs more boost than I
> have handy).

Hi, thanks this seems to get rid of the hard crash, the output from my test
case is attached below.

I have not yet tried to use any new status returns, it seems that the
immediate result is that `notmuch_threads_get ()` returns a valid (but
empty?) notmuch_thread_t * which results in
`notmuch_thread_get_thread_id ()` returning NULL. Strangely, at some
seemingly arbitrary intervals the methods seem to work correctly and
return a valid `notmuch_thread_t` struct. 

`threads` and `thread` remain not NULL.


```
$ test/test_notmuch_standalone
db: running test query..
query: *, approx: 10 threads.
thread id to change: 0000000000000002, thread no: 3
restarting query..
moving to thread: 2
tags: inbox unread 
tags: inbox unread 
continue loading..
threads != NULL
thread != NULL
loading: 2: 
tags: 
threads != NULL
thread != NULL
loading: 3: 0000000000000009
tags: inbox unread 
threads != NULL
thread != NULL
loading: 4: 
tags: 
threads != NULL
thread != NULL
loading: 5: 
tags: 
threads != NULL
thread != NULL
loading: 6: 0000000000000004
tags: inbox unread 
threads != NULL
thread != NULL
loading: 7: 0000000000000006
tags: inbox unread 
threads != NULL
thread != NULL
loading: 8: 000000000000000a
tags: inbox signed unread 
threads != NULL
thread != NULL
loading: 9: 0000000000000005
tags: inbox unread 
```

with the following changes made to the test program:
```
diff --git a/test/test_notmuch_standalone.cc b/test/test_notmuch_standalone.cc
index 5ef0a00..4634986 100644
--- a/test/test_notmuch_standalone.cc
+++ b/test/test_notmuch_standalone.cc
@@ -189,8 +189,16 @@ int main () {
       cout << "threads != NULL" << endl;
     }
     thread = notmuch_threads_get (threads);
+    if (thread == NULL) {
+      cout << "thread == NULL" << endl;
+    } else {
+      cout << "thread != NULL" << endl;
+    }
+
     cout << "loading: " << i;
-    std::string tid = notmuch_thread_get_thread_id (thread);
+    const char * cid = notmuch_thread_get_thread_id (thread);
+    std::string tid = "";
+    if (cid != NULL) tid = cid;
     cout << ": " << tid << endl;
 
     /* get tags */
```

- gaute

Re: add status value to _notmuch_message_ensure_metadata

[Gaute Hope]

[-- Attachment #1.1: Type: text/plain, Size: 530 bytes --]

Gaute Hope writes on februar 20, 2017 10:27:
> David Bremner writes on februar 18, 2017 15:45:
>> In id:1487339566.mz8acpov1j.astroid@strange.none , Gaute provided a
>> traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
>> this is simple, but somewhat intrusive.
>>
>> [...]
>>
>> I haven't tested against Gaute's test case (needs more boost than I
>> have handy).

Alright then, attached is a non-boost version that takes a notmuch db
path (absolute) as the first argument (no warranty).

- gaute

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: test_notmuch_standalone.cc --]
[-- Type: text/x-c++src; name=test_notmuch_standalone.cc, Size: 5469 bytes --]

# include <iostream>
# include <unistd.h>

# include <notmuch.h>

using std::cout;
using std::endl;

int main (int argc, char **argv) {
  if (argc < 2) {
    std::cerr << "error: specify the path to the test notmuch database" << endl;
    return 1;
  }

  std::string path_db;
  path_db = argv[1];

  cout << "using db: " << path_db << endl;

  notmuch_database_t * nm_db;

  notmuch_status_t s =
    notmuch_database_open (
      path_db.c_str(),
      notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_ONLY,
      &nm_db);


  cout << "db: running test query.." << endl;
  notmuch_query_t * q = notmuch_query_create (nm_db, "*");

  unsigned int c;
  notmuch_status_t st = notmuch_query_count_threads_st (q, &c); // destructive
  notmuch_query_destroy (q);
  q = notmuch_query_create (nm_db, "*");

  cout << "query: " << notmuch_query_get_query_string (q) << ", approx: "
       << c << " threads." << endl;

  notmuch_threads_t * threads;
  notmuch_thread_t  * thread;
  st = notmuch_query_search_threads_st (q, &threads);

  std::string thread_id;

  int i = 0;

  for (; notmuch_threads_valid (threads);
         notmuch_threads_move_to_next (threads)) {
    thread = notmuch_threads_get (threads);
    i++;

    if (i == 3)
      thread_id = notmuch_thread_get_thread_id (thread);

    notmuch_thread_destroy (thread);

    if (i == 3) break;
  }

  cout << "thread id to change: " << thread_id << ", thread no: " << i << endl;
  notmuch_query_destroy (q);

  /* restart query */
  cout << "restarting query.." << endl;
  q = notmuch_query_create (nm_db, "*");
  st = notmuch_query_search_threads_st (q, &threads);

  i = 0;
  int stop = 2;

  cout << "moving to thread: " << stop << endl;
  for ( ; notmuch_threads_valid (threads);
          notmuch_threads_move_to_next (threads))
  {
    thread = notmuch_threads_get (threads);
    notmuch_thread_get_thread_id (thread);
    i++;

    cout << "tags: ";

    /* get tags */
    notmuch_tags_t *tags;
    const char *tag;

    for (tags = notmuch_thread_get_tags (thread);
         notmuch_tags_valid (tags);
         notmuch_tags_move_to_next (tags))
    {
        tag = notmuch_tags_get (tags);
        cout << tag << " ";
    }
    cout << endl;

    notmuch_thread_destroy (thread);

    if (i == stop) break;
  }

  /* now open a new db instance, modify the already loaded thread and
   * continue loading the original query */
  notmuch_database_t * nm_db2;

  s = notmuch_database_open (
      path_db.c_str(),
      notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
      &nm_db2);


  char qry_s[256];
  sprintf (qry_s, "thread:%s", thread_id.c_str ());
  notmuch_query_t * q2 = notmuch_query_create (nm_db2, qry_s);
  notmuch_threads_t * ts2;
  notmuch_thread_t  * t2;

  st = notmuch_query_search_threads_st (q2, &ts2);

  for ( ; notmuch_threads_valid (ts2);
          notmuch_threads_move_to_next (ts2))
  {
    t2 = notmuch_threads_get (ts2);
    std::string thread_id = notmuch_thread_get_thread_id (t2);


    /* remove unread tag */
    notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
    notmuch_message_t  * m;

    for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
      m = notmuch_messages_get (ms);

      st = notmuch_message_remove_tag (m, "unread");

      notmuch_message_destroy (m);
    }

    notmuch_messages_destroy (ms);


    notmuch_thread_destroy (t2);
    break;
  }

  notmuch_query_destroy (q2);
  notmuch_database_close (nm_db2);

  /* re-add unread tag */
  s = notmuch_database_open (
      path_db.c_str(),
      notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
      &nm_db2);



  q2 = notmuch_query_create (nm_db2, qry_s);

  st = notmuch_query_search_threads_st (q2, &ts2);

  for ( ; notmuch_threads_valid (ts2);
          notmuch_threads_move_to_next (ts2))
  {
    t2 = notmuch_threads_get (ts2);
    std::string thread_id = notmuch_thread_get_thread_id (t2);


    /* remove unread tag */
    notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
    notmuch_message_t  * m;

    for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
      m = notmuch_messages_get (ms);

      st = notmuch_message_add_tag (m, "unread");

      notmuch_message_destroy (m);
    }

    notmuch_messages_destroy (ms);


    notmuch_thread_destroy (t2);
    break;
  }

  notmuch_query_destroy (q2);
  notmuch_database_close (nm_db2);

  /* continue loading */
  cout << "continue loading.." << endl;
  for ( ; notmuch_threads_valid (threads);
          notmuch_threads_move_to_next (threads))
  {
    if (threads == NULL) {
      cout << "threads == NULL" << endl;
    } else {
      cout << "threads != NULL" << endl;
    }
    thread = notmuch_threads_get (threads);
    if (thread == NULL) {
      cout << "thread == NULL" << endl;
    } else {
      cout << "thread != NULL" << endl;
    }

    cout << "loading: " << i;
    const char * cid = notmuch_thread_get_thread_id (thread);
    std::string tid = "";
    if (cid != NULL) tid = cid;
    cout << ": " << tid << endl;

    /* get tags */
    notmuch_tags_t *tags;
    const char *tag;

    cout << "tags: ";
    for (tags = notmuch_thread_get_tags (thread);
         notmuch_tags_valid (tags);
         notmuch_tags_move_to_next (tags))
    {
        tag = notmuch_tags_get (tags);
        cout << tag << " ";
    }
    cout << endl;

    i++;
    notmuch_thread_destroy (thread);
  }

  notmuch_database_close (nm_db);
  return 0;
}


[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]

Re: add status value to _notmuch_message_ensure_metadata

[David Bremner]

Gaute Hope <eg@gaute.vetsj.com> writes:

> Gaute Hope writes on februar 20, 2017 10:27:
>> David Bremner writes on februar 18, 2017 15:45:
>>> In id:1487339566.mz8acpov1j.astroid@strange.none , Gaute provided a
>>> traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
>>> this is simple, but somewhat intrusive.
>>>
>>> [...]
>>>
>>> I haven't tested against Gaute's test case (needs more boost than I
>>> have handy).
>
> Alright then, attached is a non-boost version that takes a notmuch db
> path (absolute) as the first argument (no warranty).
>

With the patches above this crashes in a predictable / preventable way,
because notmuch_message_get_tags returns NULL. It isn't clear to me yet
what the best API choice is here: minimize difference with the old API
by returning NULL to indicate errors, or switch completely to the
pattern of e.g. notmuch_query_search_messages_st. I suppose we could do
something along the same lines and add new _st versions of the
problematic functions

Re: add status value to _notmuch_message_ensure_metadata

[Gaute Hope]

David Bremner writes on februar 23, 2017 1:58:
> Gaute Hope <eg@gaute.vetsj.com> writes:
> 
>> Gaute Hope writes on februar 20, 2017 10:27:
>>> David Bremner writes on februar 18, 2017 15:45:
>>>> In id:1487339566.mz8acpov1j.astroid@strange.none , Gaute provided a
>>>> traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
>>>> this is simple, but somewhat intrusive.
>>>>
>>>> [...]
>>>>
>>>> I haven't tested against Gaute's test case (needs more boost than I
>>>> have handy).
>>
>> Alright then, attached is a non-boost version that takes a notmuch db
>> path (absolute) as the first argument (no warranty).
>>
> 
> With the patches above this crashes in a predictable / preventable way,
> because notmuch_message_get_tags returns NULL. It isn't clear to me yet
> what the best API choice is here: minimize difference with the old API
> by returning NULL to indicate errors, or switch completely to the
> pattern of e.g. notmuch_query_search_messages_st. I suppose we could do
> something along the same lines and add new _st versions of the
> problematic functions
> 

Hi,

Ideally if the error could be caught in `notmuch_threads_valid` or
`notmuch_threads_get_thread` I think that would be the clearest, _st
versions would be nice. As I mentioned in
id:1487582192.57s86yczcg.astroid@strange.none it seems that at later
arbitrary iterations (without re-loading the threads object) the
functions return valid data (even `notmuch_thread_get_tags` does). Can
this data be trusted? I feel like this should all be invalid at this
point.

Regards, Gaute

Re: add status value to _notmuch_message_ensure_metadata

[David Bremner]

Gaute Hope <eg@gaute.vetsj.com> writes:


> Ideally if the error could be caught in `notmuch_threads_valid` or
> `notmuch_threads_get_thread` I think that would be the clearest, _st
> versions would be nice.

We can't really control when the exceptions happen, due to lazily
reading data from disk.  Looking more carefully at the backtrace, the
problem is actually inside the library, in particular some descendent of
notmuch_threads_get is not properly handling the error from
notmuch_message_get_tags.

> As I mentioned in
> id:1487582192.57s86yczcg.astroid@strange.none it seems that at later
> arbitrary iterations (without re-loading the threads object) the
> functions return valid data (even `notmuch_thread_get_tags` does). Can
> this data be trusted? I feel like this should all be invalid at this
> point.
>

I guess that's up to Xapian to decide, but I imagine anything after the
first exception is "undefined behaviour". Data is cached in memory per
message in the notmuch layer, so in principle later calls that don't
actually reach xapian could succeed.

Re: [PATCH 1/8] lib: make _notmuch_message_ensure_metadata static

[David Bremner]

David Bremner <david@tethera.net> writes:

> It's not called anywhere outside message.cc.

patch 1 and 5 pushed to master

[RFC patch 1/2] lib: add notmuch_database_reopen

[David Bremner]

The main expected use is to recover from a Xapian::DatabaseChanged
exception.
---
 lib/database.cc | 19 +++++++++++++++++++
 lib/notmuch.h   | 12 ++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index 386dcd17..9c4be516 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1133,6 +1133,25 @@ notmuch_database_close (notmuch_database_t *notmuch)
     return status;
 }
 
+notmuch_status_t
+notmuch_database_reopen (notmuch_database_t *notmuch)
+{
+    if (notmuch->mode != NOTMUCH_DATABASE_MODE_READ_ONLY)
+	return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
+
+    try {
+	notmuch->xapian_db->reopen ();
+    } catch (const Xapian::Error &error) {
+	if (! notmuch->exception_reported) {
+	    _notmuch_database_log (notmuch, "Error: A Xapian exception reopening database: %s\n",
+				   error.get_msg ().c_str ());
+	    notmuch->exception_reported = TRUE;
+	    return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	}
+    }
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 static int
 unlink_cb (const char *path,
 	   unused (const struct stat *sb),
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 16da8be9..e9ed01dd 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -363,6 +363,18 @@ notmuch_status_t
 notmuch_database_close (notmuch_database_t *database);
 
 /**
+ * Reopen a (read-only) database, synching the database view with that
+ * on disk.
+ *
+ * @returns
+ * - NOTMUCH_STATUS_UNSUPPORTED_OPERATION: database is not read only
+ * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured trying to
+ *   re-open the database.
+ */
+notmuch_status_t
+notmuch_database_reopen (notmuch_database_t *database);
+
+/**
  * A callback invoked by notmuch_database_compact to notify the user
  * of the progress of the compaction process.
  */
-- 
2.11.0

[RFC patch 2/2] lib: handle DatabaseModifiedError in _n_message_ensure_metadata

[David Bremner]

The error handling here still needs work. The retry count should be
handled in more sane way, and both running out of retries and an error
return from notmuch_database_reopen should be handled.
---
 lib/message.cc | 144 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 80 insertions(+), 64 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 2fb67d85..b7c377fc 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -310,10 +310,12 @@ _notmuch_message_get_term (notmuch_message_t *message,
     return value;
 }
 
-static void
+void
 _notmuch_message_ensure_metadata (notmuch_message_t *message)
 {
     Xapian::TermIterator i, end;
+    notmuch_bool_t success = FALSE;
+
     const char *thread_prefix = _find_prefix ("thread"),
 	*tag_prefix = _find_prefix ("tag"),
 	*id_prefix = _find_prefix ("id"),
@@ -327,73 +329,87 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
      * slightly more costly than looking up individual fields if only
      * one field of the message object is actually used, it's a huge
      * win as more fields are used. */
+    for (int count=0; count < 100 && !success; count++) {
+	try {
+	    i = message->doc.termlist_begin ();
+	    end = message->doc.termlist_end ();
+
+	    /* Get thread */
+	    if (!message->thread_id)
+		message->thread_id =
+		    _notmuch_message_get_term (message, i, end, thread_prefix);
+
+	    /* Get tags */
+	    assert (strcmp (thread_prefix, tag_prefix) < 0);
+	    if (!message->tag_list) {
+		message->tag_list =
+		    _notmuch_database_get_terms_with_prefix (message, i, end,
+							     tag_prefix);
+		_notmuch_string_list_sort (message->tag_list);
+	    }
 
-    i = message->doc.termlist_begin ();
-    end = message->doc.termlist_end ();
-
-    /* Get thread */
-    if (!message->thread_id)
-	message->thread_id =
-	    _notmuch_message_get_term (message, i, end, thread_prefix);
-
-    /* Get tags */
-    assert (strcmp (thread_prefix, tag_prefix) < 0);
-    if (!message->tag_list) {
-	message->tag_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     tag_prefix);
-	_notmuch_string_list_sort (message->tag_list);
-    }
+	    /* Get id */
+	    assert (strcmp (tag_prefix, id_prefix) < 0);
+	    if (!message->message_id)
+		message->message_id =
+		    _notmuch_message_get_term (message, i, end, id_prefix);
+
+	    /* Get document type */
+	    assert (strcmp (id_prefix, type_prefix) < 0);
+	    if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
+		i.skip_to (type_prefix);
+		/* "T" is the prefix "type" fields.  See
+		 * BOOLEAN_PREFIX_INTERNAL. */
+		if (*i == "Tmail")
+		    NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+		else if (*i == "Tghost")
+		    NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+		else
+		    INTERNAL_ERROR ("Message without type term");
+		NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+	    }
 
-    /* Get id */
-    assert (strcmp (tag_prefix, id_prefix) < 0);
-    if (!message->message_id)
-	message->message_id =
-	    _notmuch_message_get_term (message, i, end, id_prefix);
-
-    /* Get document type */
-    assert (strcmp (id_prefix, type_prefix) < 0);
-    if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
-	i.skip_to (type_prefix);
-	/* "T" is the prefix "type" fields.  See
-	 * BOOLEAN_PREFIX_INTERNAL. */
-	if (*i == "Tmail")
-	    NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-	else if (*i == "Tghost")
-	    NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-	else
-	    INTERNAL_ERROR ("Message without type term");
-	NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+	    /* Get filename list.  Here we get only the terms.  We lazily
+	     * expand them to full file names when needed in
+	     * _notmuch_message_ensure_filename_list. */
+	    assert (strcmp (type_prefix, filename_prefix) < 0);
+	    if (!message->filename_term_list && !message->filename_list)
+		message->filename_term_list =
+		    _notmuch_database_get_terms_with_prefix (message, i, end,
+							     filename_prefix);
+
+
+	    /* Get property terms. Mimic the setup with filenames above */
+	    assert (strcmp (filename_prefix, property_prefix) < 0);
+	    if (!message->property_map && !message->property_term_list)
+		message->property_term_list =
+		    _notmuch_database_get_terms_with_prefix (message, i, end,
+							 property_prefix);
+
+	    /* Get reply to */
+	    assert (strcmp (property_prefix, replyto_prefix) < 0);
+	    if (!message->in_reply_to)
+		message->in_reply_to =
+		    _notmuch_message_get_term (message, i, end, replyto_prefix);
+
+
+	    /* It's perfectly valid for a message to have no In-Reply-To
+	     * header. For these cases, we return an empty string. */
+	    if (!message->in_reply_to)
+		message->in_reply_to = talloc_strdup (message, "");
+
+	    /* all the way without an exception */
+	    success = TRUE;
+	} catch (const Xapian::DatabaseModifiedError &error) {
+	    (void) notmuch_database_reopen (message->notmuch);
+	    success = FALSE;
+	} catch (const Xapian::Error &error) {
+	    _notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred fetching message metadata\n",
+				  error.get_msg().c_str());
+	    message->notmuch->exception_reported = TRUE;
+	}
     }
 
-    /* Get filename list.  Here we get only the terms.  We lazily
-     * expand them to full file names when needed in
-     * _notmuch_message_ensure_filename_list. */
-    assert (strcmp (type_prefix, filename_prefix) < 0);
-    if (!message->filename_term_list && !message->filename_list)
-	message->filename_term_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     filename_prefix);
-
-
-    /* Get property terms. Mimic the setup with filenames above */
-    assert (strcmp (filename_prefix, property_prefix) < 0);
-    if (!message->property_map && !message->property_term_list)
-	message->property_term_list =
-	    _notmuch_database_get_terms_with_prefix (message, i, end,
-						     property_prefix);
-
-    /* Get reply to */
-    assert (strcmp (property_prefix, replyto_prefix) < 0);
-    if (!message->in_reply_to)
-	message->in_reply_to =
-	    _notmuch_message_get_term (message, i, end, replyto_prefix);
-
-
-    /* It's perfectly valid for a message to have no In-Reply-To
-     * header. For these cases, we return an empty string. */
-    if (!message->in_reply_to)
-	message->in_reply_to = talloc_strdup (message, "");
 }
 
 void
-- 
2.11.0

Re: [RFC patch 2/2] lib: handle DatabaseModifiedError in _n_message_ensure_metadata

[David Bremner]

David Bremner <david@tethera.net> writes:

> The error handling here still needs work. The retry count should be
> handled in more sane way, and both running out of retries and an error
> return from notmuch_database_reopen should be handled.

Probably the number of retries can be limited to 1, since afaict, reopen
is not going to return any recoverable errors.

Gaute's test program seems run to completion without any source
modifications.

One general comment about this approach is that it does not do anything
about any the metadata that may be cached by notmuch for other
messages. Roughly speaking the behaviour should be atomic with respect
to messages, but not e.g. within threads. A message read earlier may not
reflect changes seen in a later one. That potential inconsistency would
only occur in cases currently resulting in crashes.

It is possible there is some scheme we can use to invalidate all of the
cached metadata. Here's an initial idea. The notmuch_database_t object
keeps a counter/timestamp which indicates the last time it was
invalidated. Each message object has a counter/timestamp indicating the
last time it it was synced/validated. Instead of checking if some
pointer is null force a read, we'd have to compare these two counters. I
think the main counter could be set to zero by database_open and
incremented by database_reopen

Re: [RFC patch 2/2] lib: handle DatabaseModifiedError in _n_message_ensure_metadata

[Gaute Hope]

David Bremner writes on februar 24, 2017 3:49:
> David Bremner <david@tethera.net> writes:
> 
>> The error handling here still needs work. The retry count should be
>> handled in more sane way, and both running out of retries and an error
>> return from notmuch_database_reopen should be handled.
> 
> Probably the number of retries can be limited to 1, since afaict, reopen
> is not going to return any recoverable errors.

Does _reopen block if the db is locked?

Re: [RFC patch 2/2] lib: handle DatabaseModifiedError in _n_message_ensure_metadata

[David Bremner]

Gaute Hope <eg@gaute.vetsj.com> writes:

> David Bremner writes on februar 24, 2017 3:49:
>> David Bremner <david@tethera.net> writes:
>> 
>>> The error handling here still needs work. The retry count should be
>>> handled in more sane way, and both running out of retries and an error
>>> return from notmuch_database_reopen should be handled.
>> 
>> Probably the number of retries can be limited to 1, since afaict, reopen
>> is not going to return any recoverable errors.
>
> Does _reopen block if the db is locked?

It's not documented to block or throw an exception in that case. It's
only applicable to read-only databases, so the lack of blocking makes
sense.

d