Paths

Table of Contentst

ArgsManager: keep command line and config file arguments separate
ClosedPublic
Actions

Authored by Fabien on Jan 29 2019, 13:33.

Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGINGf9092cd61146: ArgsManager: keep command line and config file arguments separate
rABCf9092cd61146: ArgsManager: keep command line and config file arguments separate

Summary

This makes it possible to know whether an argument is set from the
command line or the config file.

Behavior change: in case of multiple values for an argument, the
following rules apply:

From the command line, GetArg returns the *last* value
From the config file, GetArg return the *first* value

Partial backport of core PR11862 (commit 3673ca3)

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

PR11862_part1

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4781
Build 7625: Bitcoin ABC Buildbot (legacy)
Build 7624: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 29 2019, 13:33

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 29 2019, 13:33

Herald added a reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Fabien added a child revision: D2433: ArgsManager: drop m_negated_args.Jan 29 2019, 14:00

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

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

Harbormaster completed remote builds in B4736: Diff 6983.Jan 29 2019, 14:51

I see the last commit in that PR has release notes: https://github.com/bitcoin/bitcoin/pull/11862/commits/c25321ff96737bdba80d626d2425ef02c7a4c181 Since you're breaking these changes up, do you think updating the release notes to match the current state makes sense, or are you going to just tack c25321ff onto the end once you're done?

src/test/util_tests.cpp
441 ↗	(On Diff #6983)	Why was this default check removed?
src/util.cpp
222 ↗	(On Diff #6983)	I don't know why they did this, but the disparity between config and CLI argument reading seems like a poor design... But I don't intend to own this, so will let it slide.

This revision now requires changes to proceed.Jan 29 2019, 18:26

Yes please, add release notes along side with what they refer to.

Release notes are updated as new user facing features are added. The last commit c25321f is split in 2 parts, in D2434 and D2435.

Not only the new features, but also the behavior changes. The behavior changes are actually more important, IMO, as some people have complex configurations that could mysteriously break.

Fabien requested review of this revision.Jan 29 2019, 19:15

Fabien added inline comments.

src/test/util_tests.cpp
441 ↗	(On Diff #6983)	It doesn't makes sense anymore (and never really made sense imo, read below). mapArgs and multMapArgs used to be completely separated, such as if only mapMultiArg was set GetArg() will return the default, as mapArgs is empty. There is no such thing anymore. Cli args and config args both are an equivalent of mapMultiArg (a map of string to vector), so GetxxxArg() will always return something if there is something in the map. Please note that what it returns differs depending if this is a cli arg or a config arg (see the summary).
src/util.cpp
222 ↗	(On Diff #6983)	It actually makes sense for the command-line argument to rely on the last one (every time the argument is encountered, the previous one get overridden). Up to my knowledge this is the behavior of every command line software. Regarding the configuration, I don't really understand why the first occurrence should be considered rather than the last one ? I agree with you that having both methods consistent would have been more user friendly.

OK, I get your point. I will add an entry in the release notes for the argument ordering behavior.

Update release notes

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

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

Harbormaster failed remote builds in B4744: Diff 6993!Jan 29 2019, 19:36

Will green once release notes are in a reviewable state.

src/test/util_tests.cpp
441 ↗	(On Diff #6983)	Ah ya, makes sense.
src/util.cpp
222 ↗	(On Diff #6983)	Ya, the first part makes sense. As for why config should be treated the opposite puzzles me. It may have to do with the not-yet-backported feature that allows specifying different network configs in the same file. But I fail to see why first-seen is somehow better/easier to implement than last-seen even in that case.

Fabien added inline comments.Jan 30 2019, 07:20

src/util.cpp
222 ↗	(On Diff #6983)	It may have to do with the not-yet-backported feature that allows specifying different network configs in the same file This is in the same PR, see D2434

deadalnix requested changes to this revision.Jan 30 2019, 23:05

deadalnix added inline comments.

doc/release-notes.md
15 ↗	(On Diff #6993)	Holy shit, it's so retarded... That being said current behavior isn't that much better so let's go for it.
src/util.cpp
177 ↗	(On Diff #6993)	I love how core dev don't seems o understand what `inline` is for. hint: it's not a hint for the compiler to inline, but for the linker to use ODR. In fact, many compilers will increase the inlining cost of inline function. That being said, the C devs share a good chunk of the blame on that one.
188 ↗	(On Diff #6993)	Can you use the proper layout for these ? Namely `/**` on its own line.

This revision now requires changes to proceed.Jan 30 2019, 23:05

Fabien added inline comments.Jan 31 2019, 06:46

doc/release-notes.md
15 ↗	(On Diff #6993)	This would be an easy fix if we want to change it in another diff, but this could make the behavior confusing to users that are used to it. Thought ?
src/util.cpp
177 ↗	(On Diff #6993)	Thanks for pointing this out. They are useless here but have no drawback, so I think I should let them to avoid creating merge conflicts.
188 ↗	(On Diff #6993)	Sure