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.

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

Merge settings one place instead of five places #15934

Merged
merged 6 commits into from Nov 8, 2019

Conversation

@ryanofsky
Copy link
Contributor

ryanofsky commented May 1, 2019

This is a refactoring-only change that makes it easier to add a new settings source.

This PR doesn't change behavior. The util_ArgsMerge and util_ChainMerge tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

This change:

The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent -wallet setting are pretty straightforward and show how the new code can be extended:

  • 70675c3 from #15935Add <datadir>/settings.json persistent settings storage
  • 04c80c4 from #15937Add loadwallet and createwallet RPC load_on_startup options
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented May 2, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17385 (WIP: refactor: Use our own integer parsing/formatting everywhere by laanwj)
  • #15454 (Remove the automatic creation and loading of the default wallet by achow101)
  • #14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
  • #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

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.

Copy link
Member

promag left a comment

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

Concept ACK.

src/util/system.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch 5 times, most recently from 19e84b7 to 1d543ad May 2, 2019
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented May 8, 2019

re: #15934 (review) from promag

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

This change does make it easier to add new settings sources (with consistent handling of negated args and things), so it should be compatible with your idea and maybe helpful.

Depending on the situation, I think having chained or scoped settings could be a good idea or not. I do think that in wallet code and application code generally it's good to get away from using key-value storage classes like ArgsManager or UniValue as quickly as possible, and switch to more direct representations like CCoinControl that are type safe and can be accessed more simply.

@ryanofsky ryanofsky changed the title WIP: Dedup settings merge code Dedup settings merge code May 14, 2019
@ryanofsky ryanofsky marked this pull request as ready for review May 14, 2019
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented May 24, 2019

Concept ACK

Copy link
Member

jamesob left a comment

Tested ACK 1d543ad

Generated and reviewed the test output locally. Mucked around with various argument formulations using the following config file:

dbcache=100
[main]
dbcache=200
[test]
dbcache=300

and commandline invocations e.g.

./src/bitcoind -conf=$(pwd)/test.conf -dbcache=1000 -dbcache=500 | grep Using

to verify dbcache being set as expected.


This is a well-written change that cleans up a lot of gnarly, duplicated settings munging. It explicitly outlines surprising corner cases in existing behavior (with docs too), and makes reasoning about settings merge order easier. This change also introduces substantial test coverage to settings management (util::Settings).

After this is merged, adding a read-write settings file (whether it's JSON or something else) will be much easier.

src/util/settings.cpp Outdated Show resolved Hide resolved
src/util/settings.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
// Weird behavior preserved for backwards compatibility: command line
// options with section prefixes are allowed but ignored. It would be
// better if these options triggered the IsArgKnown error below, or were
// actually used instead of silently ignored.

This comment has been minimized.

Copy link
@jamesob

jamesob May 28, 2019

Member

Thanks for the nice comment. Potentially out of scope: could we log warnings for this instead of silently ignoring?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 29, 2019

Author Contributor

re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

// test parses and merges settings, representing the results as strings that get
// compared against an expected hash. To debug, the result strings can be dumped
// to a file (see comments below).
BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup)

This comment has been minimized.

Copy link
@jamesob

jamesob May 28, 2019

Member

Cool test! The formatted output is really helpful. Encourage other reviewers to run and inspect with

SETTINGS_MERGE_TEST_OUT=results.txt ./src/test/test_bitcoin --run_test=settings_tests/Merge
src/test/util.h Outdated Show resolved Hide resolved
src/test/settings_tests.cpp Show resolved Hide resolved
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch from 1d543ad to 955c782 May 29, 2019
@ryanofsky ryanofsky changed the title Dedup settings merge code Separate settings merging from parsing May 29, 2019
Copy link
Contributor Author

ryanofsky left a comment

Updated 1d543ad -> 2dfeff1 (pr/mergeset.6 -> pr/mergeset.7), compare) with suggested changes.
Rebased 2dfeff1 -> 955c782 (pr/mergeset.7 -> pr/mergeset.8) to share common code with #15988.

src/test/settings_tests.cpp Show resolved Hide resolved
src/util/settings.cpp Outdated Show resolved Hide resolved
src/util/settings.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
// Weird behavior preserved for backwards compatibility: command line
// options with section prefixes are allowed but ignored. It would be
// better if these options triggered the IsArgKnown error below, or were
// actually used instead of silently ignored.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 29, 2019

Author Contributor

re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

src/test/util.h Outdated Show resolved Hide resolved
@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented May 29, 2019

