Skip to content

Fix argument parsing oddity with -noX #6284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 3, 2015
Merged

Fix argument parsing oddity with -noX #6284

merged 2 commits into from
Aug 3, 2015

Conversation

laanwj
Copy link
Member

@laanwj 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 laanwj force-pushed the 2015_06_argument_parsing_oddity branch from 7d9e0ef to d3ff291 Compare June 15, 2015 15:53
@laanwj
Copy link
Member Author

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
Copy link
Member

sipa commented Jun 16, 2015

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

@fanquake
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
Member Author

laanwj commented Jul 21, 2015

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

laanwj added 2 commits July 27, 2015 14:47
@laanwj
Fix argument parsing oddity with -noX
c38c49d 
`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
doc: mention change to option parsing behavior in release notes
c6455c7
@laanwj laanwj force-pushed the 2015_06_argument_parsing_oddity branch from d3ff291 to c6455c7 Compare July 27, 2015 12:55
@laanwj laanwj merged commit c6455c7 into bitcoin:master Aug 3, 2015
laanwj added a commit that referenced this pull request Aug 3, 2015
@laanwj
Merge pull request #6284
10ac38e 
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 mentioned this pull request Apr 23, 2017
Merged
This was referenced May 18, 2017
Open
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018
@zkbot
Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
f1aeaec 
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.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 4, 2019
@zkbot
Auto merge of #2390 - str4d:2132-mapargs-prep, r=str4d
868c63f 
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#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.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laanwj @sipa @fanquake @luke-jr