Paths

Table of Contentst

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

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

Details

Reviewers

jasonbcox
schancel

Group Reviewers

Restricted Project

Commits

rSTAGINGd498617f8cdf: [refactor] Config handling refactoring in preparation for network-specific…
rABCd498617f8cdf: [refactor] Config handling refactoring in preparation for network-specific…

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

deadalnix created this revision.Sep 28 2018, 14:46

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 28 2018, 14:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3433: Diff 5155.Sep 28 2018, 15:05

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 requested review of this revision.Sep 29 2018, 08:46

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.

rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B3442: Diff 5176.Sep 29 2018, 14:28

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

schancel added inline comments.Sep 29 2018, 21:47

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 marked an inline comment as done.Sep 29 2018, 22:59

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B3446: Diff 5180.Sep 29 2018, 23:24

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

jasonbcox accepted this revision.Sep 30 2018, 16:59

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.