re-tACK 955c782 based on the interdiff and running an abbreviated version of the testing above.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch from 955c782 to 14a6dfc Jun 27, 2019
@DrahtBot DrahtBot removed the Needs rebase label Jun 27, 2019
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Jun 27, 2019

Rebased 955c782 -> 14a6dfc (pr/mergeset.8 -> pr/mergeset.9) due to conflict with #16278

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Jun 27, 2019

reACK 14a6dfc based on interdiff. Only change since pr/mergeset.8 is a trivial LogPrintf fix.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch from 14a6dfc to d074e43 Jun 28, 2019
ryanofsky and others added 4 commits Mar 5, 2019
Implement merging of settings from different sources (command line and config
file) separately from parsing code in system.cpp, so it is easier to add new
sources.

Document current inconsistent merging behavior without changing it.

This commit only adds new settings code without using it. The next commit calls
the new code to replace existing code in system.cpp.

Co-authored-by: John Newbery <john@johnnewbery.com>
Get rid of settings merging code in util/system.cpp repeated 5 places,
inconsistently:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).

Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.

This commit does not change behavior in any way.
Co-authored-by: James O'Beirne <james.obeirne@gmail.com>
Easier to review ignoring whitespace

Suggestion from John Newbery <john@johnnewbery.com> in
#15934 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 8, 2019
Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 8, 2019
includeconf -> conf_file_names
to_include -> conf_file_name
include_config -> conf_file_stream

Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 8, 2019
Easier to review ignoring whitespace

Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch from 202e198 to 20f74e2 Nov 8, 2019
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Nov 8, 2019

Updated 202e198 -> 20f74e2 (pr/mergeset.23 -> pr/mergeset.24, compare) with suggestions above.
Rebased 20f74e2 -> 083c954 (pr/mergeset.24 -> pr/mergeset.25) to fix silent merge conflict with #17384 (fatal error: 'test/setup_common.h' file not found')

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/mergeset branch from 20f74e2 to 083c954 Nov 8, 2019
// passed '-noincludeconf' on the command line, in which case we should not include anything
bool emptyIncludeConf;
// `-includeconf` cannot be included in the command line arguments except
// as `-noincludeconf` (which indicates that no conf file should be used).

This comment has been minimized.

Copy link
@ariard

ariard Nov 8, 2019

Contributor

I find comment a bit confusing, -noincludeconf means we are only going to discard includeconf in config file but not the main config itself (at least I tested that the current behavior with this patchset AFAII)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 8, 2019

Author Contributor

re: #15934 (comment)

I find comment a bit confusing, -noincludeconf means we are only going to discard includeconf in config file but not the main config itself (at least I've tested that the current behavior with this patchset)

-noincludeconf just disables the -includeconf directive. I wouldn't expect it to disable the whole config file. (I might expect -noconf to do that.) So this doesn't seem that confusing to me, but I'm happy to take a suggestion if you have an idea to improve it.

The current text just comes from John's suggestion #15934 (comment)

This comment has been minimized.

Copy link
@ariard

ariard Nov 8, 2019

Contributor

nit-picking there, was just to change comment to (which indicates that no included conf file should be used)

@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Nov 8, 2019

ACK 083c954

I've reviewed that's new code is doing what it says to do. I think that weird behaviors are compatibility-maintained but at least if we have a regression they are well-documented and so we'll know where to look, plus as said by James this part of code is low-risk so it's worth moving forward.

Great work Russ, and I'm concept ACK future PRs to remove/squeeze weird behaviors!

Copy link
Member

jnewbery left a comment

ACK 083c954

error += "-includeconf cannot be used from commandline; -includeconf=" + ic + "\n";
}
return false;
bool success = true;

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 8, 2019

Member

