Track negated options in the option parser #12713

eklitzke · 2018-03-18T02:43:34Z

This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a -no prefix. For example, -nofoo is the negated form of -foo. Negated options were originally added in the 0.6 release.

The change here allows code to explicitly distinguish between cases like -nofoo and -foo=0, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before.

The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the -debuglogfile option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in -nodebuglogfile and the code can see that it was explicitly negated, and use that to disable the log file.

This change originally split out from #12689.

meshcollider · 2018-03-18T10:08:46Z

Looks good, thanks for splitting it out. utACK 6ad2a13

maflcko · 2018-03-18T10:39:01Z

utACK 6ad2a13cfdd46899c28f21899e107ee7d4c07877 (Also noting that -a=no sets a to true, now)

eklitzke · 2018-03-18T19:28:15Z

Yeah I didn't want to add support for strings like "no" "disabled" etc. because I don't think there was ever an intention to support non-integer parameters in the first place. If we wanted to support that it's easy enough. You can also use -noa, both now and before.

donaloconnor · 2018-03-18T19:33:36Z

utACK 6ad2a13

jimpo · 2018-03-18T22:24:24Z

ACK 6ad2a13cfdd46899c28f21899e107ee7d4c07877.

promag · 2018-03-18T22:51:02Z

src/util.cpp

Why not throw an error if it's a stupid value? IMHO, for instance, -txindex=foo should fail.

Thinking more about this, with proper argument validation we might as well reject false as a value for a boolean argument. This was never supported, so it feels weird to special case it now.

@promag Two reasons. First, such a change would not be backwards compatible (a minor point, see below). A better reason is that it enables the -printtoconsole change to have a way of disabling the debug log file without actually adding new command line arguments. In other words, I first test if -debuglogfile boolean parses as "false" (i.e. -nodebuglogfile or -debuglogfile=0), and if it's not false I re-parse it as a string. There are already a lot of possible flags to give Bitcoin, and if we can re-use existing functionality without adding new flags that seems better to me.

@MarcoFalke I actually agree, as accepting false opens the door for disabled, untrue, nil, void, zip, null, non, nein, ad infinitum.

My preferred semantics would be: -nofoo sets foo to false. Also -foo=0 sets foo to false. Which is exactly what the intention of the existing code seems to be. I only handled the false flag as there is existing code committed in the Bitcoin repository that relies on it.

I started this endeavor just wanting to get bitcoind printing to stdout when run in interactive mode. Whatever semantics allow that to happen are fine with me.

I first test if -debuglogfile boolean parses as "false" (i.e. -nodebuglogfile or -debuglogfile=0), and if it's not false I re-parse it as a string.

Feel free to discount this opinion, but if the option is -debuglogfile=<filename>, I would hope that -debuglogfile=0 or -debuglogfile=false would just create log debug log files 0 or false respectively, instead of disabling logging.

Like João and Marco, I would also want ambiguous values passed as boolean options to trigger errors.

I guess I'm not so much a fan of option parsing trying to be very clever.

+1 to what @ryanofsky says; if backwards-compatability is not a constraint, doing the least clever thing seems most consistent and likely to lead to the most maintainable code, and long term least confusion for users.

(This PR seems like an improvement in readability of the argument parsing code, so any further simplification might be for another changeset.)

maflcko · 2018-03-19T16:02:26Z

~~Added "refactoring" label, since it doesn't change behavior~~

utACK e944ec998fbd232b05ec9471ae6f19d1d32e0719

Would you mind updating the OP, which ends up in the merge commit?

jnewbery · 2018-03-19T21:09:32Z

since it doesn't change behavior

This is a change in behaviour. If I take the changes to the unit test, but not the changes to util.cpp, the unit tests fail. If I take the changes to util.cpp, but not the change to wallet_resendwallettransactions.py, then wallet_resendwallettransactions.py fails.

