Disallow double negation syntax (-nofoo=0) for options

hebasto commented at 5:44 pm on July 31, 2019: member

The double negation syntax for options (-nofoo=0) is confusing and doesn’t have any uses that make sense.

This PR disallows the double negation syntax for boolean type options which are marked with the ArgsManager::ALLOW_BOOL flag. Please note, that non-boolean options (without the ArgsManager::ALLOW_BOOL flag) cannot be negated at all after #16097.

With this PR:

0$ src/bitcoind -nolisten=0
1Error: Error parsing command line arguments: Double-negative -nolisten=0 is not allowed

NOTES for reviewers:

this PR is based on top of #16097, so only the last three commits are to be reviewed here
ref: 4d34fcc7138f0ffc831f0f8601c50cc7f494c197
release notes for 0.6.0:

The -nolisten, -noupnp and -nodnsseed command-line options were renamed to -listen, -upnp and -dnsseed, with a default value of 1. The old names are still supported for compatibility (so specifying -nolisten is automatically interpreted as -listen=0; every boolean argument can now be specified as either -foo or -nofoo).

DrahtBot added the label GUI on Jul 31, 2019

DrahtBot added the label Tests on Jul 31, 2019

DrahtBot added the label Utils/log/libs on Jul 31, 2019

DrahtBot added the label Validation on Jul 31, 2019

DrahtBot added the label Wallet on Jul 31, 2019

hebasto force-pushed on Jul 31, 2019

hebasto commented at 8:41 pm on July 31, 2019: member

ping @jonasschnelli @MarcoFalke @promag @ryanofsky

ryanofsky commented at 9:12 pm on July 31, 2019: member

As a change to selectively change the current double-negative warnings into errors, I think this PR is well put together, but I think a simpler alternative to this would be to forbid the double-negative syntax entirely.

To take an example, I think we should only support negation syntax like -noupnp and -noupnp=1, and that we should start treating -noupnp=0, -noupnp=23, and -noupnp=jerry as errors for any option, regardless of whether it is boolean or not. The double negative syntax is confusing and doesn’t have any uses that make sense.

Forbidding double-negatives across the board rather than only forbidding them selectively would be a lot easier to explain in release notes. (I disagree with the “no release notes are required” in the PR description because the PR forbids configurations that were previously accepted, so users should have a heads-up about that.)

Forbidding double-negatives uniformly instead of selectively would also be a much simpler change than this. It should basically be a one line change turning the current warning into an error.

ryanofsky commented at 9:20 pm on July 31, 2019: member

Also I think it would be good to describe PRs that change behavior in terms of the behavior, instead of the implementation details. For example, I think a better title for this PR than “Use ArgsManager::ALLOW_BOOL flag explicitly” would be “Disallow double negation syntax -nofoo=0 for non-boolean options”

DrahtBot commented at 9:26 pm on July 31, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16545 (Implement missing error checking for ArgsManager flags by ryanofsky)
#16115 (On bitcoind startup, write config args to debug.log by LarryRuane)
#15934 (Separate settings merging from parsing by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake removed the label GUI on Jul 31, 2019

fanquake removed the label Tests on Jul 31, 2019

fanquake removed the label Validation on Jul 31, 2019

fanquake removed the label Wallet on Jul 31, 2019

hebasto commented at 7:23 am on August 1, 2019: member

@ryanofsky Thank you for your review.

Also I think it would be good to describe PRs that change behavior in terms of the behavior, instead of the implementation details. For example, I think a better title for this PR than “Use ArgsManager::ALLOW_BOOL flag explicitly” would be “Disallow double negation syntax -nofoo=0 for non-boolean options”

From my point of view, this PR is a milestone on the road which begins with #16097. The end goal is to replace all ArgsManager::ALLOW_ANY flags with proper type flag(s). IMO, disabling double negation syntax for non-boolean args is a pretty bonus feature in this PR.

Forbidding double-negatives uniformly instead of selectively would also be a much simpler change than this. It should basically be a one line change turning the current warning into an error.

I cannot recall any related discussion. Should this topic be raised at the next IRC meeting?

I disagree with the “no release notes are required” in the PR description because the PR forbids configurations that were previously accepted, so users should have a heads-up about that.

You are right. Going to add release notes soon.

ryanofsky commented at 12:38 pm on August 1, 2019: member

From my point of view, this PR is a milestone on the road which begins with #16097. The end goal is to replace all ArgsManager::ALLOW_ANY flags with proper type flag(s). IMO, disabling double negation syntax for non-boolean args is a pretty bonus feature in this PR. @hebasto, I agree with your goal, but think this is a bad approach. I want to get to the same place as you, but I don’t want to do it by repeatedly tweaking behavior in complicated ways that no reviewer or no user will be able to keep track of. I want to change behavior in straightforward PR’s that are understandable by normal reviewers and understandable by users who are affected by these changes.

Internal code cleanups should happen separately from behavior changes for two reasons:

It makes PR vastly easier to review. When I’m reviewing a refactoring PR, all I have to ask myself is two questions. Do the changes affect behavior, and do these changes improve the code? When you mix in behavior changes, there’s a combinatoric explosion of other questions that get added. Is this the part of the change that changes behavior or is this other part responsible? Is this behavior change good? Is the behavior change limited to what’s described or does it have other side effects? Is it good but will it break existing use cases? Is it good but could it be implemented in a simpler way? Is the behavior a stable endpoint, or does it put us in some weird intermediate state that’s going to be changed again later in a confusing way?
It ensures the reviews will actually be meaningful. People who are interested in a fix or feature will be able to understand and comment on the behavior changes without getting lost in sea of refactoring. People who are familiar and interested in the particular area of code can discuss structure and maintenance and get PRs merged more quickly because they have limited scope.

For this PR, if you aren’t actually interested making the described behavior change, I think you should just close this and move test / refactoring parts to #16097, and save changing flags for some future PR that implement behavior changes which you actually believe are beneficial to users. If you are interested in this behavior change, and think it benefits users, I would suggest forbidding double negatives in the simpler (maybe one-line) change I suggested previously.

hebasto force-pushed on Aug 1, 2019

hebasto renamed this:
~~Use ArgsManager::ALLOW_BOOL flag explicitly~~
Disallow double negation syntax for options
on Aug 1, 2019

hebasto commented at 5:18 pm on August 1, 2019: member

@ryanofsky

Forbidding double-negatives uniformly instead of selectively would also be a much simpler change than this. It should basically be a one line change turning the current warning into an error.

Done.

Also I think it would be good to describe PRs that change behavior in terms of the behavior, instead of the implementation details.

Done.

Also release notes have been added.

hebasto renamed this:
~~Disallow double negation syntax for options~~
Disallow double negation syntax (-nofoo=0) for options
on Aug 1, 2019

Disallow double negation syntax for options ecf8730f89

test: Add tests for double negated options 7fb01de3cf

doc: Add release notes for 16508 feb0fa6ebc

hebasto force-pushed on Aug 2, 2019

hebasto commented at 6:23 pm on August 2, 2019: member

Rebased on top of #16097.

laanwj commented at 1:48 pm on August 5, 2019: member

Concept ~0 on this. I don’t think double negatives are useful, but also, I don’t see a strong reason to break compatibility here just for sake of aesthetics.

hebasto commented at 4:43 pm on August 5, 2019: member

Closed in favor of #16545.

hebasto closed this on Aug 5, 2019

hebasto deleted the branch on Jun 13, 2020

DrahtBot locked this on Feb 15, 2022

Disallow double negation syntax (-nofoo=0) for options #16508

Conflicts