Drop StripRedundantLastElementsOfPath() function by hebasto · Pull Request #24265 · bitcoin/bitcoin

hebasto · 2022-02-04T21:11:19Z

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.

ryanofsky · 2022-02-04T21:25:09Z

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.

src/util/system.h

src/util/system.cpp

hebasto · 2022-02-04T22:35:55Z

@ryanofsky

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

fanquake · 2022-02-05T03:01:33Z

This all sounds good. IMO, it would be good to merge #24251 before this PR.

I agree, we'll merge #24251 first. Maybe rebase on top?

DrahtBot · 2022-02-05T09:02:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24266 (util: Avoid buggy std::filesystem:::create_directories() call by hebasto)
#23732 (refactor: Remove gArgs from bdb.h and sqlite.h by kiminuo)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

hebasto · 2022-02-05T13:21:36Z

Updated e69ab37 -> 05c8d8c (pr24265.02 -> pr24265.03):

rebased on top of the Re-enable windows path tests disabled by #20744 #24251
unit test added

prusnak · 2022-02-05T16:43:12Z

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?

ryanofsky · 2022-02-05T17:42:02Z

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 GetFileArg function 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.

ryanofsky

Code review ACK 05c8d8c. This looks great, and I really like the way you broke it up into isolated commits. Make this very easy to review.

re: #24265 (comment)

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

ryanofsky · 2022-02-05T17:00:56Z

src/test/getarg_tests.cpp

    BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0);
 }

+BOOST_AUTO_TEST_CASE(patharg)


In commit "util: Add ArgsManager::GetPathArg() function" (d32f6c0)

Nice tests!

hebasto · 2022-02-05T18:01:44Z

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 GetFileArg function 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.

Agree.

hebasto · 2022-02-05T18:06:55Z

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

PR description has been updated.

prusnak · 2022-02-05T18:40:06Z

I'd push back on these suggestions a little and save these things for followups.

Makes sense. Created a draft PR #24274 which builds on top of this PR.

hebasto · 2022-02-06T08:32:05Z

@prusnak

I'd push back on these suggestions a little and save these things for followups.

Makes sense. Created a draft PR #24274 which builds on top of this PR.

Mind leaving (N)ACK review comment?

src/util/system.h

src/util/system.cpp

prusnak

ACK 05c8d8c

maflcko · 2022-02-06T09:57:20Z

This is "refactoring"?

hebasto · 2022-02-06T10:03:38Z

@MarcoFalke

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

laanwj · 2022-02-09T15:49:32Z

Concept ACK.

Co-authored-by: Ryan Ofsky <[email protected]>

hebasto · 2022-02-09T17:43:19Z

Rebased 05c8d8c -> ebda2b8 (pr24265.03 -> pr24265.04) due to the conflict with #24266.

ryanofsky

Code review ACK ebda2b8. Only change since last review is rebase which simplified the last commit

prusnak · 2022-02-09T21:58:33Z

Since this PR has been merged to master, I rebased #24274 and marked it as ready for review.

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#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

60aa179 Use GetPathArg where possible (Pavol Rusnak) 5b946ed util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky) 687e655 util: Add GetPathArg default path argument (Ryan Ofsky) Pull request description: Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options. - Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values. - Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated. - Add unit tests for default and negated cases - Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable. ACKs for top commit: w0xlt: Tested ACK 60aa179 on Ubuntu 21.10 hebasto: re-ACK 60aa179 Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd

…idely b01f336 util, refactor: Drop explicit conversion to fs::path (Hennadii Stepanov) 138c668 util, refactor: Use GetPathArg to read "-rpccookiefile" value (Hennadii Stepanov) 1276090 util, refactor: Use GetPathArg to read "-conf" value (Hennadii Stepanov) Pull request description: This PR is a continuation of bitcoin/bitcoin#24265 and bitcoin/bitcoin#24306. Now the following command-line arguments / configure options been read with the `GetPathArg` method: - `-conf`, also `includeconf` values been normalized - `-rpccookiefile` ACKs for top commit: jarolrod: Code Review ACK b01f336 ryanofsky: Code review ACK b01f336. Changes since last review: just dropping first commit (NormalizedPathFromString) as suggested Tree-SHA512: 2d26d50b73542acdbcc63a32068977b2a49a017d31ca337471a0446f964eb0a6e3e4e3bb1ebe6771566a260f2cae3bc2ebe93b4b523183cea0d51768daab85c9

