Blocking arguments -nohelp, -noh, and -no?

[Brotcrunsher]

The three arguments -nohelp, -noh, and -no? were previously silently accepted and interpreted as -help, -h, and -? respectively. As negating these arguments is meaningless, this is now blocked and properly communicated to the user, resulting in e.g.:

Error parsing command line arguments: Negating of -help is meaningless and therefore forbidden

Not that anyone ever complained about this. I just noticed this oddity while looking through the code.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[luke-jr]

utACK

[luke-jr]

nit: Commit summary is too long. Would also be nice to rebase on top of 8afa602

[fanquake]

@ryanofsky want to take a look here?

[ryanofsky]

I think the fix in bfc2bb6 in an improvement over status quo, but is not ideal from a user perspective and also adds unnecessary code complexity. I would suggest a one-line fix instead:

--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -659,7 +659,7 @@ std::string ArgsManager::GetHelpMessage() const
 
 bool HelpRequested(const ArgsManager& args)
 {
-    return args.IsArgSet("-?") || args.IsArgSet("-h") || args.IsArgSet("-help") || args.IsArgSet("-help-debug");
+    return args.GetBoolArg("-?", false) || args.GetBoolArg("-h", false) || args.GetBoolArg("-help", false) || args.GetBoolArg("-help-debug", false);
 }
 
 void SetupHelpOptions(ArgsManager& args)

We should avoid randomly cherry-picking which options support negation and which don't. Better to just support negation consistently. There are few options which really can't accept missing values and where DISALLOW_NEGATION makes sense, but this isn't common. (In general, I'd say IsArgSet function and DISALLOW_NEGATION flags are heavily misused and it's usually good to steer away from using them.)

[DrahtBot]

Are you still working on this?

[pablomartin4btc]

tACK bfc2bb6

Good catch, I haven't noticed it.

Please consider following @luke-jr recommendation on commit subject length, mind the guidelines regarding commits.

Tested it in bitcoin-cli, bitcoind and bitcoin-qt.

I found that bitcoin-cli and bitcoin-qt don't support -help-debug (Error parsing command line arguments: Invalid parameter -help-debug ), not sure if this is something perhaps that needs to be added at somepoint in a follow-up, but as @ryanofsky pointed out in the proposed code change, it should be included in the negation check function.

While at first sight I may prefer the original code DISALLOW_NEGATION for its clarity one could say, considering the long-term maintainability of the codebase, if you anticipate more boolean options being added in the future, using args.GetBoolArg() consistently can lead to cleaner and less error-prone code, so I do agree with @ryanofsky suggestion.

[maflcko]

Closing for now, due to lack of activity. Let us know if you want to pick this up again, and if this should be reopened.

[pablomartin4btc]

@Brotcrunsher, I could pick this PR up and add you as a co-author if you are not able to work on it.