Merge bitcoin/bitcoin#24265: Drop StripRedundantLastElementsOfPath() function

ebda2b8 util: Drop no longer needed StripRedundantLastElementsOfPath() function (Hennadii Stepanov)
ecd094e Use ArgsManager::GetPathArg() for "-walletdir" option (Hennadii Stepanov)
06fed4c Use ArgsManager::GetPathArg() for "-blocksdir" option (Hennadii Stepanov)
15b632b Use ArgsManager::GetPathArg() for "-datadir" option (Hennadii Stepanov)
540ca51 util: Add ArgsManager::GetPathArg() function (Hennadii Stepanov)

Pull request description:

  [Switching](bitcoin/bitcoin#20744) to `std::filesystems` makes possible to leverage [`std::filesystem::path::lexically_normal`](https://en.cppreference.com/w/cpp/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.

ACKs for top commit:
  ryanofsky:
    Code review ACK ebda2b8. Only change since last review is rebase which simplified the last commit

Tree-SHA512: ed86959b6038b7152b5a1d473478667b72caab1716cc9149e1a75833d50511f22157e4e5e55a9465d1fa76b90bce5e5286f4e4f0d1ae65ebd9c012fae19d835f

Author: fanquake

src/init.cpp

@@ -1150,7 +1150,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
 
     // Warn about relative -datadir path.
-    if (args.IsArgSet("-datadir") && !fs::PathFromString(args.GetArg("-datadir", "")).is_absolute()) {
+    if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) {
         LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the " /* Continued */
                   "current working directory '%s'. This is fragile, because if bitcoin is started in the future "
                   "from a different location, it will be unable to locate the current data files. There could "

src/test/getarg_tests.cpp

@@ -159,6 +159,98 @@ BOOST_AUTO_TEST_CASE(intarg)
     BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0);
 }
 
+BOOST_AUTO_TEST_CASE(patharg)
+{
+    const auto dir = std::make_pair("-dir", ArgsManager::ALLOW_ANY);
+    SetupArgs({dir});
+    ResetArgs("");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
+
+    const fs::path root_path{"/"};
+    ResetArgs("-dir=/");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
+
+    ResetArgs("-dir=/.");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
+
+    ResetArgs("-dir=/./");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
+
+    ResetArgs("-dir=/.//");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
+
+#ifdef WIN32
+    const fs::path win_root_path{"C:\\"};
+    ResetArgs("-dir=C:\\");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+
+    ResetArgs("-dir=C:/");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+
+    ResetArgs("-dir=C:\\\\");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+
+    ResetArgs("-dir=C:\\.");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+
+    ResetArgs("-dir=C:\\.\\");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+
+    ResetArgs("-dir=C:\\.\\\\");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
+#endif
+
+    const fs::path absolute_path{"/home/user/.bitcoin"};
+    ResetArgs("-dir=/home/user/.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/root/../home/user/.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/./user/.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/user/.bitcoin/");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/user/.bitcoin//");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/user/.bitcoin/.");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/user/.bitcoin/./");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    ResetArgs("-dir=/home/user/.bitcoin/.//");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
+
+    const fs::path relative_path{"user/.bitcoin"};
+    ResetArgs("-dir=user/.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=somewhere/../user/.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/./.bitcoin");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/.bitcoin/");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/.bitcoin//");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/.bitcoin/.");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/.bitcoin/./");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+
+    ResetArgs("-dir=user/.bitcoin/.//");
+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
+}
+
 BOOST_AUTO_TEST_CASE(doubledash)
 {
     const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);

src/util/system.cpp

@@ -245,19 +245,6 @@ static std::optional<util::SettingsValue> InterpretValue(const KeyInfo& key, con
     return value;
 }
 
-namespace {
-fs::path StripRedundantLastElementsOfPath(const fs::path& path)
-{
-    auto result = path;
-    while (result.filename().empty() || fs::PathToString(result.filename()) == ".") {
-        result = result.parent_path();
-    }
-
-    assert(fs::equivalent(result, path.lexically_normal()));
-    return result;
-}
-} // namespace
-
 // Define default constructor and destructor that are not inline, so code instantiating this class doesn't need to
 // #include class definitions for all members.
 // For example, m_settings has an internal dependency on univalue.
@@ -399,6 +386,13 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
     return std::nullopt;
 }
 
+fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const
+{
+    auto result = fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal();
+    // Remove trailing slash, if present.
+    return result.has_filename() ? result : result.parent_path();
+}
+
 const fs::path& ArgsManager::GetBlocksDirPath() const
 {
     LOCK(cs_args);
@@ -409,7 +403,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const
     if (!path.empty()) return path;
 
     if (IsArgSet("-blocksdir")) {
-        path = fs::absolute(fs::PathFromString(GetArg("-blocksdir", "")));
+        path = fs::absolute(GetPathArg("-blocksdir"));
         if (!fs::is_directory(path)) {
             path = "";
             return path;
@@ -433,9 +427,9 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
     // this function
     if (!path.empty()) return path;
 
-    std::string datadir = GetArg("-datadir", "");
+    const fs::path datadir{GetPathArg("-datadir")};
     if (!datadir.empty()) {
-        path = fs::absolute(StripRedundantLastElementsOfPath(fs::PathFromString(datadir)));
+        path = fs::absolute(datadir);
         if (!fs::is_directory(path)) {
             path = "";
             return path;
@@ -455,7 +449,6 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
         }
     }
 
-    path = StripRedundantLastElementsOfPath(path);
     return path;
 }
 
@@ -816,8 +809,8 @@ fs::path GetDefaultDataDir()
 
 bool CheckDataDirOption()
 {
-    std::string datadir = gArgs.GetArg("-datadir", "");
-    return datadir.empty() || fs::is_directory(fs::absolute(fs::PathFromString(datadir)));
+    const fs::path datadir{gArgs.GetPathArg("-datadir")};
+    return datadir.empty() || fs::is_directory(fs::absolute(datadir));
 }
 
 fs::path GetConfigFile(const std::string& confPath)

src/util/system.h

@@ -264,6 +264,16 @@ class ArgsManager
      */
     std::optional<const Command> GetCommand() const;
 
+    /**
+     * Get a normalized path from a specified pathlike argument
+     *
+     * It is guaranteed that the returned path has no trailing slashes.
+     *
+     * @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
+     * @return Normalized path which is get from a specified pathlike argument
+     */
+    fs::path GetPathArg(std::string pathlike_arg) const;
+
     /**
      * Get blocks directory path
      *

src/wallet/load.cpp

@@ -28,7 +28,7 @@ bool VerifyWallets(WalletContext& context)
     ArgsManager& args = *Assert(context.args);
 
     if (args.IsArgSet("-walletdir")) {
-        fs::path wallet_dir = fs::PathFromString(args.GetArg("-walletdir", ""));
+        const fs::path wallet_dir{args.GetPathArg("-walletdir")};
         std::error_code error;
         // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
         // It also lets the fs::exists and fs::is_directory checks below pass on windows, since they return false

src/wallet/test/init_tests.cpp

@@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_default)
     SetWalletDir(m_walletdir_path_cases["default"]);
     bool result = m_wallet_loader->verify();
     BOOST_CHECK(result == true);
-    fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
+    fs::path walletdir = gArgs.GetPathArg("-walletdir");
     fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
     BOOST_CHECK_EQUAL(walletdir, expected_path);
 }
@@ -28,7 +28,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_custom)
     SetWalletDir(m_walletdir_path_cases["custom"]);
     bool result = m_wallet_loader->verify();
     BOOST_CHECK(result == true);
-    fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
+    fs::path walletdir = gArgs.GetPathArg("-walletdir");
     fs::path expected_path = fs::canonical(m_walletdir_path_cases["custom"]);
     BOOST_CHECK_EQUAL(walletdir, expected_path);
 }
@@ -68,7 +68,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
     SetWalletDir(m_walletdir_path_cases["trailing"]);
     bool result = m_wallet_loader->verify();
     BOOST_CHECK(result == true);
-    fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
+    fs::path walletdir = gArgs.GetPathArg("-walletdir");
     fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
     BOOST_CHECK_EQUAL(walletdir, expected_path);
 }
@@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing2)
     SetWalletDir(m_walletdir_path_cases["trailing2"]);
     bool result = m_wallet_loader->verify();
     BOOST_CHECK(result == true);
-    fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
+    fs::path walletdir = gArgs.GetPathArg("-walletdir");
     fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
     BOOST_CHECK_EQUAL(walletdir, expected_path);
 }

src/wallet/walletutil.cpp

@@ -13,7 +13,7 @@ fs::path GetWalletDir()
     fs::path path;
 
     if (gArgs.IsArgSet("-walletdir")) {
-        path = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
+        path = gArgs.GetPathArg("-walletdir");
         if (!fs::is_directory(path)) {
             // If the path specified doesn't exist, we return the deliberately
             // invalid empty string.