Page MenuHomePhabricator

[refactor] Config handling refactoring in preparation for network-specific sections
ClosedPublic

Authored by deadalnix on Sep 28 2018, 14:46.

Details

Summary
  • Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName
  • [tests] Add unit tests for GetChainName
  • Separate out ReadConfigStream from ReadConfigFile
  • ReadConfigStream: assume the stream is good
  • [tests] Add unit tests for ReadConfigStream
  • [tests] Check GetChainName works with config entries
  • [tests] Add additional unit tests for -nofoo edge cases

This is a backport of Core's PR12878

Depends on D1833

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
configfilerefac
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3433
Build 4942: Bitcoin ABC Buildbot (legacy)
Build 4941: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Sep 28 2018, 17:55
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/util_tests.cpp
204

Why would an override only partially override (as in, applies negativity but not value)? This seems totally wrong to me.

210

Strangely, this returns "1" when -nobar was set to "0" both times (lines 187 and 199).

This revision now requires changes to proceed.Sep 28 2018, 17:55
schancel requested changes to this revision.Sep 28 2018, 21:46
schancel added a subscriber: schancel.
schancel added inline comments.
src/test/util_tests.cpp
196

How does the string "0" become the string "1"?

206

Doesn't this imply foo is just unset?

220

Why is this 0?

225

I give up. This argument handling seems insane. I'm going to have to read through the parser.

src/util.cpp
485
 if (fRegTest) {
    return CBaseChainParams::REGTEST;
}
...
deadalnix marked 5 inline comments as done.
deadalnix added inline comments.
src/test/util_tests.cpp
196

Because a double negative is a positive, as stated in the comment. -nobar=0

204

Arguably not the best semantic, but that's how it is.

206

If it was unset, you'd get "xxx"

210

Once again, double negative.

220

Command line argument takes priority over config file.

src/test/util_tests.cpp
206

You're not understanding the question. It should be unset, since both true and false are both true.

You have "-nofoo -nofoo=1 -foo=1". How does this evaluate to 0?

deadalnix added inline comments.
src/test/util_tests.cpp
206

second one overrides the negative setting, but not the value.

Not advocating this semantic is the best (in fact it's rather disgusting), but that's what it is now and having it tested is better than not.

Add braces in ArgsManager::GetChainName

jasonbcox added inline comments.
src/test/util_tests.cpp
204

I'll accept this diff because it's adding tests for existing behavior and not changing it. Please create a task to redo the args semantic and reference it in a comment on this diff for posterity.

210

Confusing, but I understand this one now.

This revision is now accepted and ready to land.Oct 9 2018, 00:32
deadalnix added inline comments.
src/test/util_tests.cpp
204

There are other PR we can backport that do just that. But it's all blocked on T417 .