fourth batch of API cleanup for exception handling

fourth batch of API cleanup for exception handling

[David Bremner]

This continues

     id:20200705130025.4057292-1-david@tethera.net

and probably needs to be applied on top of it.

There are a couple cases here where we use the NULL returns from
previous API changes and convert it back into
NOTMUCH_STATUS_XAPIAN_EXCEPTION. It isn't the most lovely error path,
but it works.

I guess an enhancement would be to have some kind of errno set as
Floris mentioned. We'd still want the NULL returns to signal an error
however, so I think that could be done incrementally.

This completes (I think) the changes needed for message.cc

d

[PATCH 01/10] test: add regression test for notmuch_message_has_maildir_flag

[David Bremner]

This passes the NULL return inside _ensure_maildir_flags does not
break anything. Probably this should be handled more explicitely.
---
 lib/message.cc         |  3 ++-
 test/T560-lib-error.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/message.cc b/lib/message.cc
index 4e1be986..bb4b2fa1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1732,7 +1732,8 @@ _ensure_maildir_flags (notmuch_message_t *message, bool force)
 	    message->maildir_flags = NULL;
 	}
     }
-
+    /* n_m_get_filenames returns NULL for errors, which terminates the
+     * loop */
     for (filenames = notmuch_message_get_filenames (message);
 	 notmuch_filenames_valid (filenames);
 	 notmuch_filenames_move_to_next (filenames)) {
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index afb53346..59a59d82 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -550,4 +550,20 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle read maildir flag with closed database"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_bool_t is_set = -1;
+        is_set = notmuch_message_has_maildir_flag (message, 'S');
+        printf("%d\n%d\n", message != NULL, is_set == FALSE || is_set == TRUE);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 02/10] lib: add notmuch_message_has_maildir_flag_st

[David Bremner]

Initially the new function is mainly tested indirectly via the
wrapper.
---
 lib/message.cc         | 41 ++++++++++++++++++++++++++++++++++-------
 lib/notmuch.h          | 22 ++++++++++++++++++++++
 test/T560-lib-error.sh | 17 +++++++++++++++++
 3 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index bb4b2fa1..8e090aa3 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1717,7 +1717,7 @@ _filename_is_in_maildir (const char *filename)
     return NULL;
 }
 
-static void
+static notmuch_status_t
 _ensure_maildir_flags (notmuch_message_t *message, bool force)
 {
     const char *flags;
@@ -1732,9 +1732,10 @@ _ensure_maildir_flags (notmuch_message_t *message, bool force)
 	    message->maildir_flags = NULL;
 	}
     }
-    /* n_m_get_filenames returns NULL for errors, which terminates the
-     * loop */
-    for (filenames = notmuch_message_get_filenames (message);
+    filenames = notmuch_message_get_filenames (message);
+    if (! filenames)
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    for (;
 	 notmuch_filenames_valid (filenames);
 	 notmuch_filenames_move_to_next (filenames)) {
 	filename = notmuch_filenames_get (filenames);
@@ -1760,13 +1761,37 @@ _ensure_maildir_flags (notmuch_message_t *message, bool force)
     }
     if (seen_maildir_info)
 	message->maildir_flags = combined_flags;
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 notmuch_bool_t
 notmuch_message_has_maildir_flag (notmuch_message_t *message, char flag)
 {
-    _ensure_maildir_flags (message, false);
-    return message->maildir_flags && (strchr (message->maildir_flags, flag) != NULL);
+    notmuch_status_t status;
+    notmuch_bool_t ret;
+    status = notmuch_message_has_maildir_flag_st (message, flag, &ret);
+    if (status)
+	return FALSE;
+
+    return ret;
+}
+
+notmuch_status_t
+notmuch_message_has_maildir_flag_st (notmuch_message_t *message,
+				     char flag,
+				     notmuch_bool_t *is_set)
+{
+    notmuch_status_t status;
+    
+    if (! is_set)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    status = _ensure_maildir_flags (message, false);
+    if (status)
+	return status;
+    
+    *is_set =  message->maildir_flags && (strchr (message->maildir_flags, flag) != NULL);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 notmuch_status_t
@@ -1775,7 +1800,9 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message)
     notmuch_status_t status;
     unsigned i;
 
-    _ensure_maildir_flags (message, true);
+    status = _ensure_maildir_flags (message, true);
+    if (status)
+	return status;
     /* If none of the filenames have any maildir info field (not even
      * an empty info with no flags set) then there's no information to
      * go on, so do nothing. */
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 0f386397..4d433698 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1680,10 +1680,32 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message);
  * return TRUE if any filename of 'message' has maildir flag 'flag',
  * FALSE otherwise.
  *
