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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
utACK
nit: Commit summary is too long. Would also be nice to rebase on top of 8afa602f308ef003bb6893718eae1fe5a830690c
@ryanofsky want to take a look here?
I think the fix in bfc2bb6a270d605b4931699cbb46be9ffbacdd9e 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.)
Are you still working on this?
tACK bfc2bb6a270d605b4931699cbb46be9ffbacdd9e
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.
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.
@Brotcrunsher, I could pick this PR up and add you as a co-author if you are not able to work on it.