From 97459beed9a6155b03d5afad6c9d050d3e730f3c Mon Sep 17 00:00:00 2001 From: "Dirk-Jan C. Binnema" Date: Wed, 29 Jun 2022 22:19:26 +0300 Subject: [PATCH] maildir: improve testing coverage Remove some dead/unused code. Update docs. Add test cases. --- lib/mu-maildir.cc | 82 +++++++++++------------------------- lib/mu-maildir.hh | 15 +++---- lib/mu-store.cc | 3 +- lib/tests/test-mu-maildir.cc | 66 +++++++++++++++++++++++++---- 4 files changed, 91 insertions(+), 75 deletions(-) diff --git a/lib/mu-maildir.cc b/lib/mu-maildir.cc index 1045ca1b..15c15bff 100644 --- a/lib/mu-maildir.cc +++ b/lib/mu-maildir.cc @@ -257,43 +257,8 @@ Mu::maildir_clear_links(const std::string& path) return Ok(); } - -static size_t -get_file_size(const std::string& path) -{ - int rv; - struct stat statbuf; - - rv = ::stat(path.c_str(), &statbuf); - if (rv != 0) { - /* g_warning ("error: %s", g_strerror (errno)); */ - return -1; - } - - return static_cast(statbuf.st_size); -} - static Mu::Result -msg_move_check_pre(const std::string& src, const std::string& dst) -{ - if (::access(src.c_str(), R_OK) != 0) - return Err(Error{Error::Code::File, "cannot read %s", src.c_str()}); - - if (::access(dst.c_str(), F_OK) != 0) - return Ok(); - - /* target exist; we simply overwrite it, unless target has a different - * size. ignore the exceedingly rare case where have duplicate message - * file names with different content yet the same length. (md5 etc. is a - * bit slow) */ - if (get_file_size(src) != get_file_size(dst)) - return Err(Error{Error::Code::File, "%s already exists", dst.c_str()}); - - return Ok(); - } - -static Mu::Result -msg_move_check_post(const std::string& src, const std::string& dst) +msg_move_verify(const std::string& src, const std::string& dst) { /* double check -- is the target really there? */ if (::access(dst.c_str(), F_OK) != 0) @@ -322,9 +287,9 @@ msg_move_g_file(const std::string& src, const std::string& dst) GFile *dstfile{g_file_new_for_path(dst.c_str())}; GError* err{}; - auto res = g_file_move(srcfile, dstfile, G_FILE_COPY_NONE, + auto res = g_file_move(srcfile, dstfile, + G_FILE_COPY_OVERWRITE, NULL, NULL, NULL, &err); - g_clear_object(&srcfile); g_clear_object(&dstfile); @@ -337,38 +302,41 @@ msg_move_g_file(const std::string& src, const std::string& dst) } static Mu::Result -msg_move(const std::string& src, const std::string& dst) +msg_move(const std::string& src, const std::string& dst, bool force_gio) { - if (auto&& res = msg_move_check_pre(src, dst); !res) - return res; + if (::access(src.c_str(), R_OK) != 0) + return Err(Error{Error::Code::File, "cannot read %s", src.c_str()}); - if (::rename(src.c_str(), dst.c_str()) == 0) /* seems it worked; double-check */ - return msg_move_check_post(src, dst); + if (!force_gio) { /* for testing */ - if (errno != EXDEV) /* some unrecoverable error occurred */ - return Err(Error{Error::Code::File, "error moving %s -> %s", + if (::rename(src.c_str(), dst.c_str()) == 0) /* seems it worked; double-check */ + return msg_move_verify(src, dst); + + if (errno != EXDEV) /* some unrecoverable error occurred */ + return Err(Error{Error::Code::File, "error moving %s -> %s", src.c_str(), dst.c_str()}); + } - /* the EXDEV case -- source and target live on different filesystems */ - return msg_move_g_file(src, dst); + /* the EXDEV / force-gio case -- source and target live on different + * filesystems */ + if (!msg_move_g_file(src, dst)) + return Err(Error{Error::Code::File, "error moving %s -> %s", + src.c_str(), dst.c_str()}); + else + return msg_move_verify(src, dst); } Mu::Result Mu::maildir_move_message(const std::string& oldpath, - const std::string& newpath, - bool ignore_dups) + const std::string& newpath, + bool force_gio) { - if (oldpath == newpath) { - if (ignore_dups) - return Ok(); - else - return Err(Error{Error::Code::InvalidArgument, - "target equals source"}); - } + if (oldpath == newpath) + return Ok(); // nothing to do. g_debug("moving %s --> %s", oldpath.c_str(), newpath.c_str()); - return msg_move(oldpath, newpath); + return msg_move(oldpath, newpath, force_gio); } static std::string diff --git a/lib/mu-maildir.hh b/lib/mu-maildir.hh index 7976b04b..42a49a37 100644 --- a/lib/mu-maildir.hh +++ b/lib/mu-maildir.hh @@ -75,21 +75,20 @@ Result maildir_link(const std::string& src, const std::string& targetpath, Result maildir_clear_links(const std::string& dir); /** - * Move a message file to another maildir. If the target file already exists, it - * is overwritten. + * Move a message file to another maildir. If the target exists, it is + * overwritten. * * @param oldpath an absolute file system path to an existing message in an * actual maildir * @param newpath the absolete full path to the target file - * @param ignore_dups whether to silently ignore the src==target case - * (and return @true) - * @param err receives error information + * @param force_gio force the use of GIO for moving; this is done automatically + * when needed; forcing is mostly useful for tests * - * @return * @return a valid result (!!result) or an Error + * @return a valid result (!!result) or an Error */ Result maildir_move_message(const std::string& oldpath, - const std::string& newpath, - bool ignore_dups); + const std::string& newpath, + bool force_gio = false); /** * Determine the target path for a to-be-moved message; i.e. this does not diff --git a/lib/mu-store.cc b/lib/mu-store.cc index 706b83e0..375e0a6d 100644 --- a/lib/mu-store.cc +++ b/lib/mu-store.cc @@ -539,8 +539,7 @@ Store::move_message(Store::Id id, return Err(target_path.error()); /* 2. let's move it */ - if (const auto res = maildir_move_message( - msg->path(), target_path.value(), true/*ignore dups*/); !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.*/ diff --git a/lib/tests/test-mu-maildir.cc b/lib/tests/test-mu-maildir.cc index c8f06903..23b36b34 100644 --- a/lib/tests/test-mu-maildir.cc +++ b/lib/tests/test-mu-maildir.cc @@ -28,6 +28,7 @@ #include "test-mu-common.hh" #include "mu-maildir.hh" +#include "utils/mu-result.hh" #include "utils/mu-util.h" using namespace Mu; @@ -458,12 +459,10 @@ test_maildir_from_path(void) static void test_maildir_link() { - TempDir tmpdir{false}; + TempDir tmpdir; - g_message("%s", tmpdir.path().c_str()); - - g_assert_true(!!maildir_mkdir(tmpdir.path() + "/foo")); - g_assert_true(!!maildir_mkdir(tmpdir.path() + "/bar")); + assert_valid_result(maildir_mkdir(tmpdir.path() + "/foo")); + assert_valid_result(maildir_mkdir(tmpdir.path() + "/bar")); const auto srcpath1 = tmpdir.path() + "/foo/cur/msg1"; const auto srcpath2 = tmpdir.path() + "/foo/new/msg2"; @@ -482,8 +481,8 @@ test_maildir_link() stream.close(); } - g_assert_true(!!maildir_link(srcpath1, tmpdir.path() + "/bar", false)); - g_assert_true(!!maildir_link(srcpath2, tmpdir.path() + "/bar", false)); + assert_valid_result(maildir_link(srcpath1, tmpdir.path() + "/bar", false)); + assert_valid_result(maildir_link(srcpath2, tmpdir.path() + "/bar", false)); const auto dstpath1 = tmpdir.path() + "/bar/cur/msg1"; const auto dstpath2 = tmpdir.path() + "/bar/new/msg2"; @@ -491,11 +490,59 @@ test_maildir_link() g_assert_true(g_access(dstpath1.c_str(), F_OK) == 0); g_assert_true(g_access(dstpath2.c_str(), F_OK) == 0); - g_assert_true(!!maildir_clear_links(tmpdir.path() + "/bar")); + assert_valid_result(maildir_clear_links(tmpdir.path() + "/bar")); g_assert_false(g_access(dstpath1.c_str(), F_OK) == 0); g_assert_false(g_access(dstpath2.c_str(), F_OK) == 0); } + +static void +test_maildir_move(bool use_gio) +{ + TempDir tmpdir; + + assert_valid_result(maildir_mkdir(tmpdir.path() + "/foo")); + assert_valid_result(maildir_mkdir(tmpdir.path() + "/bar")); + + const auto srcpath1 = tmpdir.path() + "/foo/cur/msg1"; + const auto srcpath2 = tmpdir.path() + "/foo/new/msg2"; + + { + std::ofstream stream(srcpath1); + stream.write("cur", 3); + g_assert_true(stream.good()); + stream.close(); + } + + { + std::ofstream stream(srcpath2); + stream.write("new", 3); + g_assert_true(stream.good()); + stream.close(); + } + + const auto dstpath = tmpdir.path() + "/test1"; + + assert_valid_result(maildir_move_message(srcpath1, dstpath, use_gio)); + assert_valid_result(maildir_move_message(srcpath2, dstpath, use_gio)); + + + //g_assert_true(g_access(dstpath.c_str(), F_OK) == 0); +} + +static void +test_maildir_move_vanilla() +{ + test_maildir_move(false/*!gio*/); +} + +static void +test_maildir_move_gio() +{ + test_maildir_move(true/*gio*/); +} + + int main(int argc, char* argv[]) { @@ -523,5 +570,8 @@ main(int argc, char* argv[]) g_test_add_func("/mu-maildir/mu-maildir-link", test_maildir_link); + g_test_add_func("/mu-maildir/mu-maildir-move-vanilla", test_maildir_move_vanilla); + g_test_add_func("/mu-maildir/mu-maildir-move-gio", test_maildir_move_gio); + return g_test_run(); }