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 3446
Build 4968: Bitcoin ABC Buildbot (legacy)
Build 4967: 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 ↗(On Diff #5155)

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

210 ↗(On Diff #5155)

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 ↗(On Diff #5155)

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

206 ↗(On Diff #5155)

Doesn't this imply foo is just unset?

220 ↗(On Diff #5155)

Why is this 0?

225 ↗(On Diff #5155)

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

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

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

204 ↗(On Diff #5155)

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

206 ↗(On Diff #5155)

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

210 ↗(On Diff #5155)

Once again, double negative.

220 ↗(On Diff #5155)

Command line argument takes priority over config file.

src/test/util_tests.cpp
206 ↗(On Diff #5155)

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 ↗(On Diff #5155)

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 ↗(On Diff #5155)

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 ↗(On Diff #5155)

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 ↗(On Diff #5155)

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