hebasto force-pushed the 220204-norm branch from 3f2e10e to 19d7a67 Compare February 4, 2022 21:33

DrahtBot added Utils/log/libs Wallet labels Feb 4, 2022

ryanofsky reviewed Feb 4, 2022

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 220204-norm branch from 19d7a67 to e69ab37 Compare February 4, 2022 22:32

This was referenced Feb 5, 2022

util: if DataDir is a symbolic link, manually follow it #24267

Closed

util: Avoid buggy std::filesystem:::create_directories() call #24266

Merged

DrahtBot mentioned this pull request Feb 5, 2022

Re-enable windows path tests disabled by #20744 #24251

Merged

DrahtBot added the Needs rebase label Feb 5, 2022

DrahtBot mentioned this pull request Feb 5, 2022

refactor: Remove gArgs from bdb.h and sqlite.h #23732

Merged

hebasto force-pushed the 220204-norm branch from e69ab37 to 05c8d8c Compare February 5, 2022 13:17

DrahtBot removed the Needs rebase label Feb 5, 2022

ryanofsky approved these changes Feb 5, 2022

View reviewed changes

prusnak mentioned this pull request Feb 5, 2022

Introduce GetFileArg and use it where possible #24274

Closed

hebasto changed the title ~~Drop StripRedundantLastElementsOfPath() function and re-enable Windows tests~~ Drop StripRedundantLastElementsOfPath() function Feb 5, 2022

prusnak reviewed Feb 6, 2022

View reviewed changes

src/util/system.h Show resolved Hide resolved

src/util/system.cpp Show resolved Hide resolved

prusnak approved these changes Feb 6, 2022

View reviewed changes

maflcko modified the milestone: 23.0 Feb 6, 2022

maflcko removed the Wallet label Feb 7, 2022

DrahtBot added the Needs rebase label Feb 8, 2022

hebasto and others added 5 commits February 9, 2022 19:31

util: Add ArgsManager::GetPathArg() function

540ca51

Co-authored-by: Ryan Ofsky <[email protected]>

Use ArgsManager::GetPathArg() for "-datadir" option

15b632b

Use ArgsManager::GetPathArg() for "-blocksdir" option

06fed4c

Use ArgsManager::GetPathArg() for "-walletdir" option

ecd094e

util: Drop no longer needed StripRedundantLastElementsOfPath() function

ebda2b8

hebasto force-pushed the 220204-norm branch from 05c8d8c to ebda2b8 Compare February 9, 2022 17:41

ryanofsky approved these changes Feb 9, 2022

View reviewed changes

DrahtBot removed the Needs rebase label Feb 9, 2022

fanquake merged commit 8c0f02c into bitcoin:master Feb 9, 2022

ryanofsky mentioned this pull request Feb 9, 2022

util: Make ArgsManager::GetPathArg more widely usable #24306

Merged

hebasto deleted the 220204-norm branch February 10, 2022 08:21

hebasto mentioned this pull request Mar 25, 2022

util: Use ArgsManager::GetPathArg more widely #24675

Merged

bitcoin locked and limited conversation to collaborators Feb 10, 2023

Conversation

hebasto commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Feb 4, 2022

Uh oh!

Uh oh!

Uh oh!

hebasto commented Feb 4, 2022

Uh oh!

fanquake commented Feb 5, 2022

Uh oh!

DrahtBot commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Feb 5, 2022

Uh oh!

prusnak commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Feb 5, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 5, 2022

Uh oh!

hebasto commented Feb 5, 2022

Uh oh!

prusnak commented Feb 5, 2022

Uh oh!

hebasto commented Feb 6, 2022

Uh oh!

Uh oh!

Uh oh!

prusnak left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 6, 2022

Uh oh!

hebasto commented Feb 6, 2022

Uh oh!

laanwj commented Feb 9, 2022

Uh oh!

hebasto commented Feb 9, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

prusnak commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Feb 4, 2022 •

edited

Loading

DrahtBot commented Feb 5, 2022 •

edited

Loading

prusnak commented Feb 5, 2022 •

edited

Loading