Introduce GetFileArg and use it where possible #24274

pull prusnak wants to merge 2 commits into bitcoin:master from prusnak:GetFileArg changing 5 files +33 −10

prusnak commented at 6:39 PM on February 5, 2022: contributor

Follow-up to #24265

~Marking as draft until #24265 is merged as this PR builds on top of that PR~
prusnak force-pushed on Feb 5, 2022
DrahtBot added the label Utils/log/libs on Feb 5, 2022
DrahtBot added the label Wallet on Feb 5, 2022
DrahtBot commented at 9:26 PM on February 5, 2022: member


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.
in src/util/system.cpp:389 in f49b27b1c6 outdated
```
 385 | @@ -399,6 +386,23 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
 386 |      return std::nullopt;
 387 |  }
 388 |  
 389 | +fs::path ArgsManager::GetFileArg(std::string pathlike_arg, const fs::path& pathDefault) const
```
kiminuo commented at 8:03 AM on February 6, 2022:

pathlike_arg is in snake case notation, pathDefault is in camel case notation.

prusnak commented at 9:23 AM on February 6, 2022:

Right. Left review on the PR this PR builds on: #24265#pullrequestreview-874013646

prusnak commented at 9:54 AM on February 6, 2022:

Addressed in c71437c93611094aa57eb7da0e166159ff128bdd
prusnak force-pushed on Feb 6, 2022
DrahtBot added the label Needs rebase on Feb 8, 2022
prusnak force-pushed on Feb 9, 2022
DrahtBot removed the label Needs rebase on Feb 9, 2022
Introduce GetFileArg 42688109b3
Use GetFileArg where possible 7db1e1fa73
prusnak force-pushed on Feb 9, 2022
prusnak marked this as ready for review on Feb 9, 2022
ryanofsky commented at 11:14 PM on February 9, 2022: member

Concept ACK, but I think PR description should be clearer. Also, I think it would be better to drop the first commit 42688109b38063902f4c2621db3e465a3c9b2218 here, and substitute commit b00c0c2a73b3c443f78b74eee60ff3e401b0269f from #24306. Main reason is that I think it is confusing to have separate GetPathArg and GetFileArg methods which are exactly the same except one strips trailing slashes and one doesn't. I don't think it's likely we will have a need for the one which preserves trailing slashes. The commit from the other PR also adds unit test coverage.
prusnak commented at 10:27 PM on February 10, 2022: contributor

Let's close this in favor of https://github.com/bitcoin/bitcoin/pull/24306
prusnak closed this on Feb 10, 2022
prusnak deleted the branch on Feb 10, 2022
MarcoFalke referenced this in commit 6687bb24ae on Mar 7, 2022
DrahtBot locked this on Feb 10, 2023

Contributors

Labels

Wallet Utils/log/libs

Linked (view graph)

#24265 Drop StripRedundantLastElementsOfPath() function #24306 util: Make ArgsManager::GetPathArg more widely usable

Introduce GetFileArg and use it where possible #24274

Conflicts