Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: Use ArgsManager::GetPathArg more widely #24675

Merged
merged 3 commits into from
Aug 4, 2022
Merged

util: Use ArgsManager::GetPathArg more widely #24675

merged 3 commits into from
Aug 4, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 25, 2022

This PR is a continuation of #24265 and #24306.

Now the following command-line arguments / configure options been read with the GetPathArg method:

  • -conf, also includeconf values been normalized
  • -rpccookiefile

@hebasto hebasto mentioned this pull request Mar 26, 2022
Merged
@hebasto
Copy link
Member Author

hebasto commented Apr 4, 2022

Friendly ping @ryanofsky @prusnak

@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

Concept ACK

@jarolrod
Copy link
Member

jarolrod commented Apr 8, 2022

concept ack

@DrahtBot DrahtBot mentioned this pull request Apr 9, 2022
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 10, 2022 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Apr 10, 2022
Closed
ryanofsky
ryanofsky approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 92aacda, but it would be good to consider suggestions related to the NormalizedPathFromString function.

src/util/system.cpp Outdated Show resolved Hide resolved
src/fs.h Outdated Show resolved Hide resolved
@hebasto
Copy link
Member Author

hebasto commented Apr 21, 2022

@ryanofsky

Thank you for your review! Agree with all of points you mentioned above.

Updated.

@DrahtBot DrahtBot mentioned this pull request Jun 8, 2022
Closed
jarolrod
jarolrod reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK b01f336

This is a nice cleanup and it is correct to expand the usage of GetPathArg to conf settings that are being passed a path, obviously :)

ryanofsky
ryanofsky approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK b01f336. Changes since last review: just dropping first commit (NormalizedPathFromString) as suggested

@fanquake fanquake merged commit e09ad28 into bitcoin:master Aug 4, 2022
@hebasto hebasto deleted the 220325-path branch August 4, 2022 16:12
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2022
@fanquake
Merge bitcoin#24675: util: Use ArgsManager::GetPathArg more widely
bef507a
@bitcoin bitcoin locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jarolrod jarolrod jarolrod left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@hebasto @laanwj @jarolrod @DrahtBot @ryanofsky @fanquake @maflcko