Fix argument parsing oddity with -noX #6284

Merged
merged 2 commits into from Aug 3, 2015

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Jun 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented Jun 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 16, 2015

Member

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

Member

sipa commented Jun 16, 2015

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

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Jun 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'
Member

fanquake commented Jun 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented Jun 17, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 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...

Member

luke-jr commented Jun 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented Jun 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 21, 2015

Member

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

Member

laanwj commented Jul 21, 2015

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

laanwj added some commits Jun 15, 2015

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.

@laanwj laanwj merged commit c6455c7 into bitcoin:master Aug 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 3, 2015

Merge pull request #6284
c6455c7 doc: mention change to option parsing behavior in release notes (Wladimir J. van der Laan)
c38c49d Fix argument parsing oddity with -noX (Wladimir J. van der Laan)

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Apr 23, 2017

Merged

Util ports #197

zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018

Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment