Bugfix: RPC: Remove quotes from non-string oneline descriptions #28123
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
luke-jr
force-pushed
the
fix_nonstring_onelinedesc
branch
from
889b504 to
5e3e83b
Compare
maflcko
approved these changes
Jul 22, 2023
Comment on lines
+1123
to
+1126
| strprintf("Internal bug detected: non-string RPC arg \"%s\" quotes oneline_description:\n%s\n%s %s\nPlease report this issue here: %s\n", | ||
| m_names, m_opts.oneline_description, | ||
| PACKAGE_NAME, FormatFullVersion(), | ||
| PACKAGE_BUGREPORT)}; |
Member
There was a problem hiding this comment.
nit: Could use STR_INTERNAL_BUG?
Suggested change
| strprintf("Internal bug detected: non-string RPC arg \"%s\" quotes oneline_description:\n%s\n%s %s\nPlease report this issue here: %s\n", | |
| m_names, m_opts.oneline_description, | |
| PACKAGE_NAME, FormatFullVersion(), | |
| PACKAGE_BUGREPORT)}; | |
| STR_INTERNAL_BUG(strprintf("non-string RPC arg \"%s\" quotes oneline_description:\n%s", | |
| m_names, m_opts.oneline_description) | |
| )}; |
Contributor
There was a problem hiding this comment.
The above suggestion doesn't compile without syntax changes due to error: unterminated argument list invoking macro "STR_INTERNAL_BUG" This is because of a missing ) to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after m_opts.oneline_description. Once these are removed code does compile but the output is not as clean as the current commit due to an extra " on a new line after the name of the oneline_description compared to the current version which doesn't have this artifact.
Member
There was a problem hiding this comment.
I've modified my suggestion. (Sorry, I don't compile them)
Contributor
There was a problem hiding this comment.
No problem just was testing things. The new version compiles and looks better without the newline but it looks like a quote is appended to the m_opts.oneline_description arg.
The current PR displays correctly "options"
Member
There was a problem hiding this comment.
It should be fine to just drop the ", as they serve no purpose. A newline already follows the message to delimit it from the next lines.
diff --git a/src/util/check.cpp b/src/util/check.cpp
index 795dce7124..c4d4b0cc28 100644
--- a/src/util/check.cpp
+++ b/src/util/check.cpp
@@ -18,7 +18,7 @@
std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func)
{
- return strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\n"
+ return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
"%s %s\n"
"Please report this issue here: %s\n",
msg, file, line, func, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT);
Contributor
There was a problem hiding this comment.
This compiles and looks great even for the existing errors! Modifying this does cause rpc_misc.py to fail test runner. Overall quite nice.
Member
There was a problem hiding this comment.
@russeree Do you want to create a follow-up pull with the changes?
This was referenced Jul 22, 2023
Contributor
|
tACK
|
portlandhodl
approved these changes
Jul 23, 2023
portlandhodl
approved these changes
Jul 23, 2023
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 17, 2023
…ne descriptions 5e3e83b RPC/Mining: Document template_request better for getblocktemplate (Luke Dashjr) de319c6 RPC/rpcdoccheck: Error if a oneline_description has a quote for a non-string (Luke Dashjr) 7c61e9d Bugfix: RPC: Remove quotes from non-string oneline descriptions (Luke Dashjr) Pull request description: Various JSON Object parameters had a `oneline_description` with quote characters. Fix those, and extend `rpcdoccheck` to detect them. Also, slightly improve GBT's oneline description for template_request. ACKs for top commit: MarcoFalke: review ACK 5e3e83b Tree-SHA512: 363d1669a661d0acfc19fddb57e777d781c7246f330cf62160e77dde10a6adcb0249db748127067da1afe1b7d17c71cf611d9fdc3664d6bf5b3f30105637769a
mzumsande
added a commit
to mzumsande/bitcoin
that referenced
this pull request
Aug 17, 2023
This fixes a silent conflict betwen bitcoin#28123 and bitcoin#27460
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Aug 18, 2023
…ng oneline description 2394314 rpc: remove one more quote from non-string oneline description (Martin Zumsande) Pull request description: This fixes a silent conflict between bitcoin/bitcoin#28123 (which removed all `\"options\"`) and bitcoin/bitcoin#27460 (which added a new one). It should fix the current CI failures. ACKs for top commit: ajtowns: utACK 2394314 MarcoFalke: lgtm ACK 2394314 jonatack: ACK 2394314 hebasto: ACK 2394314 Tree-SHA512: feb0c2b936a77be45d9c65aa7d738277b2266b5153665fee3b1413045de521195dc7d5efa2fc8b37b22f16e9b8d0ee8de25bfd151a428666122b31f64056557a
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Sep 5, 2023
…delimitation 6e8f646 removed StrFormatInternalBug quote delimitation (Reese Russell) Pull request description: This PR rectifies an unnecessary set of quotes delimiting the contents of ```StrFormatInternalBug```. This is a follow up to MarcoFalke bitcoin/bitcoin#28123 (comment). The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format. ```STR_INTERNAL_BUG``` was applied to bitcoin/bitcoin#28123 in ```std::string RPCArg::ToString(const bool oneline)``` in ```rpc/util.cpp``` The results can be seen below. Previously  This PR  Additional context can be found here. bitcoin/bitcoin#28123 (comment) Thank you. ACKs for top commit: MarcoFalke: review ACK 6e8f646 stickies-v: ACK 6e8f646 Tree-SHA512: 35317e31a527630495b566407e37db9941dab7f81cfaeb1ea3309683c48e4273284645ad615f73e646a137b4f2ae35933603e9182a7dbdd22cac98d038c491dc
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 8, 2023
…ne description 2394314 rpc: remove one more quote from non-string oneline description (Martin Zumsande) Pull request description: This fixes a silent conflict between bitcoin#28123 (which removed all `\"options\"`) and bitcoin#27460 (which added a new one). It should fix the current CI failures. ACKs for top commit: ajtowns: utACK 2394314 MarcoFalke: lgtm ACK 2394314 jonatack: ACK 2394314 hebasto: ACK 2394314 Tree-SHA512: feb0c2b936a77be45d9c65aa7d738277b2266b5153665fee3b1413045de521195dc7d5efa2fc8b37b22f16e9b8d0ee8de25bfd151a428666122b31f64056557a
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 8, 2023
…tion 6e8f646 removed StrFormatInternalBug quote delimitation (Reese Russell) Pull request description: This PR rectifies an unnecessary set of quotes delimiting the contents of ```StrFormatInternalBug```. This is a follow up to MarcoFalke bitcoin#28123 (comment). The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format. ```STR_INTERNAL_BUG``` was applied to bitcoin#28123 in ```std::string RPCArg::ToString(const bool oneline)``` in ```rpc/util.cpp``` The results can be seen below. Previously  This PR  Additional context can be found here. bitcoin#28123 (comment) Thank you. ACKs for top commit: MarcoFalke: review ACK 6e8f646 stickies-v: ACK 6e8f646 Tree-SHA512: 35317e31a527630495b566407e37db9941dab7f81cfaeb1ea3309683c48e4273284645ad615f73e646a137b4f2ae35933603e9182a7dbdd22cac98d038c491dc
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Various JSON Object parameters had a
oneline_descriptionwith quote characters. Fix those, and extendrpcdoccheckto detect them.Also, slightly improve GBT's oneline description for template_request.