From 62cfc889509bdfc101ca9d59e46aaafce76789b8 Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Wed, 7 Dec 2022 12:29:15 +0200 Subject: [PATCH 1/7] flags: add flags_keep_unmutable + test When moving we want to maintain _some_ flags; add a function making that convenient. --- lib/message/mu-flags.cc | 14 ++++++++++++++ lib/message/mu-flags.hh | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/lib/message/mu-flags.cc b/lib/message/mu-flags.cc index 549fa2ee..bd3e57e6 100644 --- a/lib/message/mu-flags.cc +++ b/lib/message/mu-flags.cc @@ -150,6 +150,18 @@ test_flags_filter() } + +[[maybe_unused]] static void +test_flags_keep_unmutable() +{ + static_assert(flags_keep_unmutable((Flags::Seen|Flags::Passed), + (Flags::Flagged|Flags::Draft), + Flags::Replied) == + (Flags::Flagged|Flags::Draft)); +} + + + #ifdef BUILD_TESTS int main(int argc, char* argv[]) @@ -164,6 +176,8 @@ main(int argc, char* argv[]) test_flags_from_delta_expr); g_test_add_func("/message/flags/flags-filter", test_flags_filter); + g_test_add_func("/message/flags/flags-keep-unmutable", + test_flags_keep_unmutable); return g_test_run(); } diff --git a/lib/message/mu-flags.hh b/lib/message/mu-flags.hh index 0ac6496d..2ef1feb2 100644 --- a/lib/message/mu-flags.hh +++ b/lib/message/mu-flags.hh @@ -338,6 +338,28 @@ flags_filter(Flags flags, MessageFlagCategory cat) return flags; } + + +/** + * Return flags, where flags = new_flags but with unmutable_flag in the + * result the same as in old_flags + * + * @param old_flags + * @param new_flags + * @param unmutable_flag + * + * @return + */ +constexpr Flags +flags_keep_unmutable(Flags old_flags, Flags new_flags, Flags unmutable_flag) +{ + if (any_of(old_flags & unmutable_flag)) + return new_flags | unmutable_flag; + else + return new_flags & ~unmutable_flag; +} + + /** * Get a string representation of flags * From 87c3ceb7b19abebc88136c16332c421a8dfc8a72 Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Sat, 3 Dec 2022 16:42:19 +0200 Subject: [PATCH 2/7] store: update move_message API Update the move_message API so to allow for updating duplicate messages too (not implemented yet), and return all updated messages. --- lib/mu-store.cc | 142 +++++++++++++++++++++++++++++++++++++++++------- lib/mu-store.hh | 31 ++++++++--- 2 files changed, 147 insertions(+), 26 deletions(-) diff --git a/lib/mu-store.cc b/lib/mu-store.cc index 2ae1a1e8..d135febf 100644 --- a/lib/mu-store.cc +++ b/lib/mu-store.cc @@ -244,6 +244,10 @@ struct Store::Private { Option find_message_unlocked(Store::Id docid) const; Result update_message_unlocked(Message& msg, Store::Id docid); Result update_message_unlocked(Message& msg, const std::string& old_path); + Result move_message_unlocked(Message&& msg, + Option target_mdir, + Option new_flags, + MoveOptions opts); /* metadata to write as part of a transaction commit */ std::unordered_map metadata_cache_; @@ -503,44 +507,144 @@ Store::find_message(Store::Id docid) const return priv_->find_message_unlocked(docid); } - +/** + * Move a message in store and filesystem. + * + * Lock is assumed taken already + * + * @param id message id + * @param target_mdir target_midr (or Nothing for current) + * @param new_flags new flags (or Notthing) +'; * @param opts move_optionss + * + * @return the Message after the moving, or an Error + */ Result -Store::move_message(Store::Id id, - Option target_mdir, - Option new_flags, bool change_name) +Store::Private::move_message_unlocked(Message&& msg, + Option target_mdir, + Option new_flags, + MoveOptions opts) { - std::lock_guard guard{priv_->lock_}; - - auto msg = priv_->find_message_unlocked(id); - if (!msg) - return Err(Error::Code::Store, "cannot find message <%u>", id); - - const auto old_path = msg->path(); - const auto target_flags = new_flags.value_or(msg->flags()); - const auto target_maildir = target_mdir.value_or(msg->maildir()); + const auto old_path = msg.path(); + const auto target_flags = new_flags.value_or(msg.flags()); + const auto target_maildir = target_mdir.value_or(msg.maildir()); /* 1. first determine the file system path of the target */ const auto target_path = - maildir_determine_target(msg->path(), properties().root_maildir, - target_maildir,target_flags, change_name); + maildir_determine_target(msg.path(), properties_.root_maildir, + target_maildir, target_flags, + any_of(opts & MoveOptions::ChangeName)); if (!target_path) return Err(target_path.error()); /* 2. let's move it */ - if (const auto res = maildir_move_message(msg->path(), target_path.value()); !res) + if (const auto res = maildir_move_message(msg.path(), target_path.value()); !res) return Err(res.error()); /* 3. file move worked, now update the message with the new info.*/ - if (auto&& res = msg->update_after_move( + if (auto&& res = msg.update_after_move( target_path.value(), target_maildir, target_flags); !res) return Err(res.error()); /* 4. update message worked; re-store it */ - if (auto&& res = priv_->update_message_unlocked(*msg, old_path); !res) + if (auto&& res = update_message_unlocked(msg, old_path); !res) return Err(res.error()); /* 6. Profit! */ - return Ok(std::move(msg.value())); + return Ok(std::move(msg)); +} + + + +/* get a vec of all messages with the given message id */ +static Store::IdMessageVec +messages_with_msgid(const Store& store, const std::string& msgid, size_t max=100) +{ + if (msgid.size() > MaxTermLength) { + g_warning("invalid message-id '%s'", msgid.c_str()); + return {}; + } else if (msgid.empty()) + return {}; + + const auto xprefix{field_from_id(Field::Id::MessageId).shortcut}; + /*XXX this is a bit dodgy */ + auto tmp{g_ascii_strdown(msgid.c_str(), -1)}; + auto expr{g_strdup_printf("%c:%s", xprefix, tmp)}; + g_free(tmp); + + const auto res{store.run_query(expr, {}, QueryFlags::None, max)}; + g_free(expr); + if (!res) { + g_warning("failed to run message-id-query: %s", res.error().what()); + return {}; + } + if (res->empty()) { + g_warning("could not find message(s) for msgid %s", msgid.c_str()); + return {}; + } + + Store::IdMessageVec imvec; + for (auto&& mi : *res) + imvec.emplace_back(std::make_pair(mi.doc_id(), mi.message().value())); + + return imvec; +} + + +static Flags +filter_dup_flags(Flags old_flags, Flags new_flags) +{ + new_flags = flags_keep_unmutable(old_flags, new_flags, Flags::Draft); + new_flags = flags_keep_unmutable(old_flags, new_flags, Flags::Flagged); + new_flags = flags_keep_unmutable(old_flags, new_flags, Flags::Trashed); + + return new_flags; +} + +Result +Store::move_message(Store::Id id, + Option target_mdir, + Option new_flags, + MoveOptions opts) +{ + std::lock_guard guard{priv_->lock_}; + + auto msg{priv_->find_message_unlocked(id)}; + if (!msg) + return Err(Error::Code::Store, "cannot find message <%u>", id); + + auto res{priv_->move_message_unlocked(std::move(*msg), target_mdir, new_flags, opts)}; + if (!res) + return Err(res.error()); + + IdMessageVec imvec; + imvec.emplace_back(std::make_pair(id, std::move(*res))); + if (none_of(opts & Store::MoveOptions::DupFlags) || !new_flags) + return Ok(std::move(imvec)); + + /* handle the dupflags case; i.e. apply (a subset of) the flags to + * all messages with the same message-id as well */ + for (auto&& [docid, msg]: messages_with_msgid(*this, imvec.at(0).second.message_id())) { + + if (docid == id) + continue; // already + + /* For now, don't change Draft/Flagged/Trashed */ + Flags dup_flags = filter_dup_flags(msg.flags(), *new_flags); + + /* use the updated new_flags and default MoveOptions (so we don't recurse, nor do we + * change the base-name of moved messages) */ + auto dup_res = priv_->move_message_unlocked(std::move(msg), Nothing, + dup_flags, + Store::MoveOptions::None); + // just log a warning if it fails, but continue. + if (dup_res) + imvec.emplace_back(docid, std::move(*dup_res)); + else + g_warning("failed to move dup: %s", dup_res.error().what()); + } + + return Ok(std::move(imvec)); } std::string diff --git a/lib/mu-store.hh b/lib/mu-store.hh index 0a745a6f..11dc0c64 100644 --- a/lib/mu-store.hh +++ b/lib/mu-store.hh @@ -307,21 +307,37 @@ public: */ bool contains_message(const std::string& path) const; + /** - * Move a message both in the filesystem and in the store. - * After a successful move, the message is updated. + * Options for moving + * + */ + enum struct MoveOptions { + None = 0, /**< Defaults */ + ChangeName = 1 << 0, /**< Change the name when moving */ + DupFlags = 1 << 1, /**< Update flags for duplicate messages too*/ + }; + + + /** + * Move a message both in the filesystem and in the store. After a + * successful move, the message is updated. * * @param id the id for some message * @param target_mdir the target maildir (if any) * @param new_flags new flags (if any) * @param change_name whether to change the name * - * @return Result, either the moved message or some error. + * @return Result, either a vec of for the moved + * message(s) or some error. Note that in case of success at least one + * message is returned, and only with MoveOptions::DupFlags can it be + * more than one. */ - Result move_message(Store::Id id, - Option target_mdir = Nothing, - Option new_flags = Nothing, - bool change_name = false); + using IdMessageVec = std::vector>; + Result move_message(Store::Id id, + Option target_mdir = Nothing, + Option new_flags = Nothing, + MoveOptions opts = MoveOptions::None); /** * Prototype for the ForEachMessageFunc @@ -466,6 +482,7 @@ private: }; MU_ENABLE_BITOPS(Store::Options); +MU_ENABLE_BITOPS(Store::MoveOptions); } // namespace Mu From 0b516c18c29da935b0971f56863e12e2ee4a623c Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Wed, 7 Dec 2022 12:31:02 +0200 Subject: [PATCH 3/7] store: add mu_move_message dup flag test Test the new functionality --- lib/tests/test-mu-store.cc | 79 +++++++++++++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/lib/tests/test-mu-store.cc b/lib/tests/test-mu-store.cc index 8369a874..4f4f577d 100644 --- a/lib/tests/test-mu-store.cc +++ b/lib/tests/test-mu-store.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -384,6 +385,81 @@ Yes, that would be excellent. } + +static void +test_store_move_dups() +{ + const std::string msg_text = +R"(From: Valentine Michael Smith +To: Raul Endymion +Subject: Re: multi-eq hash tables +Date: Tue, 03 May 2022 20:58:02 +0200 +Message-ID: <87h766tzzz.fsf@gnus.org> + +Yes, that would be excellent. +)"; + TempDir tempdir2; + + // create a message file + dups + const auto res1 = maildir_mkdir(tempdir2.path() + "/Maildir/a"); + assert_valid_result(res1); + const auto res2 = maildir_mkdir(tempdir2.path() + "/Maildir/b"); + assert_valid_result(res2); + + auto msg1_path = tempdir2.path() + "/Maildir/a/new/msg123"; + auto msg2_path = tempdir2.path() + "/Maildir/a/cur/msgabc:2,S"; + auto msg3_path = tempdir2.path() + "/Maildir/b/cur/msgdef:2,RS"; + + TempDir tempdir; + auto store{Store::make_new(tempdir.path(), tempdir2.path() + "/Maildir", {}, {})}; + assert_valid_result(store); + + std::vector ids; + for (auto&& p: {msg1_path, msg2_path, msg3_path}) { + std::ofstream output{p}; + output.write(msg_text.c_str(), msg_text.size()); + output.close(); + auto res = store->add_message(p); + assert_valid_result(res); + ids.emplace_back(*res); + } + g_assert_cmpuint(store->size(), ==, 3); + + // mark main message (+ dups) as seen + auto mres = store->move_message(ids.at(0), {}, + Flags::Seen | Flags::Flagged | Flags::Passed, + Store::MoveOptions::DupFlags); + assert_valid_result(mres); + // al three dups should have been updated + g_assert_cmpuint(mres->size(), ==, 3); + // first should be the original + g_assert_cmpuint(mres->at(0).first, ==, ids.at(0)); + { // Message 1 + const Message& msg = mres->at(0).second; + assert_equal(msg.path(), tempdir2.path() + "/Maildir/a/cur/msg123:2,FPS"); + g_assert_true(msg.flags() == (Flags::Seen|Flags::Flagged|Flags::Passed)); + } + // note: Seen and Passed should be added to msg2/3, but Flagged shouldn't + // msg3 should loose its R flag. + + auto check_msg2 = [&](const Message& msg) { + assert_equal(msg.path(), tempdir2.path() + "/Maildir/a/cur/msgabc:2,PS"); + }; + + auto check_msg3 = [&](const Message& msg) { + assert_equal(msg.path(), tempdir2.path() + "/Maildir/b/cur/msgdef:2,PS"); + }; + + if (mres->at(1).first == ids.at(1)) { + check_msg2(mres->at(1).second); + check_msg3(mres->at(2).second); + } else { + check_msg2(mres->at(2).second); + check_msg3(mres->at(1).second); + } +} + + static void test_store_fail() { @@ -412,7 +488,8 @@ main(int argc, char* argv[]) test_message_mailing_list); g_test_add_func("/store/message/attachments", test_message_attachments); - g_test_add_func("/store/index/move", test_index_move); + g_test_add_func("/store/index/index-move", test_index_move); + g_test_add_func("/store/index/move-dups", test_store_move_dups); g_test_add_func("/store/index/fail", test_store_fail); return g_test_run(); From da7c3b0c9a1079d4654e1cc866e2edcdd8c243ee Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Sat, 3 Dec 2022 16:45:10 +0200 Subject: [PATCH 4/7] tests: update for move_message API update --- lib/tests/test-mu-store-query.cc | 11 +++++++---- lib/tests/test-mu-store.cc | 14 ++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/tests/test-mu-store-query.cc b/lib/tests/test-mu-store-query.cc index cd44bfed..051a81c8 100644 --- a/lib/tests/test-mu-store-query.cc +++ b/lib/tests/test-mu-store-query.cc @@ -565,9 +565,12 @@ Boo! /* * mark as read, i.e. move to cur/; ensure it really moved. */ - auto moved_msg = store.move_message(old_docid, Nothing, Flags::Seen, rename); - assert_valid_result(moved_msg); - const auto new_path = moved_msg->path(); + auto move_opts{rename ? Store::MoveOptions::ChangeName : Store::MoveOptions::None}; + auto moved_msgs = store.move_message(old_docid, Nothing, Flags::Seen, move_opts); + assert_valid_result(moved_msgs); + g_assert_true(moved_msgs->size() == 1); + const auto& moved_msg{moved_msgs->at(0).second}; + const auto new_path = moved_msg.path(); if (!rename) assert_equal(new_path, store.properties().root_maildir + "/inbox/cur/msg:2,S"); g_assert_cmpuint(store.size(), ==, 1); @@ -576,7 +579,7 @@ Boo! /* also ensure that the cached sexp for the message has been updated; * that's what mu4e uses */ - const auto moved_sexp{moved_msg->sexp()}; + const auto moved_sexp{moved_msg.sexp()}; //std::cerr << "@@ " << *moved_msg << '\n'; g_assert_true(moved_sexp.plistp()); g_assert_true(moved_sexp.has_prop(":path")); diff --git a/lib/tests/test-mu-store.cc b/lib/tests/test-mu-store.cc index 4f4f577d..7801ad06 100644 --- a/lib/tests/test-mu-store.cc +++ b/lib/tests/test-mu-store.cc @@ -373,14 +373,16 @@ Yes, that would be excellent. // Move the message from new->cur std::this_thread::sleep_for(1s); /* ctime should change */ - const auto msg3 = store->move_message(msg->docid(), {}, Flags::Seen); - assert_valid_result(msg3); - assert_equal(msg3->maildir(), "/a"); - assert_equal(msg3->path(), tempdir2.path() + "/Maildir/a/cur/msg:2,S"); - g_assert_true(::access(msg3->path().c_str(), R_OK)==0); + const auto msgs3 = store->move_message(msg->docid(), {}, Flags::Seen); + assert_valid_result(msgs3); + g_assert_true(msgs3->size() == 1); + const auto& msg3{msgs3->at(0).second}; + assert_equal(msg3.maildir(), "/a"); + assert_equal(msg3.path(), tempdir2.path() + "/Maildir/a/cur/msg:2,S"); + g_assert_true(::access(msg3.path().c_str(), R_OK)==0); g_assert_false(::access(oldpath.c_str(), R_OK)==0); - g_debug("%s", msg3->sexp().to_string().c_str()); + g_debug("%s", msg3.sexp().to_string().c_str()); g_assert_cmpuint(store->size(), ==, 1); } From b71751a185bbf9c09d182d32cbd5764fcaa4c02e Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Sat, 3 Dec 2022 16:45:41 +0200 Subject: [PATCH 5/7] mu-server: update for move_message API update --- lib/mu-server.cc | 119 ++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 64 deletions(-) diff --git a/lib/mu-server.cc b/lib/mu-server.cc index b14ce570..3457f4c2 100644 --- a/lib/mu-server.cc +++ b/lib/mu-server.cc @@ -109,17 +109,17 @@ private: Store::Id docid, const Option qm) const; - Sexp move_docid(Store::Id docid, Option flagstr, + void move_docid(Store::Id docid, Option flagstr, bool new_name, bool no_view); - Sexp perform_move(Store::Id docid, - const Message& msg, + void perform_move(Store::Id docid, + const Message& msg, const std::string& maildirarg, Flags flags, bool new_name, bool no_view); - bool view_mark_as_read(Store::Id docid, const Message& msg, bool rename); + void view_mark_as_read(Store::Id docid, Message&& msg, bool rename); Store& store_; Server::Output output_; @@ -797,7 +797,7 @@ Server::Private::mkdir_handler(const Command& cmd) ":message", format("%s has been created", path.c_str()))); } -Sexp +void Server::Private::perform_move(Store::Id docid, const Message& msg, const std::string& maildirarg, @@ -813,20 +813,26 @@ Server::Private::perform_move(Store::Id docid, } else /* are we moving to a different mdir, or is it just flags? */ different_mdir = maildir != msg.maildir(); - const auto new_msg = store().move_message(docid, maildir, flags, new_name); - if (!new_msg) - throw new_msg.error(); + Store::MoveOptions move_opts{Store::MoveOptions::DupFlags}; + if (new_name) + move_opts |= Store::MoveOptions::ChangeName; - Sexp seq; - seq.put_props(":update", build_message_sexp(new_msg.value(), docid, {})); - /* note, the :move t thing is a hint to the frontend that it - * could remove the particular header */ - if (different_mdir) - seq.put_props(":move", Sexp::t()); - if (!no_view) - seq.put_props(":maybe-view", Sexp::t()); + /* note: we get back _all_ the messages that changed; the first is the + * primary mover; the rest (if present) are any dups affected */ + const auto idmsgvec{store().move_message(docid, maildir, flags, move_opts)}; + if (!idmsgvec) + throw idmsgvec.error(); - return seq; + for (auto&&[id, msg]: *idmsgvec) { + Sexp sexp{":update"_sym, build_message_sexp(idmsgvec->at(0).second, id, {})}; + /* note, the :move t thing is a hint to the frontend that it + * could remove the particular header */ + if (different_mdir) + sexp.put_props(":move", Sexp::t()); + if (!no_view && id == docid) + sexp.put_props(":maybe-view", Sexp::t()); + output_sexp(std::move(sexp)); + } } @@ -847,7 +853,7 @@ calculate_message_flags(const Message& msg, Option flagopt) return flags.value(); } -Sexp +void Server::Private::move_docid(Store::Id docid, Option flagopt, bool new_name, @@ -861,9 +867,7 @@ Server::Private::move_docid(Store::Id docid, throw Error{Error::Code::Store, "failed to get message from store"}; const auto flags = calculate_message_flags(msg.value(), flagopt); - auto lst = perform_move(docid, *msg, "", flags, new_name, no_view); - - return lst; + perform_move(docid, *msg, "", flags, new_name, no_view); } /* @@ -871,9 +875,6 @@ Server::Private::move_docid(Store::Id docid, * flags. parameters are *either* a 'docid:' or 'msgid:' pointing to * the message, a 'maildir:' for the target maildir, and a 'flags:' * parameter for the new flags. - * - * returns an (:update ) - * */ void Server::Private::move_handler(const Command& cmd) @@ -890,8 +891,7 @@ Server::Private::move_handler(const Command& cmd) "can't move multiple messages at the same time"}; // multi. for (auto&& docid : docids) - output_sexp(move_docid(docid, flagopt, - rename, no_view)); + move_docid(docid, flagopt, rename, no_view); return; } auto docid{docids.at(0)}; @@ -907,7 +907,7 @@ Server::Private::move_handler(const Command& cmd) * we received (ie., flagstr), if any, plus the existing message * flags. */ const auto flags = calculate_message_flags(msg, flagopt); - output_sexp(perform_move(docid, msg, maildir, flags, rename, no_view)); + perform_move(docid, msg, maildir, flags, rename, no_view); } void @@ -965,7 +965,7 @@ Server::Private::remove_handler(const Command& cmd) if (!store().remove_message(path)) g_warning("failed to remove message @ %s (%d) from store", path.c_str(), docid); - output_sexp(Sexp().put_props(":remove", docid)); // act as if it worked. + output_sexp(Sexp().put_props(":remove", docid)); // act as if it worked. } void @@ -982,43 +982,32 @@ Server::Private::sent_handler(const Command& cmd) ":docid", docid.value())); } -bool -Server::Private::view_mark_as_read(Store::Id docid, const Message& msg, bool rename) +void +Server::Private::view_mark_as_read(Store::Id docid, Message&& msg, bool rename) { - /* move some message if the flags changes; and send either a :view (main message - * or :update (the rest))*/ - auto maybe_move = [&](Store::Id msg_docid, Flags old_flags, - bool do_rename, bool do_view)->bool { - const auto newflags{flags_from_delta_expr("+S-u-N", old_flags)}; - if (!newflags || old_flags == *newflags) - return false; + auto move_res = std::invoke([&]()->Result { + const auto newflags{flags_from_delta_expr("+S-u-N", msg.flags())}; + if (!newflags || msg.flags() == *newflags) { + /* case 1: message was already read; do nothing */ + Store::IdMessageVec idmvec; + idmvec.emplace_back(docid, std::move(msg)); + return idmvec; + } else { + /* case 2: move message (and possibly dups) */ + Store::MoveOptions move_opts{Store::MoveOptions::DupFlags}; + if (rename) + move_opts |= Store::MoveOptions::ChangeName; + return store().move_message(docid, {}, newflags, move_opts); + } + }); - auto updated_msg = store().move_message(msg_docid, {}, newflags, do_rename); - if (!updated_msg) - throw updated_msg.error(); + if (!move_res) + throw move_res.error(); - output_sexp(Sexp().put_props(do_view ? ":view" : ":update", - build_message_sexp(*updated_msg, docid, {}))); - return true; - }; - - /* now get _al_ the message-ids for the given message-id, - * since, we want to apply the read-status to _all_. */ - - /* first the main message */ - bool moved = maybe_move(docid, msg.flags(), rename, true/*:view*/); - - /* now any other message with the same message-id */ - for (auto&& rel_docid: docids_for_msgid(store_, msg.message_id())) { - /* ignore main one since we already handled it. */ - if (rel_docid == docid) - continue; - if (auto msg{store().find_message(docid)}; msg) - maybe_move(rel_docid, msg->flags(), rename, false/*:update*/); - } - - return moved; + for (auto&& [id, msg]: move_res.value()) + output_sexp(Sexp{id == docid ? ":view"_sym : ":update"_sym, + build_message_sexp(msg, id, {})}); } void @@ -1038,10 +1027,12 @@ Server::Private::view_handler(const Command& cmd) .or_else([]{throw Error{Error::Code::Store, "failed to find message for view"};}).value(); - /* if the message is marked-as-read, the response is handled there; - * otherwise, we do so here. */ - if (!mark_as_read || !view_mark_as_read(docid, msg, rename)) + /* if the message should not be marked-as-read, we're done. */ + if (!mark_as_read) output_sexp(Sexp().put_props(":view", build_message_sexp(msg, docid, {}))); + else + view_mark_as_read(docid, std::move(msg), rename); + /* otherwise, mark message and and possible dups as read */ } Server::Server(Store& store, Server::Output output) From ca05c82451f199bbb3407a50d413d4d4a0eb1ca7 Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Thu, 8 Dec 2022 19:30:20 +0200 Subject: [PATCH 6/7] query-threads: add multi-dup unit test --- lib/mu-query-threads.cc | 47 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/mu-query-threads.cc b/lib/mu-query-threads.cc index 22a3b7af..be2f68f7 100644 --- a/lib/mu-query-threads.cc +++ b/lib/mu-query-threads.cc @@ -1,5 +1,5 @@ /* -** Copyright (C) 2021 Dirk-Jan C. Binnema +** Copyright (C) 2022 Dirk-Jan C. Binnema ** ** This program is free software; you can redistribute it and/or modify it ** under the terms of the GNU General Public License as published by the @@ -661,6 +661,10 @@ assert_thread_paths(const MockQueryResults& qrs, const Expected& expected) qr.path().value_or("") == exp.first; }); g_assert_true(it != qrs.end()); + g_debug("thread-path (%s@%s): expected: '%s'; got '%s'", + it->message_id().value_or("").c_str(), + it->path().value_or("").c_str(), + exp.second.c_str(), it->query_match().thread_path.c_str()); g_assert_cmpstr(exp.second.c_str(), ==, it->query_match().thread_path.c_str()); } } @@ -754,13 +758,43 @@ test_dups_dup_first() calculate_threads(results, false); - assert_thread_paths(results, - { - {"/path2", "0"}, - {"/path1", "0:0"}, - }); + assert_thread_paths(results, { + {"/path2", "0"}, + {"/path1", "0:0"}, + }); } +static void +test_dups_dup_multi() +{ + // now dup becomes the leader; this will _demote_ + // r1. + + MockQueryResult r1_dup1{"m1", "1", {}}; + r1_dup1.query_match().flags |= QueryMatch::Flags::Duplicate; + r1_dup1.path_ = "/path1"; + + MockQueryResult r1_dup2{"m1", "1", {}}; + r1_dup2.query_match().flags |= QueryMatch::Flags::Duplicate; + r1_dup2.path_ = "/path2"; + + MockQueryResult r1{"m1", "1", {}}; + r1.query_match().flags |= QueryMatch::Flags::Leader; + r1.path_ = "/path3"; + + auto results = MockQueryResults{r1_dup1, r1_dup2, r1}; + calculate_threads(results, false); + + assert_thread_paths(results, { + {"/path3", "0"}, + {"/path1", "0:0"}, + {"/path2", "0:1"}, + }); +} + + + + static void test_do_not_prune_root_empty_with_children() { @@ -896,6 +930,7 @@ try { g_test_add_func("/threader/id-table-inconsistent", test_id_table_inconsistent); g_test_add_func("/threader/dups/dup-last", test_dups_dup_last); g_test_add_func("/threader/dups/dup-first", test_dups_dup_first); + g_test_add_func("/threader/dups/dup-multi", test_dups_dup_multi); g_test_add_func("/threader/prune/do-not-prune-root-empty-with-children", test_do_not_prune_root_empty_with_children); From d5fb15574b1cb8207152e3444472bc775548d202 Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Thu, 8 Dec 2022 19:31:07 +0200 Subject: [PATCH 7/7] mu-query-match-decider: add 'Related' to flags We were _replacing_ the flags (such as Duplicate), but we should add to them instead. Add a unit-test for this. --- lib/mu-query-match-deciders.cc | 2 +- lib/tests/test-mu-store-query.cc | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/lib/mu-query-match-deciders.cc b/lib/mu-query-match-deciders.cc index b988b970..999d6093 100644 --- a/lib/mu-query-match-deciders.cc +++ b/lib/mu-query-match-deciders.cc @@ -180,7 +180,7 @@ struct MatchDeciderRelated final : public MatchDecider { auto qm{make_query_match(doc)}; if (should_include(qm)) { - qm.flags = QueryMatch::Flags::Related; + qm.flags |= QueryMatch::Flags::Related; decider_info_.matches.emplace(doc.get_docid(), std::move(qm)); return true; } else diff --git a/lib/tests/test-mu-store-query.cc b/lib/tests/test-mu-store-query.cc index 051a81c8..e2b8f13c 100644 --- a/lib/tests/test-mu-store-query.cc +++ b/lib/tests/test-mu-store-query.cc @@ -669,6 +669,57 @@ Boo! } } +static void +test_related_dup_threaded() +{ + // test message sent to self, and copy of received msg. + + const auto test_msg = R"(From: "Edward Mallory" +To: "Laurence Oliphant +Subject: Boo +Date: Wed, 07 Dec 2022 18:38:06 +0200 +Message-ID: <875yentbhg.fsf@djcbsoftware.nl> +MIME-Version: 1.0 +Content-Type: text/plain + +Boo! +)"; + const TestMap test_msgs = { + {"sent/cur/msg1", test_msg }, + {"inbox/cur/msg1", test_msg }, + {"inbox/cur/msg2", test_msg }}; + + TempDir tdir; + auto store{make_test_store(tdir.path(), test_msgs, {})}; + + g_assert_cmpuint(store.size(), ==, 3); + + + // normal query should give 2 + { + auto qr = store.run_query("maildir:/inbox", Field::Id::Date, + QueryFlags::None); + assert_valid_result(qr); + g_assert_cmpuint(qr->size(), ==, 2); + } + + // a related query should give 3 + { + auto qr = store.run_query("maildir:/inbox", Field::Id::Date, + QueryFlags::IncludeRelated); + assert_valid_result(qr); + g_assert_cmpuint(qr->size(), ==, 3); + } + + // a related/threading query should give 3. + { + auto qr = store.run_query("maildir:/inbox", Field::Id::Date, + QueryFlags::IncludeRelated | QueryFlags::Threading); + assert_valid_result(qr); + g_assert_cmpuint(qr->size(), ==, 3); + } + +} int main(int argc, char* argv[]) @@ -692,6 +743,8 @@ main(int argc, char* argv[]) test_duplicate_refresh_rename); g_test_add_func("/store/query/term-split", test_term_split); + g_test_add_func("/store/query/related-dup-threaded", + test_related_dup_threaded); return g_test_run(); }