[PATCH 08/10] test: regression for retrieving closed db from message
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 10/10] lib: fix return value for n_m_reindex
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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 01/10] test: add regression test for notmuch_message_has_maildir_flag
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 07/10] test: regression test for destroying message with closed db
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 04/10] test: add broken test for n_m_remove_all_tags
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
fourth batch of API cleanup for exception handling
This continues id:20200705130025.4057292-1-da...@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 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 06/10] test: regression tests of n_m_freeze and n_m_thaw on closed db
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 < 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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 03/10] test: add regression test for n_m_maildir_flags_to_tags
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 02/10] lib: add notmuch_message_has_maildir_flag_st
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, );
+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] flagflag 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 < 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', );
+printf("%d\n%d\n", message != NULL, status ==
NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+}
+EOF
+cat < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 05/10] lib: handle xapian exception in n_m_remove_all_tags
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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 09/10] test: add known broken test for n_m_reindex on closed db
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 < 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 < EXPECTED
+== stdout ==
+1
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
test_done
--
2.27.0
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [RFC PATCH] lib: document new database_open API
On Sat 04 Jul 2020 at 14:01 -0300, David Bremner wrote: > Floris Bruynooghe writes: > >>> + * >>> + * - in the environment variable NOTMUCH_DATABASE, if non-empty >>> + * >>> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME >>> + * defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to >>> + * "default" >> >> I like the profile support, is the plan for >> $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the >> database? > > Yes, with "NOTMUCH_PROFILE=default" by default. > >> It's nice that the environment variable handling is done in the library, >> should make it more consistent for bindings. As long as it can be >> overwritten I guess. > > Overwritten how? By passing parameters? yes, that's what I meant. Which I think you design here allows. Just takes a while to figure out what the right parameter combination is... ;) >> The API is rather complex though, perhaps easier when split across >> several functions? Maybe a notmuch_database_open_profile(const char >> *profile, notmuch_database_t**) is useful as the simple one which always >> does the right thing when called with NULL for profile. Not sure what >> other combinations would be needed. > > I have no objections to a "do the write thing" wrapper or two. I don't > think that increases maintence cost too much. > > d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
On Sun 05 Jul 2020 at 08:17 -0300, David Bremner wrote: > David Bremner writes: > >> Floris Bruynooghe writes: >> >>> notmuch_database_get_version currently returns and unsigned int and >>> segfaults on use with a closed db. >> >> Yes, the ones without a proper status value are going to be a bit work. >> >> In the next series I just posted [1], I started providing status value >> returning version (see notmuch_message_get_flag_st). We've been through >> a few of these migrations and it has not been too painful. >> > > I thought of another variation for the boolean valued functions. We > could embed the boolean values in the notmuch_status_t value by adding > one or more new status values corresponding to TRUE and FALSE. I'm not > sure if that would be much simpler, but it would avoid the use of output > parameters. This also seems very reasonable. ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
On Sat 04 Jul 2020 at 14:17 -0300, David Bremner wrote: > Floris Bruynooghe writes: > > >>> - * 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). >>> + * This function will return NULL if triggers an unhandled Xapian >>> + * exception. > >> How much of a departure from the existing API is this? Will this be >> possible with all functions? I had a quick look and tried some other >> functions that don't return notmuch_status_t: > > It's upward compatible in that any code which crashes because it was not > expecting a NULL pointer, will already be crashing in the same > circumstances because of an uncaught exception / call to abort. Oh yes, that is a very good point. This choice seems very reason then. Cheers, Floris ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
