diff --git a/libtransmission/file-posix.cc b/libtransmission/file-posix.cc index 868078508..295326b54 100644 --- a/libtransmission/file-posix.cc +++ b/libtransmission/file-posix.cc @@ -398,17 +398,50 @@ std::string tr_sys_path_basename(std::string_view path, tr_error** error) return {}; } -std::string tr_sys_path_dirname(std::string_view path, tr_error** error) +// This function is adapted from Node.js's path.posix.dirname() function, +// which is copyrighted by Joyent, Inc. and other Node contributors +// and is distributed under MIT (SPDX:MIT) license. +std::string_view tr_sys_path_dirname(std::string_view path) { - auto tmp = tr_pathbuf{ path }; + auto const len = std::size(path); - if (char const* ret = dirname(std::data(tmp)); ret != nullptr) + if (len == 0U) { - return ret; + return "."sv; } - set_system_error(error, errno); - return {}; + auto const has_root = path[0] == '/'; + auto end = std::string_view::npos; + auto matched_slash = bool{ true }; + + for (auto i = len - 1; i >= 1U; --i) + { + if (path[i] == '/') + { + if (!matched_slash) + { + end = i; + break; + } + } + else + { + // We saw the first non-path separator + matched_slash = false; + } + } + + if (end == std::string_view::npos) + { + return has_root ? "/"sv : "."sv; + } + + if (has_root && end == 1) + { + return "//"sv; + } + + return path.substr(0, end); } bool tr_sys_path_rename(char const* src_path, char const* dst_path, tr_error** error) diff --git a/libtransmission/file-win32.cc b/libtransmission/file-win32.cc index fcea860d6..ba310ca79 100644 --- a/libtransmission/file-win32.cc +++ b/libtransmission/file-win32.cc @@ -687,62 +687,128 @@ std::string tr_sys_path_basename(std::string_view path, tr_error** error) return { name, size_t(end - name) }; } -std::string tr_sys_path_dirname(std::string_view path, tr_error** error) +[[nodiscard]] static bool isWindowsDeviceRoot(char ch) noexcept { - if (std::empty(path)) + return isalpha(static_cast(ch)) != 0; +} + +[[nodiscard]] static constexpr bool isPathSeparator(char ch) noexcept +{ + return ch == '/' || ch == '\\'; +} + +// This function is adapted from Node.js's path.win32.dirname() function, +// which is copyrighted by Joyent, Inc. and other Node contributors +// and is distributed under MIT (SPDX:MIT) license. +std::string_view tr_sys_path_dirname(std::string_view path) +{ + auto const len = std::size(path); + + if (len == 0) { - return "."; + return "."sv; } - if (!is_valid_path(path)) + if (len == 1) { - set_system_error(error, ERROR_PATH_NOT_FOUND); - return {}; + return isPathSeparator(path[0]) ? path : "."sv; } - bool const is_unc = is_unc_path(path); + auto root_end = std::string_view::npos; + auto offset = std::string_view::size_type{ 0 }; - if (is_unc && path[2] == '\0') + // Try to match a root + if (isPathSeparator(path[0])) { - return std::string{ path }; + // Possible UNC root + + root_end = offset = 1; + + if (isPathSeparator(path[1])) + { + // Matched double path separator at beginning + std::string_view::size_type j = 2; + std::string_view::size_type last = j; + // Match 1 or more non-path separators + while (j < len && !isPathSeparator(path[j])) + { + j++; + } + if (j < len && j != last) + { + // Matched! + last = j; + // Match 1 or more path separators + while (j < len && isPathSeparator(path[j])) + { + j++; + } + if (j < len && j != last) + { + // Matched! + last = j; + // Match 1 or more non-path separators + while (j < len && !isPathSeparator(path[j])) + { + j++; + } + if (j == len) + { + // We matched a UNC root only + return path; + } + if (j != last) + { + // We matched a UNC root with leftovers + + // Offset by 1 to include the separator after the UNC root to + // treat it as a "normal root" on top of a (UNC) root + root_end = offset = j + 1; + } + } + } + } + // Possible device root + } + else if (isWindowsDeviceRoot(path[0]) && path[1] == ':') + { + root_end = len > 2 && isPathSeparator(path[2]) ? 3 : 2; + offset = root_end; } - char const* const begin = std::data(path); - char const* end = begin + std::size(path); - - while (end > begin && is_slash(*(end - 1))) + auto end = std::string_view::npos; + auto matched_slash = bool{ true }; + for (std::string_view::size_type i = len - 1; i >= offset; --i) { - --end; + if (isPathSeparator(path[i])) + { + if (!matched_slash) + { + end = i; + break; + } + } + else + { + // We saw the first non-path separator + matched_slash = false; + } + if (i <= offset) + { + break; + } } - if (end == begin) + if (end == std::string_view::npos) { - return "/"; + if (root_end == std::string_view::npos) + { + return "."sv; + } + + end = root_end; } - - char const* name = end; - - while (name > begin && *(name - 1) != ':' && !is_slash(*(name - 1))) - { - --name; - } - - while (name > begin && is_slash(*(name - 1))) - { - --name; - } - - if (name == begin) - { - return is_unc ? "\\\\" : "."; - } - - if (name > begin && *(name - 1) == ':' && *name != '\0' && !is_slash(*name)) - { - return fmt::format("{}:.", begin[0]); - } - - return { begin, size_t(name - begin) }; + return path.substr(0, end); } bool tr_sys_path_rename(char const* src_path, char const* dst_path, tr_error** error) diff --git a/libtransmission/file.h b/libtransmission/file.h index eef92fb4a..fa7f7a955 100644 --- a/libtransmission/file.h +++ b/libtransmission/file.h @@ -230,15 +230,13 @@ std::string tr_sys_path_basename(std::string_view path, struct tr_error** error * @brief Portability wrapper for `dirname()`. * * @param[in] path Path to file or directory. - * @param[out] error Pointer to error object. Optional, pass `nullptr` if you - * are not interested in error details. * * @return Pointer to newly allocated buffer containing directory (parent path; * last path component removed) on success (use @ref tr_free to free it * when no longer needed), `nullptr` otherwise (with `error` set * accordingly). */ -std::string tr_sys_path_dirname(std::string_view path, struct tr_error** error = nullptr); +std::string_view tr_sys_path_dirname(std::string_view path); /** * @brief Portability wrapper for `rename()`. diff --git a/libtransmission/open-files.cc b/libtransmission/open-files.cc index e6c99c47b..35db9928c 100644 --- a/libtransmission/open-files.cc +++ b/libtransmission/open-files.cc @@ -152,20 +152,15 @@ std::optional tr_open_files::get( tr_error* error = nullptr; if (writable) { - auto const dir = tr_sys_path_dirname(filename, &error); + auto const dir = tr_sys_path_dirname(filename); if (std::empty(dir)) { - tr_logAddError(fmt::format( - _("Couldn't create '{path}': {error} ({error_code})"), - fmt::arg("path", filename), - fmt::arg("error", error->message), - fmt::arg("error_code", error->code))); - tr_error_free(error); + tr_logAddError(fmt::format(_("Couldn't create '{path}'"), fmt::arg("path", filename))); return {}; } - if (!tr_sys_dir_create(dir.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0777, &error)) + if (!tr_sys_dir_create(std::string{ dir }.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0777, &error)) { tr_logAddError(fmt::format( _("Couldn't create '{path}': {error} ({error_code})"), diff --git a/libtransmission/utils.cc b/libtransmission/utils.cc index cddb741c9..fd700beee 100644 --- a/libtransmission/utils.cc +++ b/libtransmission/utils.cc @@ -968,8 +968,8 @@ bool tr_moveFile(std::string_view oldpath_in, std::string_view newpath_in, tr_er } // ensure the target directory exists - if (auto const newdir = tr_sys_path_dirname(newpath, error); - std::empty(newdir) || !tr_sys_dir_create(newdir.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0777, error)) + if (auto const newdir = tr_sys_path_dirname(newpath); + std::empty(newdir) || !tr_sys_dir_create(std::string{ newdir }.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0777, error)) { tr_error_prefix(error, "Unable to create directory for new file: "); return false; diff --git a/tests/libtransmission/file-test.cc b/tests/libtransmission/file-test.cc index d63f6f318..3ebeee010 100644 --- a/tests/libtransmission/file-test.cc +++ b/tests/libtransmission/file-test.cc @@ -8,6 +8,7 @@ #include #include #include +#include #ifndef _WIN32 #include @@ -181,6 +182,32 @@ protected: } } + static void testPathXname( + XnameTestData const* data, + size_t data_size, + std::string_view (*func)(std::string_view, tr_error**)) + { + for (size_t i = 0; i < data_size; ++i) + { + tr_error* err = nullptr; + auto const name = func(data[i].input, &err); + std::cerr << __FILE__ << ':' << __LINE__ << " in [" << data[i].input << "] out [" << name << ']' << std::endl; + + if (data[i].output != nullptr) + { + EXPECT_NE(""sv, name); + EXPECT_EQ(nullptr, err) << *err; + EXPECT_EQ(std::string{ data[i].output }, name); + } + else + { + EXPECT_EQ(""sv, name); + EXPECT_NE(nullptr, err); + tr_error_clear(&err); + } + } + } + static void testDirReadImpl(tr_pathbuf const& path, bool* have1, bool* have2) { *have1 = *have2 = false; @@ -687,7 +714,7 @@ TEST_F(FileTest, pathResolve) #endif } -TEST_F(FileTest, pathBasenameDirname) +TEST_F(FileTest, pathBasename) { auto const common_xname_tests = std::vector{ XnameTestData{ "/", "/" }, @@ -725,7 +752,7 @@ TEST_F(FileTest, pathBasenameDirname) }; testPathXname(common_xname_tests.data(), common_xname_tests.size(), tr_sys_path_basename); - testPathXname(common_xname_tests.data(), common_xname_tests.size(), tr_sys_path_dirname); + // testPathXname(common_xname_tests.data(), common_xname_tests.size(), tr_sys_path_dirname); auto const basename_tests = std::vector{ XnameTestData{ "a", "a" }, @@ -751,36 +778,91 @@ TEST_F(FileTest, pathBasenameDirname) }; testPathXname(basename_tests.data(), basename_tests.size(), tr_sys_path_basename); +} - auto const dirname_tests = std::vector{ - XnameTestData{ "/a/b/c", "/a/b" }, - { "a/b/c", "a/b" }, - { "a/b/c/", "a/b" }, - { "a", "." }, - { "a/", "." }, +TEST_F(FileTest, pathDirname) +{ #ifdef _WIN32 - { "C:\\a/b\\c", "C:\\a/b" }, - { "C:\\a/b\\c\\", "C:\\a/b" }, - { "C:\\a/b", "C:\\a" }, - { "C:/a", "C:" }, - { "C:", "C:" }, - { "C:/", "C:" }, - { "C:\\", "C:" }, - { "c:a/b", "c:a" }, - { "c:a", "c:." }, - { "c:.", "c:." }, - { "\\\\a\\b\\c", "\\\\a\\b" }, - { "\\\\a\\b\\c/", "\\\\a\\b" }, - { "//a/b", "//a" }, - { "//1.2.3.4/b", "//1.2.3.4" }, - { "\\\\a", "\\\\" }, - { "\\\\1.2.3.4", "\\\\" }, - { "\\\\", "\\\\" }, - { "a/b\\c", "a/b" }, + static auto constexpr DirnameTests = std::array, 48>{ { + { "C:\\a/b\\c"sv, "C:\\a/b"sv }, + { "C:\\a/b\\c\\"sv, "C:\\a/b"sv }, + { "C:\\a/b"sv, "C:\\a"sv }, + { "C:/a"sv, "C:/"sv }, + { "C:"sv, "C:"sv }, + { "C:/"sv, "C:/"sv }, + { "c:a/b"sv, "c:a"sv }, + { "c:a"sv, "c:"sv }, + { "\\\\a"sv, "\\"sv }, + { "\\\\1.2.3.4"sv, "\\"sv }, + { "\\\\"sv, "\\"sv }, + { "a/b\\c"sv, "a/b"sv }, + // taken from Node.js unit tests + // https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/test/parallel/test-path-dirname.js + { "c:\\"sv, "c:\\"sv }, + { "c:\\foo"sv, "c:\\"sv }, + { "c:\\foo\\"sv, "c:\\"sv }, + { "c:\\foo\\bar"sv, "c:\\foo"sv }, + { "c:\\foo\\bar\\"sv, "c:\\foo"sv }, + { "c:\\foo\\bar\\baz"sv, "c:\\foo\\bar"sv }, + { "c:\\foo bar\\baz"sv, "c:\\foo bar"sv }, + { "\\"sv, "\\"sv }, + { "\\foo"sv, "\\"sv }, + { "\\foo\\"sv, "\\"sv }, + { "\\foo\\bar"sv, "\\foo"sv }, + { "\\foo\\bar\\"sv, "\\foo"sv }, + { "\\foo\\bar\\baz"sv, "\\foo\\bar"sv }, + { "\\foo bar\\baz"sv, "\\foo bar"sv }, + { "c:"sv, "c:"sv }, + { "c:foo"sv, "c:"sv }, + { "c:foo\\"sv, "c:"sv }, + { "c:foo\\bar"sv, "c:foo"sv }, + { "c:foo\\bar\\"sv, "c:foo"sv }, + { "c:foo\\bar\\baz"sv, "c:foo\\bar"sv }, + { "c:foo bar\\baz"sv, "c:foo bar"sv }, + { "file:stream"sv, "."sv }, + { "dir\\file:stream"sv, "dir"sv }, + { "\\\\unc\\share"sv, "\\\\unc\\share"sv }, + { "\\\\unc\\share\\foo"sv, "\\\\unc\\share\\"sv }, + { "\\\\unc\\share\\foo\\"sv, "\\\\unc\\share\\"sv }, + { "\\\\unc\\share\\foo\\bar"sv, "\\\\unc\\share\\foo"sv }, + { "\\\\unc\\share\\foo\\bar\\"sv, "\\\\unc\\share\\foo"sv }, + { "\\\\unc\\share\\foo\\bar\\baz"sv, "\\\\unc\\share\\foo\\bar"sv }, + { "/a/b/"sv, "/a"sv }, + { "/a/b"sv, "/a"sv }, + { "/a"sv, "/"sv }, + { ""sv, "."sv }, + { "/"sv, "/"sv }, + { "////"sv, "/"sv }, + { "foo"sv, "."sv }, + } }; +#else + static auto constexpr DirnameTests = std::array, 15>{ { + // taken from Node.js unit tests + // https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/test/parallel/test-path-dirname.js + { "/a/b/"sv, "/a"sv }, + { "/a/b"sv, "/a"sv }, + { "/a"sv, "/"sv }, + { ""sv, "."sv }, + { "/"sv, "/"sv }, + { "////"sv, "/"sv }, + { "//a"sv, "//"sv }, + { "foo"sv, "."sv }, + // taken from dirname(3) manpage + { "usr"sv, "."sv }, + { "/usr/lib", "/usr"sv }, + { "/usr/"sv, "/"sv }, + { "/usr/"sv, "/"sv }, + { "/"sv, "/"sv }, + { "."sv, "."sv }, + { ".."sv, "."sv }, + } }; #endif - }; - testPathXname(dirname_tests.data(), dirname_tests.size(), tr_sys_path_dirname); + for (auto const& [input, expected] : DirnameTests) + { + EXPECT_EQ(expected, tr_sys_path_dirname(input)) << "input[" << input << "] expected [" << expected << "] actual [" + << tr_sys_path_dirname(input) << ']' << std::endl; + } /* TODO: is_same(dirname(x) + '/' + basename(x), x) */ } diff --git a/tests/libtransmission/remove-test.cc b/tests/libtransmission/remove-test.cc index 571e9d1a5..81a25dcde 100644 --- a/tests/libtransmission/remove-test.cc +++ b/tests/libtransmission/remove-test.cc @@ -180,10 +180,11 @@ protected: createFileWithContents(filename, std::data(Content), std::size(Content)); paths.emplace(filename); - while (!tr_sys_path_is_same(parent, filename)) + auto walk = std::string_view{ filename.sv() }; + while (!tr_sys_path_is_same(parent, std::string{ walk }.c_str())) { - filename = tr_sys_path_dirname(filename); - paths.emplace(filename); + walk = tr_sys_path_dirname(walk); + paths.emplace(walk); } } @@ -301,7 +302,7 @@ TEST_F(RemoveTest, LeavesNonJunkAlone) EXPECT_EQ(expected_tree, getSubtreeContents(parent)); files.remove(parent, "tmpdir_prefix"sv, sysPathRemove); - expected_tree = { parent, tr_sys_path_dirname(nonjunk_file), nonjunk_file.c_str() }; + expected_tree = { parent, std::string{ tr_sys_path_dirname(nonjunk_file) }, nonjunk_file.c_str() }; EXPECT_EQ(expected_tree, getSubtreeContents(parent)); } diff --git a/tests/libtransmission/rename-test.cc b/tests/libtransmission/rename-test.cc index 39f55370a..3f0edcfcd 100644 --- a/tests/libtransmission/rename-test.cc +++ b/tests/libtransmission/rename-test.cc @@ -350,7 +350,7 @@ TEST_F(RenameTest, multifileTorrent) str = tr_torrentFindFile(tor, 2); EXPECT_NE(nullptr, str); tr_sys_path_remove(str); - tr_sys_path_remove(tr_sys_path_dirname(str).c_str()); + tr_sys_path_remove(std::string{ tr_sys_path_dirname(str) }.c_str()); tr_free(str); sync(); blockingTorrentVerify(tor); diff --git a/tests/libtransmission/subprocess-test.cc b/tests/libtransmission/subprocess-test.cc index b766602f3..403ca24d2 100644 --- a/tests/libtransmission/subprocess-test.cc +++ b/tests/libtransmission/subprocess-test.cc @@ -35,7 +35,7 @@ std::string getTestProgramPath(std::string const& filename) { auto const exe_path = makeString(tr_sys_path_resolve(testing::internal::GetArgvs().front().data())); auto const exe_dir = tr_sys_path_dirname(exe_path); - return exe_dir + TR_PATH_DELIMITER + filename; + return std::string{ exe_dir } + TR_PATH_DELIMITER + filename; } class SubprocessTest diff --git a/tests/libtransmission/test-fixtures.h b/tests/libtransmission/test-fixtures.h index a264c97bd..24da49662 100644 --- a/tests/libtransmission/test-fixtures.h +++ b/tests/libtransmission/test-fixtures.h @@ -184,7 +184,7 @@ protected: auto const dir = tr_sys_path_dirname(path); tr_error* error = nullptr; - tr_sys_dir_create(dir.data(), TR_SYS_DIR_CREATE_PARENTS, 0700, &error); + tr_sys_dir_create(std::string{ dir }.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0700, &error); EXPECT_EQ(nullptr, error) << "path[" << path << "] dir[" << dir << "] " << *error; errno = tmperr; @@ -411,7 +411,7 @@ protected: auto const filename = tr_pathbuf{ base, '/', subpath, suffix }; auto const dirname = tr_sys_path_dirname(filename); - tr_sys_dir_create(dirname.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0700); + tr_sys_dir_create(std::string{ dirname }.c_str(), TR_SYS_DIR_CREATE_PARENTS, 0700); auto fd = tr_sys_file_open(filename, TR_SYS_FILE_WRITE | TR_SYS_FILE_CREATE | TR_SYS_FILE_TRUNCATE, 0600); auto const file_size = metainfo->fileSize(i);