Page MenuHomePhabricator

ArgsManager: keep command line and config file arguments separate

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



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

rABC Bitcoin ABC
Lint OK
No Unit 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 subscriber: schancel. · View Herald Transcript
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: 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?

441 ↗(On Diff #6983)

Why was this default check removed?

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.

Fabien added a comment.Jan 29 2019, 19:01

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

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.

Fabien updated this revision to Diff 6993.Jan 29 2019, 19:21

Update release notes

Will green once release notes are in a reviewable state.

441 ↗(On Diff #6983)

Ah ya, makes sense.

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

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

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)


Fabien updated this revision to Diff 7047.Jan 31 2019, 10:25

Fix comments layout

jasonbcox added inline comments.Jan 31 2019, 18:15
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 accepted this revision.Jan 31 2019, 22:15
deadalnix added inline comments.
15 ↗(On Diff #6993)

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

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