EDIT: the reason for this is that atoi("false") will return 0, so -walletbroadcast=false is equivalent to -walletbroadcast=0`

atoi reference: http://www.cplusplus.com/reference/cstdlib/atoi/ . The first non-whitespace character in false is not a digit, so atoi("false') = 0

eklitzke · 2018-03-20T07:55:28Z

These are all easy to implement. I can make -debuglogfile=0 log to a file named 0, I can make it not log at all, I can special case -nodebuglogfile and have that not create a file, I can parse false as false or parse it as true, I can make a backwards compatible change and I can make a backwards incompatible change.

I'm not really sure what to do, as this has gotten an unusual amount of debate for something that is frankly quite banal. I got three different suggestions for how to disable the flag in the original PR:

-nodebuglogfile
-debuglogfile=0
-debuglogfile=/dev/null

If there is not consensus on whether we should change this I will just drop this change altogether and add a brand new flag in the original PR (-disablelogfile).

ryanofsky · 2018-03-20T09:40:48Z

I'm not really sure what to do, as this has gotten an unusual amount of debate for something that is frankly quite banal. I got three different suggestions for how to disable the flag in the original PR:

From my reading of the feedback, I think if you go with -nodebuglogfile to disable logging, -debuglogfile=<file> to enable logging to a custom location, and -debuglogfile and -debuglogfile= to enable logging to the default location, you will have no objections. The ambivalence just came from zip/nein/0 stuff, which can be avoided by treating all nonempty string values the same.

hkjn · 2018-03-20T11:27:26Z

ACK e944ec9.

This behavior as well as code seems slightly simpler than before, with more
tests.

I'll note we accept some pretty wacky combinations even after this change though,
that we may want to make errors in the future, at least for boolean options:

$ bitcoind -printtoconsole=0       # false
$ bitcoind -noprinttoconsole       # false
$ bitcoind -noprinttoconsole=no    # false
$ bitcoind -noprinttoconsole=false # false
$ bitcoind -noprinttoconsole=1     # false
$ bitcoind -printtoconsole=1       # true
$ bitcoind -printtoconsole=no      # true
$ bitcoind -printtoconsole=false   # true
$ bitcoind -noprinttoconsole=0     # true

eklitzke · 2018-03-20T17:29:46Z

@ryanofsky I will put up another version of this diff with that change.

eklitzke · 2018-03-20T19:23:56Z

Handle arguments like -nofoo more explicitly.

This change keeps track of whether an option was passed with the -no semantics. This way in #12689 I can do something like:

if (gArgs.IsArgInverted("-debuglogfile")) {
   ....
}

to see if the debug log file should be disabled.

I also removed some gratuitous boost and string copies.

@jnewbery @MarcoFalke You guys figure out how you want the -foo=false case to be handled and let me know what you decide on.

eklitzke · 2018-03-21T05:41:54Z

Updated for more consistent naming -- instead of "negated" and "inverted" I now always use the negated terminology.

maflcko

re-utACK 6b95d6b (Changes were to get rid of some boost usage and expose IsArgNegated, which I slightly object to)

maflcko · 2018-03-21T12:31:19Z

src/util.h

+     * @param strArg Argument to get (e.g. "-foo")
+     * @return true if the argument was passed negated
+     */
+    bool IsArgNegated(const std::string& strArg) const;


I don't think we should expose this in the code base. Historically, -nofoo always implied -foo=0, so use cases within the code base should always compare with the string "0" to check if foo was unset.

Yes, I know that that would forbid use cases, such as -myfile=0, but imo that is acceptable and desirable to keep previous behavior.

Edit: This is bike shedding, so feel free to ignore.

Historically, -nofoo always implied -foo=0, so use cases within the code base should always compare with the string "0" to check if foo was unset.

This is true for boolean options, and should still be true with this PR. I don't think it was ever historically true that specifying "0" or "false" for path type options would disable those options. At least this isn't the case currently for -conf -debuglogfile -pid -rpccookiefile -wallet or other path options. So whether or not special path strings are a good idea (I think they are a bad idea), I don't think there is a historic argument for having them.

I wanted to raise the point that we specifically check for the string "0" in some non-bool arguments to see if they are disabled. E.g. -connect=0 or -debug=0. So in case someone were to use IsArgNegated for those, it would break it.

Again, only bike-shedding here, so please ignore.

I agree with @ryanofsky that not being able to create a log file named "0" (or "false") is weird. Tracking negation makes the arguments more strongly typed in a sense. I also felt like it made my printtoconsole change a little easier to understand, because it removes the code that calls GetBoolArg() on a non-boolean field.

jnewbery · 2018-03-21T15:27:16Z

Concept ACK the behaviour change. Making only the exact string "0" evaluate to false seems like better behaviour to me.

Tested ACK the negated argument change.

ACK the variable name changes.

Tested ACK the WIN32 boost::algorithm -> std::transform change on linux (by commenting out the #ifdef WIN32 in ArgsManager::ParseParameters() and IsSwitchChar()).

This needs release notes since it's a subtle change to the way that arguments are parsed.

ryanofsky

utACK 6b95d6b.

It seems to me though that this is doing two different things that don't need to be tied to together:

Introducing a nice IsArgNegated API that provides a consistent way of disabling non-bool options, especially features that read or write to files.
Interpreting -foo=bar and similar strings as false instead of true.

Second change seems fine, but IMO would probably be better off in a separate PR. It could also be tweaked to accept more false strings, and should definitely be documented.

ryanofsky · 2018-03-21T15:38:19Z

src/util.cpp

-    if (strValue.empty())
-        return true;
-    return (atoi(strValue) != 0);
+    return val != "0";


I think would be a little better to not change InterpretBool here. This seems making a backwards incompatible change in a PR that would otherwise be backwards compatible. If there is actually a reason to make a change here, it would be good to document it. I think the following table might summarize:

Argument Previous Current

-foo true true

-foo= true true

-foo=1 true true

-foo=0 false false

-foo=-0 false true

-foo=+0 false true

-foo=00 false true

-foo=bar false true

-foo=0bar false true

-foo=1bar true true

Also, if you want to change this function here, I don't think there is any downside to treating strings like false and no as false, since GetBoolArg() should only be called on bool options, not path options.

ryanofsky · 2018-03-21T15:53:43Z

src/util.cpp

+        }
+        key.erase(1, 2);
+        m_negated_args.insert(key);
+        val = bool_val ? "0" : "1";


Instead of munging the string value, it seems like it would be slightly better just to avoid inserting new entries into the arg maps when bool_val is true. But this would require updating Get*Arg functions to check the m_negated_args map in addition to the normal maps, so I understand why you didn't do this here. And it's pretty much an internal implementation detail, so it could be saved for a future improvement.

ryanofsky · 2018-03-21T16:24:48Z

src/test/util_tests.cpp

    BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
 }

+BOOST_AUTO_TEST_CASE(util_GetBoolArg)


Great tests!

hkjn · 2018-03-21T19:52:15Z

Re-ACK 6b95d6b.

+1 on documenting the change in behavior for flags in release notes.

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064.

… options e9a78e9 doc: Change documentation for =0 for non-boolean options (Wladimir J. van der Laan) Pull request description: PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to `0`, but to remove it from the options. I think this is better because it gets rid of the special meaning of `'0'`. However it needs to be documented. I attempt to do so in this PR. Addresses bitcoin#14064. There might be options I missed, please help check: - `-connect` - `-proxy` - `-onion` - `-debug` - `-debuglogfile` Needs a manpage update. Tree-SHA512: a69e63e04a6c8bf6f61a58ddc0f5ebd4b9af7a2e0ea5174e668f2e3edd31a541910a125605ca4bfccf3aca6e59267d98a66de9d1e73650f48c8a1828d315d35d

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064. Github-Pull: bitcoin#14100 Rebased-From: e9a78e9

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064.

f7683cb Track negated arguments in the argument paser. (Evan Klitzke) 4f872b2 Add additional tests for GetBoolArg() (Evan Klitzke) Pull request description: This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a `-no` prefix. For example, `-nofoo` is the negated form of `-foo`. Negated options were originally added in the 0.6 release. The change here allows code to explicitly distinguish between cases like `-nofoo` and `-foo=0`, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before. The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the `-debuglogfile` option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in `-nodebuglogfile` and the code can see that it was explicitly negated, and use that to disable the log file. This change originally split out from bitcoin#12689. Tree-SHA512: cd5a7354eb03d2d402863c7b69e512cad382781d9b8f18c1ab104fc46d45a712530818d665203082da39572c8a42313c5be09306dc2a7227cdedb20ef7314823

4709ff8 remove now unused gArgs wrappers (Fuzzbawls) cc40f8d Manual prep for gArg wrapper removal (Fuzzbawls) b3c843c scripted-diff: stop using the gArgs wrappers (Fuzzbawls) 5ce3844 Migrate g_logger fields to gArgs (Fuzzbawls) 72a6f56 Track negated arguments in the argument paser. (Fuzzbawls) 3142e1b Add additional tests for GetBoolArg() (Fuzzbawls) 4aef2f3 Fix constness of ArgsManager methods (Fuzzbawls) 13fd27d Fix indentation after 'Remove redundant calls to gArgs.IsArgSet()' (Fuzzbawls) a4114f3 Util: Remove redundant calls to gArgs.IsArgSet() (Fuzzbawls) 92e7227 Util: Small improvements in gArgs usage (Fuzzbawls) a8b26e7 Util: Put mapMultiArgs inside ArgsManager (Fuzzbawls) d2f2e25 scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs (Fuzzbawls) b2abc59 Util: Create ArgsManager class... (Fuzzbawls) 8c4d9c5 Add a ForceSetArg method for testing (Fuzzbawls) 59d6529 Lock mapArgs/mapMultiArgs access in util (Fuzzbawls) f7f2689 Un-expose mapArgs from util.h (Fuzzbawls) 7894b07 Get rid of mapArgs direct access in ZMQ construction (Matt Corallo) b28789f Introduce (and use) an IsArgSet accessor method (Fuzzbawls) 4d8ecf9 Fix non-const mapMultiArgs[] access after init. (Fuzzbawls) 391388d Remove arguments to ReadConfigFile (Fuzzbawls) Pull request description: This is a cumulation of several upstream backports surrounding the argument parsing code. Included here are the following upstream PRs: - bitcoin#9243 - bitcoin#9494 - bitcoin#10118 - bitcoin#10901 - bitcoin#12713 - bitcoin#10607 I've also refactored the global logging argument parsing to match, as that was an "out-of-order" PR that was previously backported to match our previous state. ACKs for top commit: random-zebra: utACK 4709ff8 furszy: cool encapsulation, utACK 4709ff8 and merging Tree-SHA512: 74c8dc1b2280b726eff44855e9908f3563d4ef6493f296ebfafe240ac6470bc45cbc75baac614ed59dfda9e1377b7da7d47a695dfc05e364cada2468bb99a258

f7683cb Track negated arguments in the argument paser. (Evan Klitzke) 4f872b2 Add additional tests for GetBoolArg() (Evan Klitzke) Pull request description: This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a `-no` prefix. For example, `-nofoo` is the negated form of `-foo`. Negated options were originally added in the 0.6 release. The change here allows code to explicitly distinguish between cases like `-nofoo` and `-foo=0`, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before. The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the `-debuglogfile` option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in `-nodebuglogfile` and the code can see that it was explicitly negated, and use that to disable the log file. This change originally split out from bitcoin#12689. Tree-SHA512: cd5a7354eb03d2d402863c7b69e512cad382781d9b8f18c1ab104fc46d45a712530818d665203082da39572c8a42313c5be09306dc2a7227cdedb20ef7314823

… options e9a78e9 doc: Change documentation for =0 for non-boolean options (Wladimir J. van der Laan) Pull request description: PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to `0`, but to remove it from the options. I think this is better because it gets rid of the special meaning of `'0'`. However it needs to be documented. I attempt to do so in this PR. Addresses bitcoin#14064. There might be options I missed, please help check: - `-connect` - `-proxy` - `-onion` - `-debug` - `-debuglogfile` Needs a manpage update. Tree-SHA512: a69e63e04a6c8bf6f61a58ddc0f5ebd4b9af7a2e0ea5174e668f2e3edd31a541910a125605ca4bfccf3aca6e59267d98a66de9d1e73650f48c8a1828d315d35d # Conflicts: # src/init.cpp

fanquake added the Utils/log/libs label Mar 18, 2018

promag reviewed Mar 18, 2018

View reviewed changes

jimpo mentioned this pull request Mar 18, 2018

Smarter default behavior for -printtoconsole #12689

Closed

eklitzke force-pushed the bool-option-parsing branch from 6ad2a13 to e944ec9 Compare March 19, 2018 13:34

maflcko added the Refactoring label Mar 19, 2018

maflcko removed the Refactoring label Mar 19, 2018

eklitzke force-pushed the bool-option-parsing branch from e944ec9 to b6597eb Compare March 20, 2018 19:19

eklitzke force-pushed the bool-option-parsing branch 2 times, most recently from ff3f2c3 to 8ef4c69 Compare March 21, 2018 05:41

eklitzke changed the title ~~Fix unintuitive boolean option parsing behavior~~ Track negated options better Mar 21, 2018

eklitzke mentioned this pull request Mar 21, 2018

WIP: eliminate dependency on boost::program_options #12744

Closed

eklitzke force-pushed the bool-option-parsing branch from 8ef4c69 to 6b95d6b Compare March 21, 2018 07:05

maflcko reviewed Mar 21, 2018

View reviewed changes

ryanofsky reviewed Mar 21, 2018

View reviewed changes

AkioNak mentioned this pull request Aug 27, 2018

-debuglogfile=0 does not disable logging to a file #14064

Closed

laanwj mentioned this pull request Aug 29, 2018

doc: Change documentation for =0 for non-boolean options #14100

Merged

HashUnlimited mentioned this pull request Sep 14, 2018

doc: Change documentation for =0 for non-boolean options chaincoin/chaincoin#667

Merged

UdjinM6 mentioned this pull request Apr 29, 2020

Backport loadwallet dashpay/dash#3462

Merged

Fuzzbawls mentioned this pull request Oct 23, 2020

[Core] Arg parsing overhaul PIVX-Project/PIVX#1938

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Argument	Previous	Current
`-foo`	true	true
`-foo=`	true	true
`-foo=1`	true	true
`-foo=0`	false	false
`-foo=-0`	false	true
`-foo=+0`	false	true
`-foo=00`	false	true
`-foo=bar`	false	true
`-foo=0bar`	false	true
`-foo=1bar`	true	true

Track negated options in the option parser #12713

Track negated options in the option parser #12713

Uh oh!

Conversation

eklitzke commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Mar 18, 2018

Uh oh!

maflcko commented Mar 18, 2018

Uh oh!

eklitzke commented Mar 18, 2018

Uh oh!

donaloconnor commented Mar 18, 2018

Uh oh!

jimpo commented Mar 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

ryanofsky commented Mar 20, 2018

Uh oh!

hkjn commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke commented Mar 21, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 21, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkjn commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

eklitzke commented Mar 18, 2018 •

edited

Loading

maflcko commented Mar 19, 2018 •

edited

Loading

jnewbery commented Mar 19, 2018 •

edited

Loading

eklitzke commented Mar 20, 2018 •

edited

Loading

maflcko Mar 21, 2018 •

edited

Loading

eklitzke Mar 22, 2018 •

edited

Loading

ryanofsky Mar 21, 2018 •

edited

Loading