store: update "move" and some related APIs

Update test cases as well.

lib/mu-server.cc

@@ -621,7 +621,7 @@ determine_docids(const Store& store, const Command& cmd)
 	if (docid != 0)
 		return {static_cast<Store::Id>(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)};
+	const auto ids{unwrap(store().move_message(docid, maildir, flags, move_opts))};
-	if (!idmsgvec)
-		throw idmsgvec.error();
 
-	for (auto&&[id, msg]: *idmsgvec) {
+	for (auto&& id: ids) {
-		auto sexpstr = "(:update " + msg_sexp_str(idmsgvec->at(0).second, id, {});
+		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<Flags> {
+		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<Store::IdMessageVec> {
+	auto&& nflags = new_flags(msg);
-			const auto newflags{flags_from_delta_expr("+S-u-N", msg.flags())};
+	if (!nflags) { // nothing to move, just send the message for viewing.
-			if (!newflags || msg.flags() == *newflags) {
+		output(mu_format("(:view {})", msg_sexp_str(msg, docid, {})));
-				/* case 1: message was already read; do nothing */
+		return;
-				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);
-			}
-		});
 
-	if (!move_res)
+	// move message + dups, present results.
-		throw move_res.error();
 
-	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

lib/mu-store.cc

@@ -133,9 +133,13 @@ struct Store::Private {
 	}
 
 	Option<Message> find_message_unlocked(Store::Id docid) const;
+	Store::IdVec find_duplicates_unlocked(const Store& store,
+					      const std::string& message_id) const;
+
 	Result<Store::Id> add_message_unlocked(Message& msg);
 	Result<Store::Id> update_message_unlocked(Message& msg, Store::Id docid);
 	Result<Store::Id> update_message_unlocked(Message& msg, const std::string& old_path);
+
 	Result<Message> move_message_unlocked(Message&& msg,
 					      Option<const std::string&> target_mdir,
 					      Option<Flags> 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<Private>(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 target_mdir target_mdir (or Nothing for current)
- * @param new_flags new flags (or Notthing)
+ * @param new_flags new flags (or Nothing)
- * @param opts move_optionss
+ * @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<Flags> new_flags,
 				      MoveOptions opts)
 {
-	const auto	old_path       = msg.path();
+	const auto old_path       = msg.path();
-	const auto	target_flags   = new_flags.value_or(msg.flags());
+	const auto target_flags   = new_flags.value_or(msg.flags());
-	const auto	target_maildir = target_mdir.value_or(msg.maildir());
+	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 */
+Store::IdVec
-static Store::IdMessageVec
+Store::find_duplicates(const std::string& message_id) const
-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<std::string>
-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::IdVec>
-Store::find_docids_with_message_id(const std::string& msgid) const
 {
 	std::lock_guard guard{priv_->lock_};
 
-	if (auto&& query{message_id_query(msgid)}; !query)
+	return priv_->find_duplicates_unlocked(*this, message_id);
-		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));
-	}
 }
 
 
-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::IdVec>
-}
-
-Result<Store::IdMessageVec>
 Store::move_message(Store::Id id,
 		    Option<const std::string&> target_mdir,
 		    Option<Flags> 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;
+	IdVec ids{id};
-	imvec.emplace_back(std::make_pair(id, std::move(*res)));
+	if (none_of(opts & Store::MoveOptions::DupFlags) || message_id.empty() || !new_flags)
-	if (none_of(opts & Store::MoveOptions::DupFlags) || !new_flags)
+		return Ok(std::move(ids));
-		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())) {
+	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 */
+		auto dup_msg{priv_->find_message_unlocked(dupid)};
-		Flags dup_flags = filter_dup_flags(msg.flags(), *new_flags);
+		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,
+		if (auto dup_res = priv_->move_message_unlocked(
-							    dup_flags,
+			    std::move(*dup_msg), Nothing,
-							    Store::MoveOptions::None);
+			    dup_flags,
-		// just log a warning if it fails, but continue.
+			    Store::MoveOptions::None); !dup_res)
-		if (dup_res)
-			imvec.emplace_back(docid, std::move(*dup_res));
-		else
 			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

lib/mu-store.hh

@@ -42,8 +42,9 @@ namespace Mu {
 
 class Store {
 public:
-	using Id                      = Xapian::docid; /**< Id for a message in the store */
+	using Id                      = Xapian::docid;   /**< Id for a message in the store */
-	static constexpr Id InvalidId = 0;             /**< Invalid  store id */
+	static constexpr Id InvalidId = 0;               /**< Invalid  store id */
+	using IdVec                   = std::vector<Id>; /**< Vector of document ids */
 
 	/**
 	 * Configuration options.
@@ -257,17 +258,26 @@ public:
 	 */
 	Option<Message> find_message(Id id) const;
 
-	using IdVec = std::vector<Id>;
 
 	/**
-	 * 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 ids document ids for the message
-	 * @param max_results maximum number of results
 	 *
-	 * @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<IdVec> find_docids_with_message_id(const std::string& msg_id) const;
+	using IdMessageVec = std::vector<std::pair<Id, Message>>;
+	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 <doc-id, message> 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<std::pair<Id, Message>>;
+	Result<IdVec> move_message(Store::Id id,
-	Result<IdMessageVec> move_message(Store::Id id,
+				   Option<const std::string&> target_mdir = Nothing,
-					Option<const std::string&> target_mdir = Nothing,
+				   Option<Flags> new_flags = Nothing,
-					Option<Flags> new_flags = Nothing,
+				   MoveOptions opts = MoveOptions::None);
-					MoveOptions opts = MoveOptions::None);
 
 	/**
 	 * Prototype for the ForEachMessageFunc

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);

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_debug("%s", msg3.sexp().to_string().c_str());	 g_assert_cmpuint(store->size(), ==, 1);
-	 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 msg1_path = join_paths(tempdir2.path(), "Maildir/a/new/msg123");
-	 auto msg2_path = tempdir2.path() + "/Maildir/a/cur/msgabc:2,S";
+	 auto msg2_path = join_paths(tempdir2.path(), "Maildir/a/cur/msgabc:2,S");
-	 auto msg3_path = tempdir2.path() + "/Maildir/b/cur/msgdef:2,RS";
+	 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<Store::Id> 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)) {
+	 if (id_msgs.at(1).first == ids.at(1)) {
-		 check_msg2(mres->at(1).second);
+		 check_msg2(id_msgs.at(1).second);
-		 check_msg3(mres->at(2).second);
+		 check_msg3(id_msgs.at(2).second);
 	 } else  {
-		 check_msg2(mres->at(2).second);
+		 check_msg2(id_msgs.at(2).second);
-		 check_msg3(mres->at(1).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);