Fix argument parsing oddity with -noX #6284

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2015_06_argument_parsing_oddity changing 3 files +38 −39

laanwj commented at 3:30 PM on June 15, 2015: member

bitcoind -X -noX ends up, unintuitively, with X set. (for all boolean options X)

This result is due to the odd two-pass processing of arguments. This patch fixes this oddity (by always taking the latter option setting) and simplifies the code at the same time.

Discovered while testing #6272.
laanwj added the label Bug on Jun 15, 2015
laanwj added the label Priority Low on Jun 15, 2015
laanwj force-pushed on Jun 15, 2015
laanwj commented at 3:54 PM on June 15, 2015: member

Hmm somehow the tests also check the old behavior. I don't understand. It seems very counter-intuitive. Updated the tests, although this may make the change more controversial.
sipa commented at 1:46 PM on June 16, 2015: member

I also don't understand the reasoning behind the old semantics.

fanquake commented at 10:07 PM on June 16, 2015: member

Current behaviour in latest master

src/qt/bitcoin-qt -proxy=0 ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -proxy=1.0.0.0 ->> -proxy=1.0.0.0 is set.
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy ->>  -proxy=1.0.0.0 is set.
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy=0 ->> -proxy=1.0.0.0 is set
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy=1 ->> -proxy=1.0.0.0 is set
src/qt/bitcoin-qt -noproxy -proxy=1.0.0.0 ->>  -proxy=1.0.0.0 is set
src/qt/bitcoin-qt -noproxy ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -noproxy=0 ->>  -proxy=1 is set
src/qt/bitcoin-qt -noproxy=1 ->> Invalid -proxy address: '0'

After applying this pull

src/qt/bitcoin-qt -proxy=0 ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -proxy=1.0.0.0 ->> -proxy=1.0.0.0 is set.
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy=0 ->> -proxy=1 is set
src/qt/bitcoin-qt -proxy=1.0.0.0 -noproxy=1 ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -noproxy -proxy=1.0.0.0 ->> -proxy=1.0.0.0 is set
src/qt/bitcoin-qt -noproxy ->> Invalid -proxy address: '0'
src/qt/bitcoin-qt -noproxy=0 ->> -proxy=1 is set
src/qt/bitcoin-qt -noproxy=1 ->> Invalid -proxy address: '0'

laanwj commented at 7:07 AM on June 17, 2015: member

@fanquake Proxy is not a boolean option, although -noproxy will be valid after #6272.

I agree that these -noX=Y semantics are a tad strange as well (which assigns !Y to X), but decided to keep them to make this not too big of a change. For me personally, though, -noX only makes sense without argument. I've never felt the desire to use, say -noX=0 to say -X=1.
luke-jr commented at 6:17 AM on June 23, 2015: member

I suspect the reasoning for this, was that -noX is meant to be deprecated, while -X=0 is the new form. I don't really care either way about changing it, though...
laanwj commented at 7:59 AM on June 23, 2015: member

I suspect the reasoning for this, was that -noX is meant to be deprecated

What makes you think that? -noX was introduced later (only in 0.6, according to the comment) while -X=0 was (I suppose?) always possible.
laanwj commented at 10:08 AM on July 21, 2015: member

Behavior change needs a mention in doc/release-notes.md [done]

Fix argument parsing oddity with -noX

`bitcoind -X -noX` ends up, unintuitively, with `X` set.
(for all boolean options X)

This result is due to the odd two-pass processing of arguments. This
patch fixes this oddity and simplifies the code at the same time.

c38c49d0b7

doc: mention change to option parsing behavior in release notes c6455c77ab
laanwj force-pushed on Jul 27, 2015
laanwj merged this on Aug 3, 2015
laanwj closed this on Aug 3, 2015
laanwj referenced this in commit 10ac38ed9f on Aug 3, 2015
zkbot referenced this in commit f1aeaec471 on Mar 21, 2018
zkbot referenced this in commit 4fc490c430 on Dec 4, 2019
zkbot referenced this in commit 868c63f92d on Dec 4, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6272 Improve proxy initialization (continues #4871)