From 8caf504381d80b07cd58d4125f776bfb172c54e0 Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Wed, 9 Aug 2023 20:09:46 +0300 Subject: [PATCH] store: update "move" and some related APIs Update test cases as well. --- lib/mu-server.cc | 53 +++++----- lib/mu-store.cc | 170 +++++++++++++++---------------- lib/mu-store.hh | 37 ++++--- lib/tests/test-mu-store-query.cc | 6 +- lib/tests/test-mu-store.cc | 44 ++++---- 5 files changed, 162 insertions(+), 148 deletions(-) diff --git a/lib/mu-server.cc b/lib/mu-server.cc index 7c75c067..402f7bd5 100644 --- a/lib/mu-server.cc +++ b/lib/mu-server.cc @@ -621,7 +621,7 @@ determine_docids(const Store& store, const Command& cmd) if (docid != 0) return {static_cast(docid)}; else - return unwrap(store.find_docids_with_message_id(msgid)); + return store.find_duplicates(msgid); } size_t @@ -881,12 +881,14 @@ Server::Private::perform_move(Store::Id docid, /* 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(); + const auto ids{unwrap(store().move_message(docid, maildir, flags, move_opts))}; - for (auto&&[id, msg]: *idmsgvec) { - auto sexpstr = "(:update " + msg_sexp_str(idmsgvec->at(0).second, id, {}); + for (auto&& id: ids) { + auto idmsg{store().find_message(id)}; + if (!idmsg) + mu_warning("failed to find message for id {}", id); + + auto sexpstr = "(:update " + msg_sexp_str(*idmsg, id, {}); /* note, the :move t thing is a hint to the frontend that it * could remove the particular header */ if (different_mdir) @@ -1052,29 +1054,30 @@ Server::Private::sent_handler(const Command& cmd) void Server::Private::view_mark_as_read(Store::Id docid, Message&& msg, bool rename) { + auto new_flags = [](const Message& m)->Option { + auto nflags = flags_from_delta_expr("+S-u-N", m.flags()); + if (!nflags || nflags == m.flags()) + return Nothing; // nothing to do + else + return nflags; + }; - 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&& nflags = new_flags(msg); + if (!nflags) { // nothing to move, just send the message for viewing. + output(mu_format("(:view {})", msg_sexp_str(msg, docid, {}))); + return; + } - if (!move_res) - throw move_res.error(); + // move message + dups, present results. - for (auto&& [id, msg]: move_res.value()) + Store::MoveOptions move_opts{Store::MoveOptions::DupFlags}; + if (rename) + move_opts |= Store::MoveOptions::ChangeName; + auto&& ids = unwrap(store().move_message(docid, {}, nflags, move_opts)); + for (auto&& [id, moved_msg]: store().find_messages(ids)) { output(mu_format("({} {})", id == docid ? ":view" : ":update", - msg_sexp_str(msg, id, {}))); + msg_sexp_str(moved_msg, id, {}))); + } } void diff --git a/lib/mu-store.cc b/lib/mu-store.cc index 971900de..c0236c2d 100644 --- a/lib/mu-store.cc +++ b/lib/mu-store.cc @@ -133,9 +133,13 @@ struct Store::Private { } Option find_message_unlocked(Store::Id docid) const; + Store::IdVec find_duplicates_unlocked(const Store& store, + const std::string& message_id) const; + Result add_message_unlocked(Message& msg); 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, @@ -191,6 +195,31 @@ Store::Private::find_message_unlocked(Store::Id docid) const return Some(std::move(*msg)); } +Store::IdVec +Store::Private::find_duplicates_unlocked(const Store& store, + const std::string& message_id) const +{ + if (message_id.empty() || message_id.size() > MaxTermLength) { + mu_warning("invalid message-id '{}'", message_id); + return {}; + } + + auto expr{mu_format("{}:{}", + field_from_id(Field::Id::MessageId).shortcut, + message_id)}; + if (auto&& res{store.run_query(expr)}; !res) { + mu_warning("error finding message-ids: {}", res.error().what()); + return {}; + + } else { + Store::IdVec ids; + ids.reserve(res->size()); + for (auto&& mi: *res) + ids.emplace_back(mi.doc_id()); + return ids; + } +} + Store::Store(const std::string& path, Store::Options opts) : priv_{std::make_unique(path, none_of(opts & Store::Options::Writable))} @@ -375,15 +404,29 @@ Store::find_message(Store::Id docid) const return priv_->find_message_unlocked(docid); } +Store::IdMessageVec +Store::find_messages(IdVec ids) const +{ + std::lock_guard guard{priv_->lock_}; + + IdMessageVec id_msgs; + for (auto&& id: ids) { + if (auto&& msg{priv_->find_message_unlocked(id)}; msg) + id_msgs.emplace_back(std::make_pair(id, std::move(*msg))); + } + + return id_msgs; +} + /** * 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 + * @param target_mdir target_mdir (or Nothing for current) + * @param new_flags new flags (or Nothing) + * @param opts move_options * * @return the Message after the moving, or an Error */ @@ -393,9 +436,9 @@ Store::Private::move_message_unlocked(Message&& msg, Option new_flags, MoveOptions opts) { - 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 = @@ -422,122 +465,71 @@ Store::Private::move_message_unlocked(Message&& msg, 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) { - mu_warning("invalid message-id '{}'", msgid.c_str()); - return {}; - } else if (msgid.empty()) - return {}; - - constexpr auto xprefix{field_from_id(Field::Id::MessageId).shortcut}; - auto expr{mu_format("{}:{}", xprefix, - to_string_gchar(g_ascii_strdown(msgid.c_str(), -1)))}; - - const auto res{store.run_query(expr, {}, QueryFlags::None, max)}; - if (!res) { - mu_warning("failed to run message-id-query: {}", res.error().what()); - return {}; - } - if (res->empty()) { - mu_warning("could not find message(s) for msgid {}", msgid); - return {}; - } - - Store::IdMessageVec imvec; - for (auto&& mi : *res) - imvec.emplace_back(std::make_pair(mi.doc_id(), mi.message().value())); - - return imvec; -} - - -static Result -message_id_query(const std::string& msgid) -{ - if (msgid.empty()) - return Err(Error::Code::InvalidArgument, "empty message-id"); - else if (msgid.size() > MaxTermLength) - return Err(Error::Code::InvalidArgument, "message-id too long"); - else - return Ok(mu_format("{}:{}", field_from_id(Field::Id::MessageId).shortcut, msgid)); -} - - -Result -Store::find_docids_with_message_id(const std::string& msgid) const +Store::IdVec +Store::find_duplicates(const std::string& message_id) const { std::lock_guard guard{priv_->lock_}; - if (auto&& query{message_id_query(msgid)}; !query) - return Err(std::move(query.error())); - else if (auto&& res{run_query(*query)}; !res) - return Err(std::move(res.error())); - else { - Store::IdVec ids; - for (auto&& mi: *res) - ids.emplace_back(mi.doc_id()); - return Ok(std::move(ids)); - } + return priv_->find_duplicates_unlocked(*this, message_id); } -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 +Result Store::move_message(Store::Id id, Option target_mdir, Option new_flags, MoveOptions opts) { + auto filter_dup_flags=[](Flags old_flags, Flags new_flags) -> 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; + }; + std::lock_guard guard{priv_->lock_}; auto msg{priv_->find_message_unlocked(id)}; if (!msg) return Err(Error::Code::Store, "cannot find message <{}>", id); - auto res{priv_->move_message_unlocked(std::move(*msg), target_mdir, new_flags, opts)}; + const auto message_id{msg->message_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)); + IdVec ids{id}; + if (none_of(opts & Store::MoveOptions::DupFlags) || message_id.empty() || !new_flags) + return Ok(std::move(ids)); /* 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())) { + auto dups{priv_->find_duplicates_unlocked(*this, message_id)}; + for (auto&& dupid: dups) { - if (docid == id) + if (dupid == id) continue; // already - /* For now, don't change Draft/Flagged/Trashed */ - Flags dup_flags = filter_dup_flags(msg.flags(), *new_flags); + auto dup_msg{priv_->find_message_unlocked(dupid)}; + if (!dup_msg) + continue; // no such message + /* For now, don't change Draft/Flagged/Trashed */ + const auto dup_flags{filter_dup_flags(dup_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 + if (auto dup_res = priv_->move_message_unlocked( + std::move(*dup_msg), Nothing, + dup_flags, + Store::MoveOptions::None); !dup_res) mu_warning("failed to move dup: {}", dup_res.error().what()); + else + ids.emplace_back(dupid); } - return Ok(std::move(imvec)); + return Ok(std::move(ids)); } time_t diff --git a/lib/mu-store.hh b/lib/mu-store.hh index 047ef84a..c040aa71 100644 --- a/lib/mu-store.hh +++ b/lib/mu-store.hh @@ -42,8 +42,9 @@ namespace Mu { class Store { public: - using Id = Xapian::docid; /**< Id for a message in the store */ - static constexpr Id InvalidId = 0; /**< Invalid store id */ + using Id = Xapian::docid; /**< Id for a message in the store */ + static constexpr Id InvalidId = 0; /**< Invalid store id */ + using IdVec = std::vector; /**< Vector of document ids */ /** * Configuration options. @@ -257,17 +258,26 @@ public: */ Option find_message(Id id) const; - using IdVec = std::vector; /** - * Get the doc-ids for messages with the given message-id + * Find the messages for the given ids * - * @param msg_id a message id - * @param max_results maximum number of results + * @param ids document ids for the message * - * @return either an Error or a vector of docids + * @return id, message pairs for the messages found + * (which not necessarily _all_ of the ids) */ - Result find_docids_with_message_id(const std::string& msg_id) const; + using IdMessageVec = std::vector>; + IdMessageVec find_messages(IdVec ids) const; + + /** + * Find the ids for all messages with a give message-id + * + * @param message_id a message id + * + * @return the ids of all messages with the given message-id + */ + IdVec find_duplicates(const std::string& message_id) const; /** * does a certain message exist in the store already? @@ -299,16 +309,15 @@ public: * @param new_flags new flags (if any) * @param change_name whether to change the name * - * @return Result, either a vec of for the moved + * @return Result, either an IdVec with ids 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. */ - using IdMessageVec = std::vector>; - Result move_message(Store::Id id, - Option target_mdir = Nothing, - Option new_flags = Nothing, - MoveOptions opts = MoveOptions::None); + Result move_message(Store::Id id, + Option target_mdir = Nothing, + Option new_flags = Nothing, + MoveOptions opts = MoveOptions::None); /** * Prototype for the ForEachMessageFunc diff --git a/lib/tests/test-mu-store-query.cc b/lib/tests/test-mu-store-query.cc index 27133239..4a8f632a 100644 --- a/lib/tests/test-mu-store-query.cc +++ b/lib/tests/test-mu-store-query.cc @@ -574,8 +574,11 @@ Boo! 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}; + auto&& moved_msg_opt = store.find_message(moved_msgs->at(0)); + g_assert_true(!!moved_msg_opt); + const auto&moved_msg = std::move(*moved_msg_opt); const auto new_path = moved_msg.path(); if (!rename) assert_equal(new_path, store.root_maildir() + "/inbox/cur/msg:2,S"); @@ -586,7 +589,6 @@ 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()}; - //std::cerr << "@@ " << *moved_msg << '\n'; g_assert_true(moved_sexp.plistp()); g_assert_true(!!moved_sexp.get_prop(":path")); assert_equal(moved_sexp.get_prop(":path").value().string(), new_path); diff --git a/lib/tests/test-mu-store.cc b/lib/tests/test-mu-store.cc index 9c28ee82..fbf553e8 100644 --- a/lib/tests/test-mu-store.cc +++ b/lib/tests/test-mu-store.cc @@ -381,14 +381,16 @@ Yes, that would be excellent. 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}; + auto&& msg3_opt{store->find_message(msgs3->at(0))}; + g_assert_true(!!msg3_opt); + auto&& msg3{std::move(*msg3_opt)}; + 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_assert_cmpuint(store->size(), ==, 1); + g_debug("%s", msg3.sexp().to_string().c_str()); g_assert_cmpuint(store->size(), ==, 1); } @@ -413,12 +415,13 @@ Yes, that would be excellent. 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"; + auto msg1_path = join_paths(tempdir2.path(), "Maildir/a/new/msg123"); + auto msg2_path = join_paths(tempdir2.path(), "Maildir/a/cur/msgabc:2,S"); + auto msg3_path = join_paths(tempdir2.path(),"Maildir/b/cur/msgdef:2,RS"); TempDir tempdir; - auto store{Store::make_new(tempdir.path(), tempdir2.path() + "/Maildir")}; + auto store{Store::make_new(tempdir.path(), + join_paths(tempdir2.path() , "Maildir"))}; assert_valid_result(store); std::vector ids; @@ -437,12 +440,18 @@ Yes, that would be excellent. Flags::Seen | Flags::Flagged | Flags::Passed, Store::MoveOptions::DupFlags); assert_valid_result(mres); + mu_info("found {} matches", mres->size()); + for (auto&& m: *mres) + mu_info("id: {}", m); + // al three dups should have been updated g_assert_cmpuint(mres->size(), ==, 3); + auto&& id_msgs{store->find_messages(*mres)}; + // first should be the original - g_assert_cmpuint(mres->at(0).first, ==, ids.at(0)); + g_assert_cmpuint(id_msgs.at(0).first, ==, ids.at(0)); { // Message 1 - const Message& msg = mres->at(0).second; + const Message& msg = id_msgs.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)); } @@ -450,19 +459,18 @@ Yes, that would be excellent. // msg3 should loose its R flag. auto check_msg2 = [&](const Message& msg) { - assert_equal(msg.path(), tempdir2.path() + "/Maildir/a/cur/msgabc:2,PS"); + assert_equal(msg.path(), join_paths(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"); + assert_equal(msg.path(), join_paths(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); + if (id_msgs.at(1).first == ids.at(1)) { + check_msg2(id_msgs.at(1).second); + check_msg3(id_msgs.at(2).second); } else { - check_msg2(mres->at(2).second); - check_msg3(mres->at(1).second); + check_msg2(id_msgs.at(2).second); + check_msg3(id_msgs.at(1).second); } } @@ -525,8 +533,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/move-dups", test_store_move_dups); 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/circular-symlink", test_store_circular_symlink); g_test_add_func("/store/index/fail", test_store_fail);