Switching to std::filesystems makes possible to leverage std::filesystem::path::lexically_normal and get rid of ugly StripRedundantLastElementsOfPath() crutch.
To make its usage simple and error-proof, a new ArgsManager::GetPathArg() member function introduced which guarantees to return a normalized with no trailing slashes paths provided via -datadir, -blocksdir or -walletdir command-line arguments or configure options.
Also it became possible to re-enable Windows tests which were disabled in #20744. So this PR, apparently, is an alternative to the #24251.
This all sounds good. IMO, it would be good to merge #24251 before this PR. #24251 has two very simple fixes for the regressions caused by #20744 yesterday. This PR should be trivial to rebase if #24251 is merged first, and should become a little simpler, too.
270 | + * It is guaranteed that the returned path has no trailing slashes. 271 | + * 272 | + * @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir") 273 | + * @return Normalized path which is get from a specified pathlike argument 274 | + */ 275 | + fs::path GetNormalPath(std::string pathlike_arg) const;
In commit "util: Add ArgsManager::GetNormalPath() function" (034f12a115e21b4205b532a3f946b129e83505d9)
Minor: Would vote for calling this GetPathArg to be consistent with GetIntArg GetBoolArg, instead of GetNormalPath
403 | +{ 404 | + const std::string path_str{GetArg(pathlike_arg, "")}; 405 | + if (path_str.empty()) return {}; 406 | + // Adding a slash followed by parent_path() call 407 | + // guaranties no trailing slashes in the resulted path. 408 | + return fs::PathFromString(path_str + "/").lexically_normal().parent_path();
In commit "util: Add ArgsManager::GetNormalPath() function" (034f12a115e21b4205b532a3f946b129e83505d9)
I think previous approach which avoids direct string manipulation and avoids assuming "/" is a path separator is a little cleaner. Would suggest something like:
fs::path result = fs::PathFromString(path_str).lexically_normal(); // Collapse slashes and dots
return result.has_filename() ? result : result.parent_path(); // Remove trailing slash, if present
Also it would be great to write a simple unit test for this new function.
Thanks for your suggestions! Most of them are implemented.
Also it would be great to write a simple unit test for this new function.
Will add in the morning :)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
gArgs from bdb.h and sqlite.h by kiminuo)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Updated e69ab379283af554d60c484d5715112b887fa0c3 -> 05c8d8cd075c37f43ad195bceac3eaa1f2907e07 (pr24265.02 -> pr24265.03):
There are couple of more places where we can do similar optimization, but for arguments which expect filename, not path (= directory).
src/init.cpp:138: return AbsPathForConfigVal(fs::PathFromString(args.GetArg("-pid", BITCOIN_PID_FILENAME)));
src/init.cpp:1249: fs::path asmap_path = fs::PathFromString(args.GetArg("-asmap", ""));
src/init/common.cpp:84: LogInstance().m_file_path = AbsPathForConfigVal(fs::PathFromString(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE)));
src/bench/bench_bitcoin.cpp:112: args.output_csv = fs::PathFromString(argsman.GetArg("-output_csv", ""));
src/bench/bench_bitcoin.cpp:113: args.output_json = fs::PathFromString(argsman.GetArg("-output_json", ""));
It would be possible to introduce a GetFileArg function using the following refactor and use that to simplify the lines above:
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 496c82584..ef7c1cfff 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -386,9 +386,14 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
return std::nullopt;
}
-fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const
+fs::path ArgsManager::GetFileArg(std::string pathlike_arg, const std::string& strDefault = "") const
{
- auto result = fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal();
+ return fs::PathFromString(GetArg(pathlike_arg, strDefault)).lexically_normal();
+}
+
+fs::path ArgsManager::GetPathArg(std::string pathlike_arg, const std::string& strDefault = "") const
+{
+ auto result = GetFileArg(pathlike_arg, strDefault);
// Remove trailing slash, if present.
return result.has_filename() ? result : result.parent_path();
}
What do you think?
158 | @@ -159,6 +159,98 @@ BOOST_AUTO_TEST_CASE(intarg) 159 | BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0); 160 | } 161 | 162 | +BOOST_AUTO_TEST_CASE(patharg)
In commit "util: Add ArgsManager::GetPathArg() function" (d32f6c0dc89f1a609d59a222b09eabb1eb0da542)
Nice tests!
There are couple of more places where we can do similar optimization, but for arguments which expect filename, not path (= directory).
It would be possible to introduce a
GetFileArgfunction using the following refactor and use that to simplify the lines above:
I'd push back on these suggestions a little and save these things for followups. They would be natural extensions of this PR, but would risk unintentionally changing behavior and don't need to be part of it. Also a std::string default argument would pass more paths as strings, when I think paths should be represented by fs::path not std::string wherever possible.
Code review ACK 05c8d8cd075c37f43ad195bceac3eaa1f2907e07. This looks great, and I really like the way you broke it up into isolated commits. Make this very easy to review.
re: #24265#issue-1124619834
Also it became possible to re-enable Windows tests which were disabled in #20744. So this PR, apparently, is an alternative to the #24251.
This is a little out of date and could be removed from the description
There are couple of more places where we can do similar optimization, but for arguments which expect filename, not path (= directory). It would be possible to introduce a
GetFileArgfunction using the following refactor and use that to simplify the lines above:I'd push back on these suggestions a little and save these things for followups. They would be natural extensions of this PR, but would risk unintentionally changing behavior and don't need to be part of it. Also a
std::stringdefault argument would pass more paths as strings, when I think paths should be represented byfs::pathnotstd::stringwherever possible.
Agree.
270 | + * It is guaranteed that the returned path has no trailing slashes. 271 | + * 272 | + * @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir") 273 | + * @return Normalized path which is get from a specified pathlike argument 274 | + */ 275 | + fs::path GetPathArg(std::string pathlike_arg) const;
Other Get...Arg methods use the camelCase notation for arguments. Let's change to pathlikeArg or similar.
For new code we strive to follow our symbol naming convention:
Variable (including function arguments) and namespace names are all lowercase and may use _ to separate words (snake_case).
385 | @@ -399,6 +386,13 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co 386 | return std::nullopt; 387 | } 388 | 389 | +fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const
Other Get...Arg methods use the camelCase notation for arguments. Let's change to pathlikeArg or similar.
ACK 05c8d8c
This is "refactoring"?
This is "refactoring"?
There are some changes, I assume they are nice, in behavior. For example, the following works:
$ ./src/bitcoind -datadir=/root/../home/hebasto/.bitcoin
Concept ACK.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Rebased 05c8d8cd075c37f43ad195bceac3eaa1f2907e07 -> ebda2b8c819d989327c6b3e29237dfb43628e647 (pr24265.03 -> pr24265.04) due to the conflict with #24266.
Code review ACK ebda2b8c819d989327c6b3e29237dfb43628e647. Only change since last review is rebase which simplified the last commit