Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied. However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.
See #30529 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31767.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, ryanofsky, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
0@@ -0,0 +1,3 @@
1+RPC
-debug is a setting related to logging.
93+ auto last_negated = categories.rbegin();
94+ for (; last_negated != categories.rend(); ++last_negated) {
95+ if (*last_negated == "0" || *last_negated == "none") {
96+ break;
97+ }
98+ }
nit: I tried to replicate what ChatGPT suggested to ryanofsky, but std::views wouldn’t compile (didn’t look into modifying my setup). Was able to combine it with your version though into:
0 auto last_negated = std::find_if(categories.rbegin(), categories.rend(),
1 [](const std::string& cat) { return cat == "0" || cat == "none"; });
99+
100+ auto range_to_process = (last_negated == categories.rend()) ? categories : std::ranges::subrange(last_negated.base(), categories.end());
101+
102+ for (const auto& cat : range_to_process) {
103+ if (!LogInstance().EnableCategory(cat)) {
104+ return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)};
mega-nit: I find this marginally nicer, but still not great:
0 for (auto it = last_negated == categories.rend() ? categories.begin() : last_negated.base();
1 it != categories.end(); ++it) {
2 if (!LogInstance().EnableCategory(*it)) {
3 return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", *it)};
98@@ -99,6 +99,18 @@ def run_test(self):
99 match=ErrorMatch.PARTIAL_REGEX,
100 )
101
102+ self.log.info("Test that -nodebug,-debug=0,-debug=none clear previously specified debug options")
103+ disable_debug_options = [
104+ '-debug=0',
105+ '-debug=none',
106+ '-nodebug'
-nodebug to prove existing behavior, before the commit performing the C++ change and adding 0 + none tests (it passed). You might want to rewrite the commit history to do that as well, but I’m okay with the current approach.
107+ ]
108+
109+ for debug_opt in disable_debug_options:
110+ self.restart_node(0, ['-debug=http', debug_opt, '-debug=rpc'])
111+ logging = self.nodes[0].logging()
112+ assert logging['rpc']
nit: Could add invalid abc category prior to disabling, and verify that multiple categories after the disabling work.
0 self.restart_node(0, ['-debug=http', '-debug=abc', debug_opt, '-debug=rpc', '-debug=net'])
1 logging = self.nodes[0].logging()
2 assert logging['rpc']
3 assert logging['net']
ACK 1f4aff75c308226d58a45460982f75287e15cb81
Having -debug=0 and -debug=none perform the same effect as -nodebug in discarding all preceding categories makes more sense, instead of disabling all categories including the ones following them.
As well as running the new test as-is, I also verified stated behavior of -nodebug prior to the C++ change by modifying included test.
Only found nits, the last_negated calculation in particular feels unnecessarily verbose & mutation heavy.
Thank you for the reviews! In my last push:
96+ for (; last_negated != categories.rend(); ++last_negated) {
97+ if (*last_negated == "0" || *last_negated == "none") {
98+ break;
99+ }
100+ }
101+
Seems like you forgot to remove this? (last_negated should theoretically be able to be marked const, but makes line go over 80 chars).
last_negated and categories_to_process const
Previously, -nodebug cleared all prior -debug configurations in the
command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging,
even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none
behave like -nodebug: they now clear previously set debug configurations
but do not disable debugging for categories specified later.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
re-ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1
Changes since previous ACK:
79@@ -80,15 +80,17 @@ util::Result<void> SetLoggingLevel(const ArgsManager& args)
80 util::Result<void> SetLoggingCategories(const ArgsManager& args)
81 {
82 if (args.IsArgSet("-debug")) {
Nit in a8fedb36a71ac193fc158284b14d4eb474e5ab62: Why is this needed? It seems like a no-op:
-nodebug, IsArgSet will be true, but GetArgs will be empty.So my recommendation would be to remove it. If you want, you can keep the {} for the scope.
re: #31767 (review)
Nit in a8fedb3: Why is this needed? It seems like a no-op:
Right it is is a no-op, see #30529 which makes the change you suggested and removes many other misuses of IsArgSet
87- [](std::string cat){return cat == "0" || cat == "none";})) {
88- for (const auto& cat : categories) {
89- if (!LogInstance().EnableCategory(cat)) {
90- return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)};
91- }
92+ // Special-case: Disregard any debugging categories appearing before -debug=0/none
// GetArgs disregards any debugging categories appearing before -nodebug, so do the same for -debug=0/none
Nit in a8fedb3: Would be nice to explain why this is done: Something like:
// GetArgs disregards any debugging categories appearing before -nodebug, so do the same for -debug=0/none
Could be good to add a reason for this, but the suggested reason is pretty obscure one. I think the general reason for doing this is that later arguments are supposed to take precedence over earlier ones, so when -debug=none is followed by -debug=something, -debug=something should take precedence, and vice versa.
left two nits
review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1 👡
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1 👡
3ArIz38cfPYwJHMq9d1QKT9t9zk+QWETW8C/1e1B83bsin8cm2QVQRHActkHAgZSX49gcc2TtDyMFdPVuDcWZDw==