Page MenuHomePhabricator

ArgsManager: keep command line and config file arguments separate
ClosedPublic

Authored by Fabien on Tue, Jan 29, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Tue, Jan 29, 13:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jan 29, 13:33
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Tue, Jan 29, 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.Tue, Jan 29, 18:26

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

Fabien added a comment.Tue, Jan 29, 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.Tue, Jan 29, 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.Tue, Jan 29, 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.Tue, Jan 29, 19:21

Update release notes

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.Wed, Jan 30, 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.Wed, Jan 30, 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.Wed, Jan 30, 23:05
Fabien added inline comments.Thu, Jan 31, 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

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

Fix comments layout

jasonbcox added inline comments.Thu, Jan 31, 18:15
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 accepted this revision.Thu, Jan 31, 22:15
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.

jasonbcox accepted this revision.Tue, Feb 5, 04:12
This revision is now accepted and ready to land.Tue, Feb 5, 04:12
Closed by commit rABCf9092cd61146: ArgsManager: keep command line and config file arguments separate (authored by Anthony Towns <aj@erisian.com.au>, committed by Fabien). · Explain WhyTue, Feb 5, 07:54
This revision was automatically updated to reflect the committed changes.