Page MenuHomePhabricator

ArgsManager: keep command line and config file arguments separate
ClosedPublic

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

Details

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

jasonbcox requested changes to this revision.Jan 29 2019, 18:26
jasonbcox added a subscriber: jasonbcox.

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.

Fabien planned changes to this revision.Jan 29 2019, 19:17

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

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.

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
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

doc/release-notes.md
15 ↗(On Diff #6993)

I think we should only take ownership on this after our backports are more caught up. I agree with the sentiment though.

deadalnix added inline comments.
doc/release-notes.md
15 ↗(On Diff #6993)

Let's catch up first and then change it if it is worth it.

This revision is now accepted and ready to land.Feb 5 2019, 04:12
This revision was automatically updated to reflect the committed changes.