[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, &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] 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', &out); +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