+ * Deprecated wrapper for notmuch_message_has_maildir_flag_st
+ *
+ * @returns FALSE in case of error
+ * @deprecated libnotmuch5.2 (notmuch 0.31)
  */
+NOTMUCH_DEPRECATED(5,2)
 notmuch_bool_t
 notmuch_message_has_maildir_flag (notmuch_message_t *message, char flag);
 
+/**
+ * check message for maildir flag
+ *
+ * @param [in,out]	message message to check
+ * @param [in] flag	flag to check for
+ * @param [out] is_set  pointer to boolean
+ *
+ * @retval #NOTMUCH_STATUS_SUCCESS
+ * @retval #NOTMUCH_STATUS_NULL_POINTER is_set is NULL
+ * @retval #NOTMUCH_STATUS_XAPIAN_EXCEPTION Accessing the database
+ * triggered an exception.
+ */
+notmuch_status_t
+notmuch_message_has_maildir_flag_st (notmuch_message_t *message,
+				     char flag,
+				     notmuch_bool_t *is_set);
+
 /**
  * Rename message filename(s) to encode tags as maildir flags.
  *
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 59a59d82..41aad09d 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -566,4 +566,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle checking maildir flag with closed db (new API)"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        notmuch_bool_t out;
+        status = notmuch_message_has_maildir_flag_st (message, 'S', &out);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 03/10] test: add regression test for n_m_maildir_flags_to_tags

[David Bremner]

This function currently catches at least the obvious Xapian exceptions
and we want to keep it that way.
---
 test/T560-lib-error.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 41aad09d..fb8828fe 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -583,4 +583,20 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle converting maildir flags to tags with closed db"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        status = notmuch_message_maildir_flags_to_tags (message);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 04/10] test: add broken test for n_m_remove_all_tags

[David Bremner]

The Xapian exception is actually caught here, but the NULL return is
not dealt with properly.
---
 test/T560-lib-error.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index fb8828fe..acde5786 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -599,4 +599,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle removing all tags with closed db"
+test_subtest_known_broken
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        status = notmuch_message_remove_all_tags (message);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 05/10] lib: handle xapian exception in n_m_remove_all_tags

[David Bremner]

At least the exception we already catch should be reported properly.
---
 lib/message.cc         | 10 +++++++---
 lib/notmuch.h          |  6 ++++--
 test/T560-lib-error.sh |  1 -
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 8e090aa3..09708ed9 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -2071,16 +2071,20 @@ notmuch_message_remove_all_tags (notmuch_message_t *message)
     status = _notmuch_database_ensure_writable (message->notmuch);
     if (status)
 	return status;
+    tags = notmuch_message_get_tags (message);
+    if (! tags)
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 
-    for (tags = notmuch_message_get_tags (message);
+    for (;
 	 notmuch_tags_valid (tags);
 	 notmuch_tags_move_to_next (tags)) {
 	tag = notmuch_tags_get (tags);
 
 	private_status = _notmuch_message_remove_term (message, "tag", tag);
 	if (private_status) {
-	    INTERNAL_ERROR ("_notmuch_message_remove_term return unexpected value: %d\n",
-			    private_status);
+	    return COERCE_STATUS (private_status,
+				   "_notmuch_message_remove_term return unexpected value: %d\n",
+				   private_status);
 	}
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 4d433698..9a19e2f7 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1635,8 +1635,10 @@ notmuch_message_remove_tag (notmuch_message_t *message, const char *tag);
  * See notmuch_message_freeze for an example showing how to safely
  * replace tag values.
  *
- * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
- *	mode so message cannot be modified.
+ * @retval #NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in
+ *	read-only mode so message cannot be modified.
+ * @retval #NOTMUCH_STATUS_XAPIAN_EXCEPTION: an execption was thrown
+ *      accessing the database.
  */
 notmuch_status_t
 notmuch_message_remove_all_tags (notmuch_message_t *message);
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index acde5786..5361be77 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -600,7 +600,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Handle removing all tags with closed db"
-test_subtest_known_broken
 cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         notmuch_status_t status;
