[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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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 ↗	(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 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 ↗	(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.

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

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

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 ↗	(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.