nit (please don't change in this PR. Leave for a follow-up, if at all): 'we clear it' in the comment above is inaccurate. If an -includeconf is found on the command line then we return false rather than silently removing the bad config. Change that comment to // Do not allow -includeconf from the command line (except for -noincludeconf)

In fact, I don't think we need the local success variable at all. You could just return false from the inner for loop. I don't think the user needs to be told about the multiple -includeconf command line arguments they've used. Just alerting about the first should be enough.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Nov 8, 2019

Thanks! Will leave PR at 083c954. Some notes for future followup:

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 8, 2019

Other potential future follow-ups:

  • Don't throw in GetChainName()

(

throw std::runtime_error("Invalid combination of -regtest, -testnet and -chain. Can use at most one.");
). I think the calling code was originally supposed to catch this (eg
SelectParams(gArgs.GetChainName());
), but this is no longer the case because GetChainName() is called in ReadConfigFiles(), and so starting with -regtest -testnet results in an unhandled exception.

  • Remove ArgsManagerHelper()

Here in master:

class ArgsManagerHelper {
. After this PR there are only two functions in this class. Just move them into ArgsManager(). The class was only added to speed up rebuilding (#11862 (comment)), which doesn't seem like a good reason to keep it.

  • Move the ArgsManager code into util/settings ?
@jamesob
jamesob approved these changes Nov 8, 2019
Copy link
Member

jamesob left a comment

ACK 083c954

Reviewed diff-of-diffs both on Github and locally. Only changes since my last ACK are those discussed above. Nice job persevering, @ryanofsky, and thanks to @jnewbery @ariard for good feedback.

// precedence over early settings, but for backwards compatibility in
// the config file the precedence is reversed for all settings except
// chain name settings.
const bool reverse_precedence = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !get_chain_name;

This comment has been minimized.

Copy link
@jamesob

jamesob Nov 8, 2019

Member

These long lines are killin' me but I'll bite my tongue in the interest of a merge.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 8, 2019

Author Contributor

Will fix with the other followups, but if it in case its useful https://greasyfork.org/en/scripts/18789-github-toggle-code-wrap seems to work well. (Found through https://github.com/StylishThemes/GitHub-code-wrap)

laanwj added a commit that referenced this pull request Nov 8, 2019
083c954 Add settings_tests (Russell Yanofsky)
7f40528 Deduplicate settings merge code (Russell Yanofsky)
9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cf Remove includeconf nested scope (Russell Yanofsky)
5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky)

Pull request description:

  This is a refactoring-only change that makes it easier to add a new settings source.

  This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

  This change:

  - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from #15935).
  - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways.
  - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108).

  The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:

  * 70675c3 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
  * 04c80c4 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_

ACKs for top commit:
  ariard:
    ACK 083c954
  jnewbery:
    ACK 083c954
  jamesob:
    ACK 083c954

Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
@laanwj laanwj merged commit 083c954 into bitcoin:master Nov 8, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
includeconf -> conf_file_names
to_include -> conf_file_name
include_config -> conf_file_stream

Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
Easier to review ignoring whitespace

Suggestion from John Newbery <john@johnnewbery.com> in
bitcoin#15934 (comment)
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
083c954 Add settings_tests (Russell Yanofsky)
7f40528 Deduplicate settings merge code (Russell Yanofsky)
9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cf Remove includeconf nested scope (Russell Yanofsky)
5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky)

Pull request description:

  This is a refactoring-only change that makes it easier to add a new settings source.

  This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

  This change:

  - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from bitcoin#15935).
  - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways.
  - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108).

  The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:

  * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_
  * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_

ACKs for top commit:
  ariard:
    ACK 083c954
  jnewbery:
    ACK 083c954
  jamesob:
    ACK 083c954

Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
@fanquake fanquake removed this from Blockers in High-priority for review Nov 14, 2019
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Suggested by James O'Beirne <james.obeirne@gmail.com>
bitcoin#15934 (comment)

This commit does not change behavior.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Suggested by Antoine Riard <ariard@student.42.fr>
bitcoin#15934 (comment)

and John Newbery <john@johnnewbery.com>
bitcoin#15934 (comment)

This commit does not change behavior.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
bitcoin#16545 (comment)

This also gets rid of ArgsManager::NONE constant, which was an implementation
detail not meant to be used by ArgsManager callers.

Finally this reverts a change from 7f40528
bitcoin#15934 adding "-" characters to argument
names. Better for GetArgFlags to require "-" prefixes for consistency with
other argument methods, and to be more efficient later when GetArg functions
need to call GetArgFlags.

This commit does not change behavior.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Suggested by John Newbery <john@johnnewbery.com>
bitcoin#15934 (comment)

This commit does not change behavior.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
bitcoin#16545 (comment)

This also gets rid of ArgsManager::NONE constant, which was an implementation
detail not meant to be used by ArgsManager callers.

Finally this reverts a change from 7f40528
bitcoin#15934 adding "-" characters to argument
names. Better for GetArgFlags to require "-" prefixes for consistency with
other ArgsManager methods, and to be more efficient later when GetArg functions
need to call GetArgFlags (bitcoin#16545)

This commit does not change behavior.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Suggested by John Newbery <john@johnnewbery.com>
bitcoin#15934 (comment)

This commit does not change behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.