-- 
2.27.0

[PATCH 06/10] test: regression tests of n_m_freeze and n_m_thaw on closed db

[David Bremner]

Neither of these accesses the database, so should be safe. Add the
tests to catch any changes in exception throwing.
---
 test/T560-lib-error.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 5361be77..2bfca179 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -615,4 +615,36 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle freezing message with closed db"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        status = notmuch_message_freeze (message);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_SUCCESS);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Handle thawing message with closed db"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        status = notmuch_message_thaw (message);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 07/10] test: regression test for destroying message with closed db

[David Bremner]

This should be fine because the message belongs to the
database (talloc context wise).
---
 test/T560-lib-error.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 2bfca179..371b0f9e 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -647,4 +647,19 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle destroying message with closed db"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_message_destroy (message);
+        printf("%d\n%d\n", message != NULL,  1);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 08/10] test: regression for retrieving closed db from message

[David Bremner]

This is actually one of the few potentially useful things you can do
with a message belonging to a closed database, since in principle you
could re-open the database.
---
 test/T560-lib-error.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 371b0f9e..ef279e77 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -662,4 +662,20 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle retrieving closed db from message"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_database_t *db2;
+        db2 = notmuch_message_get_database (message);
+        printf("%d\n%d\n", message != NULL,  db == db2);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 09/10] test: add known broken test for n_m_reindex on closed db

[David Bremner]

This is another case where the code should not call INTERNAL_ERROR.
---
 test/T560-lib-error.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index ef279e77..e303720c 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -678,4 +678,21 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Handle reindexing message with closed db"
+test_subtest_known_broken
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        notmuch_status_t status;
+        status = notmuch_message_reindex (message, NULL);
+        printf("%d\n%d\n", message != NULL,  status == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

[PATCH 10/10] lib: fix return value for n_m_reindex

[David Bremner]

Also update the documentation for the behaviour of n_m_get_thread_id
that this fix relies on.
---
 lib/message.cc         | 6 ++++--
 lib/notmuch.h          | 4 ++--
 test/T560-lib-error.sh | 1 -
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 09708ed9..87448101 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -2205,8 +2205,10 @@ notmuch_message_reindex (notmuch_message_t *message,
     /* Save in case we need to delete message */
     orig_thread_id = notmuch_message_get_thread_id (message);
     if (! orig_thread_id) {
-	/* XXX TODO: make up new error return? */
-	INTERNAL_ERROR ("message without thread-id");
+	/* the following is correct as long as there is only one reason
+	   n_m_get_thread_id returns NULL
+	*/
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 
     /* strdup it because the metadata may be invalidated */
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 9a19e2f7..97ebc17d 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1378,8 +1378,8 @@ 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.
+ * This function will return NULL if triggers an unhandled Xapian
+ * exception.
  */
 const char *
 notmuch_message_get_thread_id (notmuch_message_t *message);
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index e303720c..536ff701 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -679,7 +679,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Handle reindexing message with closed db"
-test_subtest_known_broken
 cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         notmuch_status_t status;
-- 
2.27.0

Re: fourth batch of API cleanup for exception handling

[David Bremner]

David Bremner <david@tethera.net> writes:

> This continues
>
>      id:20200705130025.4057292-1-david@tethera.net
>
> and probably needs to be applied on top of it.

Batch applied to master

d