scripted-diff: LogPrint -> LogDebug #30750

1. 
   maflcko commented at 9:33 am on August 29, 2024: member

   LogPrint has many issues:

   • It seems to indicate that something is being “printed”, however config options such as -printtoconsole actually control what and where something is logged.
   • It does not mention the log severity (debug).
   • It is a deprecated alias for LogDebug, according to the dev notes.
   • It wastes review cycles, because reviewers sometimes point out that it is deprecated.
   • It makes the code inconsistent, when both are used, possibly even in lines right next to each other (like in InitHTTPServer)

   Fix all issues by removing the deprecated alias.

   I checked all conflicting pull requests and at the time of writing there are no conflicts, except in pull requests that are marked as draft, are yet unreviewed, or are blocked on feedback for other reasons. So I think it is fine to do now.

2. 
   DrahtBot commented at 9:34 am on August 29, 2024: contributor

   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	stickies-v

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30673 (coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries by andrewtoth)
   • #30661 (fuzz: Test headers pre-sync through p2p by marcofleon)
   • #30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
   • #30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)
   • #30110 (refactor: TxDownloadManager + fuzzing by glozow)
   • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
   • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
   • #29256 (Improve new LogDebug/Trace/Info/Warning/Error Macros by ryanofsky)
   • #28676 ([WIP] Cluster mempool implementation by sdaftuar)
   • #28521 (net, net_processing: additional and consistent disconnect logging by Sjors)
   • #28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)
   • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
   • #25832 (tracing: network connection tracepoints by 0xB10C)
   • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

   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.

3. DrahtBot renamed this:
   scripted-diff: LogPrint -> LogDebug
   scripted-diff: LogPrint -> LogDebug
   on Aug 29, 2024
4. DrahtBot added the label Refactoring on Aug 29, 2024
5. in src/test/logging_tests.cpp:114 in fa6b80b2d2 outdated

   110@@ -111,7 +111,7 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintf_, LogSetup)
   111 BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacrosDeprecated, LogSetup)
   112 {
   113     LogPrintf("foo5: %s\n", "bar5");
   114-    LogPrint(BCLog::NET, "foo6: %s\n", "bar6");
   115+    LogDebug(BCLog::NET, "foo6: %s\n", "bar6");
   

   ---

   ---

   

   stickies-v commented at 10:03 am on August 29, 2024:

   This should not be in logging_LogPrintMacrosDeprecated, and since it’s already covered in the next test logging_LogPrintMacrosDeprecated I think this line can simply be removed in the second commit.
6. in src/bench/logging.cpp:43 in fa6b80b2d2 outdated

   41@@ -42,12 +42,12 @@ static void LogPrintLevelWithoutThreadNames(benchmark::Bench& bench)
   42 
   43 static void LogPrintWithCategory(benchmark::Bench& bench)
   

   ---

   ---

   

   stickies-v commented at 10:08 am on August 29, 2024:

   nit: fn name (+ below) should be updated to LogDebugWithCategory (+ also in docstring on L14)

   0static void LogDebugWithCategory(benchmark::Bench& bench)
   

   ---

   

   stickies-v commented at 1:45 pm on August 29, 2024:

   Thanks for addressing, the docstring still needs updating: https://github.com/bitcoin/bitcoin/pull/30750/commits/faa7db7d3c22c71d0cd25887abec097a044659e0#diff-b65a90c310b14b523533c724ac297ed970168e01e9589352700c7c52353609f3L14
7. stickies-v approved
8. 
   stickies-v commented at 10:17 am on August 29, 2024: contributor

   ACK fa6b80b2d2b8efcdb05c5940b380cb5c350efedd

   As already addressed in PR description, there are a fair amount of conflicts with other PRs, but the fact that it’s a trivial scripted-diff makes it very easy to incorporate this change with git rebase --exec.

   It wastes review cycles, because reviewers sometimes point out that it is deprecated.

   This alone is worth the (again, trivial) merge conflicts imo.

9. scripted-diff: LogPrint -> LogDebug

   -BEGIN VERIFY SCRIPT-
    sed -i 's/\<LogPrint\>/LogDebug/g' $( git grep -l '\<LogPrint\>'  -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
   -END VERIFY SCRIPT-

   3333415890
10. maflcko force-pushed on Aug 29, 2024
11. maflcko force-pushed on Aug 29, 2024
12. refactor: Remove unused LogPrint fa09cb41f5
13. maflcko force-pushed on Aug 29, 2024
14. stickies-v approved
15. 
    stickies-v commented at 2:07 pm on August 29, 2024: contributor

    ACK fa09cb41f58d0483ffe134eb274b9048c5260faa
16. maflcko requested review from TheCharlatan on Sep 2, 2024
17. 
    danielabrozzoni commented at 10:50 am on September 2, 2024: contributor

    utACK fa09cb41f58d0483ffe134eb274b9048c5260faa
18. TheCharlatan approved
19. 
    TheCharlatan commented at 10:52 am on September 2, 2024: contributor

    ACK fa09cb41f58d0483ffe134eb274b9048c5260faa
20. fanquake merged this on Sep 2, 2024
21. fanquake closed this on Sep 2, 2024

    ---

22. maflcko deleted the branch on Sep 2, 2024
23. 
    Sjors commented at 8:07 am on September 3, 2024: member

    The merge conflicts for #28521 were indeed easy to handle.

Contributors
maflcko DrahtBot stickies-v danielabrozzoni TheCharlatan Sjors

Labels
Refactoring