log: Enforce trailing newline #30929

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2409-log-nl changing 9 files +18 −238
  1. maflcko commented at 2:53 pm on September 19, 2024: member

    There are many problems around missing a trailing newline while logging:

    • All log lines are currently terminated by a trailing newline. This means any runtime code trying to handle a “missing” newline is currently dead code.
    • Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
    • It requires extra code like m_started_new_line to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see #30386 (review).
    • It requires a standalone unterminated-logprintf clang-tidy plugin, which is unmaintained (no one updated it for the new log function names), probably harder to maintain than normal C++ code (because it requires clang AST matcher knowledge), brittle (it can fail to detect issues at any time, if it goes out-of-sync, or be explicitly disabled via NOLINT), and annoying for devs (it is slow and intricate to run locally and thus only effectively run on CI or via the CI scripts).

    Fix all issues by enforcing the trailing newline in logs directly in the code. Then remove all the other stuff.

    This refactor does not change behavior.

  2. DrahtBot commented at 2:53 pm on September 19, 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, ryanofsky, achow101
    Stale ACK l0rinc

    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:

    • #30928 (tinyformat: refactor: increase compile-time checks and don’t throw for tfm::format_error by stickies-v)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

    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:
    log: Enforce trailing newline at compile time
    log: Enforce trailing newline at compile time
    on Sep 19, 2024
  4. DrahtBot added the label Utils/log/libs on Sep 19, 2024
  5. maflcko force-pushed on Sep 19, 2024
  6. DrahtBot added the label CI failed on Sep 19, 2024
  7. DrahtBot commented at 3:11 pm on September 19, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30383147556

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. in contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp:16 in fa30228869 outdated
    11@@ -13,7 +12,6 @@ class BitcoinModule final : public clang::tidy::ClangTidyModule
    12 public:
    13     void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override
    14     {
    15-        CheckFactories.registerCheck<bitcoin::LogPrintfCheck>("bitcoin-unterminated-logprintf");
    


    l0rinc commented at 3:14 pm on September 19, 2024:
    nice
  9. in src/index/base.cpp:31 in fa30228869 outdated
    27@@ -28,7 +28,7 @@ constexpr auto SYNC_LOG_INTERVAL{30s};
    28 constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
    29 
    30 template <typename... Args>
    31-void BaseIndex::FatalErrorf(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
    32+void BaseIndex::FatalErrorf(util::ConstevalFormatString<false, sizeof...(Args)> fmt, const Args&... args)
    


    l0rinc commented at 3:15 pm on September 19, 2024:

    nit: would it make sense to add a bool comment for these params?

    0void BaseIndex::FatalErrorf(util::ConstevalFormatString</*force_trailing_newline=*/false, sizeof...(Args)> fmt, const Args&... args)
    

    l0rinc commented at 6:54 pm on September 19, 2024:
    Even better, resolve it please
  10. fanquake requested review from stickies-v on Sep 19, 2024
  11. DrahtBot removed the label CI failed on Sep 19, 2024
  12. maflcko added the label Refactoring on Sep 19, 2024
  13. l0rinc commented at 6:01 pm on September 19, 2024: contributor
    concept ACK
  14. maflcko force-pushed on Sep 19, 2024
  15. in src/logging.h:242 in fa7190db83 outdated
    238@@ -246,7 +239,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
    239 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
    240 
    241 template <typename... Args>
    242-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
    243+inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)> fmt, const Args&... args)
    


    stickies-v commented at 10:49 am on September 20, 2024:

    nit: might be nice introducing a LogFmt alias here?

     0diff --git a/src/logging.h b/src/logging.h
     1index e0bd79f793..ed224565e6 100644
     2--- a/src/logging.h
     3+++ b/src/logging.h
     4@@ -239,7 +239,10 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
     5 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
     6 
     7 template <typename... Args>
     8-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)> fmt, const Args&... args)
     9+using LogFmt = util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)>;
    10+
    11+template <typename... Args>
    12+inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, LogFmt<Args...> fmt, const Args&... args)
    13 {
    14     if (LogInstance().Enabled()) {
    15         std::string log_msg;
    16diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
    17index e038c4c77a..a16f5c99d6 100644
    18--- a/src/wallet/scriptpubkeyman.h
    19+++ b/src/wallet/scriptpubkeyman.h
    20@@ -254,7 +254,7 @@ public:
    21 
    22     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
    23     template <typename... Params>
    24-    void WalletLogPrintf(util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Params)> wallet_fmt, const Params&... params) const
    25+    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const
    26     {
    27         LogInfo("%s %s\n", m_storage.GetDisplayName(), util::RemoveSuffixView(tfm::format(wallet_fmt, params...), "\n"));
    28     };
    29diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
    30index f85bebe179..807c95ae1c 100644
    31--- a/src/wallet/wallet.h
    32+++ b/src/wallet/wallet.h
    33@@ -927,7 +927,7 @@ public:
    34 
    35     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
    36     template <typename... Params>
    37-    void WalletLogPrintf(util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Params)> wallet_fmt, const Params&... params) const
    38+    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const
    39     {
    40         LogInfo("%s %s\n", GetDisplayName(), util::RemoveSuffixView(tfm::format(wallet_fmt, params...), "\n"));
    41     };
    

    maflcko commented at 11:54 am on September 20, 2024:
    Sure, done!
  16. stickies-v commented at 10:54 am on September 20, 2024: contributor
    Concept ACK on these checks being done at compile-time instead of through clang-tidy/linters/…, and the significant resulting code clean-up. Added verbosity can be hidden, e.g. by the alias suggested as a nit.
  17. maflcko force-pushed on Sep 20, 2024
  18. DrahtBot added the label Needs rebase on Sep 27, 2024
  19. maflcko force-pushed on Sep 27, 2024
  20. maflcko force-pushed on Sep 27, 2024
  21. DrahtBot removed the label Needs rebase on Sep 27, 2024
  22. maflcko commented at 1:45 pm on September 27, 2024: member

    (rebased, trival)

  23. in src/test/util_string_tests.cpp:32 in fac4a34d0a outdated
    30+template <unsigned WrongNumArgs, util::EndCheck end_check = util::NoEndCheck>
    31 inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
    32 {
    33-    BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
    34+    using WrongFmt = util::ConstevalFormatString<end_check, WrongNumArgs>;
    35+    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
    


    l0rinc commented at 1:59 pm on September 27, 2024:

    nit: if you edit again (the spirit of @hodlinator is haunting me here):

    0    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
    

    maflcko commented at 8:22 am on September 30, 2024:
    Ok, done
  24. in src/wallet/scriptpubkeyman.h:257 in fac4a34d0a outdated
    253@@ -254,9 +254,9 @@ class ScriptPubKeyMan
    254 
    255     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
    256     template <typename... Params>
    257-    void WalletLogPrintf(util::ConstevalFormatString<sizeof...(Params)> wallet_fmt, const Params&... params) const
    258+    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const
    


    l0rinc commented at 2:07 pm on September 27, 2024:

    nit: in tinyformat the print variant that appends a \n is called printfln -> please consider a similar naming (I’m not necessarily recommending it, just bringing it to your attention):

    0    void WalletLogPrintfln(LogFmt<Params...> wallet_fmt, const Params&... params) const
    

    If you disagree, just resolve it, no explanation needed.


    maflcko commented at 3:00 pm on September 27, 2024:

    It does not append a \n. It removes one and adds one, the effect of which is a nullopt.

    I understand that this is a bit confusing and it would be better if there were no \n anywhere in log strings, but this can be done in the future. For example when doing other breaking changes, like switching to std::format in C++23 in a few years down the line. Closing for now.

  25. in src/test/util_string_tests.cpp:37 in fac4a34d0a outdated
    35+    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
    36+}
    37+
    38+BOOST_AUTO_TEST_CASE(ConstevalFormatString_Newline)
    39+{
    40+    PassFmtNewline<0>("\n");
    


    l0rinc commented at 2:11 pm on September 27, 2024:

    nit: given that we’re removing \n specifically in https://github.com/bitcoin/bitcoin/blob/fabf6dcdd1b1e1250936444a80ef25fdf737e2f2/src/wallet/scriptpubkeyman.h#L259 - should we document what we expect to happen with Windows newlines (which also end with \n, but we’re not removing \r and adding back the \n later):

    0    PassFmtNewline<0>("\r\n");
    

    maflcko commented at 3:06 pm on September 27, 2024:
    There aren’t any Windows newlines anywhere in log lines, and this pull requests isn’t changing anything about that. So I’ll leave this as-is for now.
  26. in src/test/util_string_tests.cpp:41 in fac4a34d0a outdated
    38+BOOST_AUTO_TEST_CASE(ConstevalFormatString_Newline)
    39+{
    40+    PassFmtNewline<0>("\n");
    41+    PassFmtNewline<1>("%s\n");
    42+    PassFmtNewline<0>("%%\n");
    43+    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier
    


    l0rinc commented at 2:22 pm on September 27, 2024:
    This was already tested in ConstevalFormatString_NumSpec - maybe we could move the comment or remove that example

    maflcko commented at 3:07 pm on September 27, 2024:
    Seems fine to document twice. I don’t really see the risk or downside. Leaving as-is for now.
  27. in src/test/util_string_tests.cpp:42 in fac4a34d0a outdated
    39+{
    40+    PassFmtNewline<0>("\n");
    41+    PassFmtNewline<1>("%s\n");
    42+    PassFmtNewline<0>("%%\n");
    43+    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier
    44+
    


    l0rinc commented at 2:22 pm on September 27, 2024:
    we could add a few more corner cases, e.g. \n\n and \n -> otherwise the tests would pass for a contains("\n") or count("\n") == 1 as well

    maflcko commented at 8:23 am on September 30, 2024:
    Sure, done
  28. in src/logging.cpp:393 in fac4a34d0a outdated
    389@@ -389,23 +390,10 @@ void BCLog::Logger::LogPrintStr(std::string_view str, std::string_view logging_f
    390 
    391 void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
    392 {
    393+    Assume(str.ends_with("\n")); // Checked at compile-time, so this can never fail
    


    l0rinc commented at 2:30 pm on September 27, 2024:

    Assume should be used to document assumptions when program execution can safely continue even if the assumption is violated.

    Shouldn’t this be an assert instead?


    maflcko commented at 3:02 pm on September 27, 2024:

    A missing \n is a harmless, albeit confusing, stylistic issue in the debug logs. I’d very much say that the program can safely continue and that this is not a fatal error.

    Edit: It should be trivial to add the missing newline in this case at runtime, but I think playing code-golf on effectively dead code isn’t too useful.


    maflcko commented at 11:46 am on September 30, 2024:
    To clarify, with “code-golf on effectively dead code” I meant adding the newline while keeping the compile-time check. Either the newline is appended going forward, dropping the compile-time checks, or the compile-time checks are kept and the newline is appended at runtime. But doing both doesn’t make sense.

    ryanofsky commented at 12:16 pm on September 30, 2024:

    It seems a little crazy for logging code to use \n in format strings when logger doesn’t support ~multiline~ multipart log strings, and more crazy to add a compile time check enforcing presence of \n characters which serve no purpose, add noise, and give misleading impression that the logger supports ~multiline~ multipart strings when it doesn’t. I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.

    (EDIT: s/multiline/multipart/ above to be more precise. I’m referring to logging feature which used to exist which allowed subsequent log messages to be joined together if they didn’t end in \n)


    maflcko commented at 1:44 pm on September 30, 2024:

    I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.

    Thanks, pushed the diff from #30929 (comment)

  29. in src/test/util_string_tests.cpp:43 in fac4a34d0a outdated
    41+    PassFmtNewline<1>("%s\n");
    42+    PassFmtNewline<0>("%%\n");
    43+    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier
    44+
    45+    auto err_newline{"Format string must end with a newline"};
    46+    FailFmtWithError<1, util::TrailingNewlineCheck>("", err_newline);
    


    l0rinc commented at 2:36 pm on September 27, 2024:

    given that PassFmtNewline<1>("\n") would already fail - and this is an test for a missing newline, not invalid parameter count -, consider the following:

    0    FailFmtWithError<0, util::TrailingNewlineCheck>("", err_newline);
    

    maflcko commented at 8:23 am on September 30, 2024:
    Sure, done
  30. l0rinc commented at 2:41 pm on September 27, 2024: contributor
    Thanks for automating these! I left a few naming notes, a few additional test recommendations and an Assume -> assert suggestion based on its description.
  31. maflcko force-pushed on Sep 30, 2024
  32. maflcko commented at 9:48 am on September 30, 2024: member
    Force pushed some trivial test-only fixups.
  33. l0rinc approved
  34. l0rinc commented at 10:38 am on September 30, 2024: contributor
    ACK facfdcef4a512d4249c63134e632f6154652f022
  35. DrahtBot requested review from stickies-v on Sep 30, 2024
  36. maflcko commented at 10:53 am on September 30, 2024: member

    It would be possible to drop the first commit completely and just replace it with a patch that appends a newline if it is missing. This means that no behavior is enforced at compile-time and the newline placement in code could be inconsistent, but the thing that matters is fixed: A trailing newline is ensured to be always present.

     0diff --git a/src/logging.h b/src/logging.h
     1index f465622d0b..9998f85c35 100644
     2--- a/src/logging.h
     3+++ b/src/logging.h
     4@@ -256,7 +256,7 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin
     5             /* Original format string will have newline so don't add one here */
     6             log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
     7         }
     8+        if (!log_msg.ends_with('\n')) log_msg.push_back('\n');
     9         LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level);
    10     }
    11 }
    
  37. l0rinc commented at 11:02 am on September 30, 2024: contributor

    replace it with a patch that appends a newline if it is missing

    Didn’t you call this code-golf in #30929 (review)? I’d prefer keeping this explicit (as it i now) - or renaming the method to end with ln if we’re appending.

  38. maflcko renamed this:
    log: Enforce trailing newline at compile time
    log: Enforce trailing newline
    on Sep 30, 2024
  39. maflcko force-pushed on Sep 30, 2024
  40. ryanofsky commented at 2:49 pm on September 30, 2024: contributor

    Approach ACK fadce8893d766a9ead7493c1a00a885066156b00. Two things:

    • Would be good to clarify in the description that the reason why “This refactor does not change behavior” is that the only unterminated log prints were removed in #30485, so there is no code relying on the old behavior. (Assuming this is the case)

    • I wonder if it would be possible to simplify the implementation by just making the newline part of log formatting:

     0--- a/src/logging.cpp
     1+++ b/src/logging.cpp
     2@@ -369,6 +369,8 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
     3 
     4 void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
     5 {
     6+    if (!str.ends_with('\n')) str.push_back('\n');
     7+
     8     str.insert(0, GetLogPrefix(category, level));
     9 
    10     if (m_log_sourcelocations) {
    11@@ -390,7 +392,6 @@ void BCLog::Logger::LogPrintStr(std::string_view str, std::string_view logging_f
    12 
    13 void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
    14 {
    15-    Assume(str.ends_with("\n")); // Enforced at the call-site, so this can never fail
    16     std::string str_prefixed = LogEscapeMessage(str);
    17 
    18     if (m_buffering) {
    19--- a/src/logging.h
    20+++ b/src/logging.h
    21@@ -249,10 +249,6 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin
    22             /* Original format string will have newline so don't add one here */
    23             log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
    24         }
    25-        // Some log messages have a trailing newline, but this is not required
    26-        // going forward. However, LogPrintStr requires it, so add one if it
    27-        // was missing.
    28-        if (!log_msg.ends_with('\n')) log_msg.push_back('\n');
    29         LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level);
    30     }
    31 }
    
  41. maflcko force-pushed on Sep 30, 2024
  42. ryanofsky approved
  43. ryanofsky commented at 3:26 pm on September 30, 2024: contributor
    Code review ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f. Nice simplification removing dead code after #30485.
  44. DrahtBot requested review from l0rinc on Sep 30, 2024
  45. in src/logging.cpp:372 in fa6444fe62 outdated
    368@@ -368,6 +369,8 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    369 
    370 void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
    371 {
    372+    if (!str.ends_with('\n')) str.push_back('\n');
    


    stickies-v commented at 4:33 pm on September 30, 2024:

    Would be nice to add test coverage to prevent regressions? Imo can be as simple as slightly modifying the existing logging_LogPrintMacros test. E.g. this would fail on fae943042f38012ec3410cc8988928896e924352:

     0diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
     1index 8217a2385c..9cfdf8293a 100644
     2--- a/src/test/logging_tests.cpp
     3+++ b/src/test/logging_tests.cpp
     4@@ -135,8 +135,8 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup)
     5 {
     6     LogTrace(BCLog::NET, "foo6: %s\n", "bar6"); // not logged
     7     LogDebug(BCLog::NET, "foo7: %s\n", "bar7");
     8-    LogInfo("foo8: %s\n", "bar8");
     9-    LogWarning("foo9: %s\n", "bar9");
    10+    LogInfo("foo8: %s", "bar8");
    11+    LogWarning("foo9: %s", "bar9");
    12     LogError("foo10: %s\n", "bar10");
    13     std::ifstream file{tmp_log_path};
    14     std::vector<std::string> log_lines;
    

    stickies-v commented at 5:01 pm on September 30, 2024:

    maflcko commented at 9:35 am on October 1, 2024:
    Sure, done both

    laanwj commented at 9:36 am on October 3, 2024:
    thanks for doing it in this way instead of making it an error/warning, i like that we can now just do LogPrintfs without typing \n every time
  46. in contrib/devtools/bitcoin-tidy/example_logprintf.cpp:1 in fa6444fe62 outdated
    -1@@ -1,108 +0,0 @@
    0-// Copyright (c) 2023 Bitcoin Developers
    


    stickies-v commented at 4:37 pm on September 30, 2024:

    Suppression for this file can be cleaned up now:

     0diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py
     1index a809851ec6..86a17fb0f8 100755
     2--- a/test/lint/lint-format-strings.py
     3+++ b/test/lint/lint-format-strings.py
     4@@ -62,7 +62,7 @@ def main():
     5 
     6         matching_files_filtered = []
     7         for matching_file in matching_files:
     8-            if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)|contrib/devtools/bitcoin-tidy/example_logprintf.cpp', matching_file):
     9+            if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)', matching_file):
    10                 matching_files_filtered.append(matching_file)
    11         matching_files_filtered.sort()
    12 
    

    maflcko commented at 9:35 am on October 1, 2024:
    Sure, done
  47. stickies-v approved
  48. stickies-v commented at 5:02 pm on September 30, 2024: contributor

    ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f , I much prefer this new approach with less code complexity and a more user-friendly logging interface, that allows for a gradual transition to not having \n-terminated log statements.

    I wonder if it would make more sense to switch the commit order so the bitcoin-tidy check is removed after auto-appending the newline, instead of before?

    As a quick belt-and-suspenders way to check that there’s no behaviour change, I ran a -debug=all node for a while (as well as unit and functional test suite) with no issues with this diff applied:

     0diff --git a/src/logging.cpp b/src/logging.cpp
     1index 5f055566ef..917d3e9932 100644
     2--- a/src/logging.cpp
     3+++ b/src/logging.cpp
     4@@ -369,7 +369,7 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
     5 
     6 void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
     7 {
     8-    if (!str.ends_with('\n')) str.push_back('\n');
     9+    if (!str.ends_with('\n')) assert(false);
    10 
    11     str.insert(0, GetLogPrefix(category, level));
    12 
    
  49. log: Enforce trailing newline, Remove redundant m_started_new_line
    All log lines already have a trailing newline, but enforcing it allows
    to delete unused code.
    bbbb2e43ee
  50. Remove redundant unterminated-logprintf tidy check fa2b7d8d6b
  51. maflcko force-pushed on Oct 1, 2024
  52. stickies-v commented at 10:32 am on October 1, 2024: contributor

    re-ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab

    No changes except for rebase and addressing review comments (test/lint/doc updates and commit re-ordering).

  53. DrahtBot requested review from ryanofsky on Oct 1, 2024
  54. ryanofsky approved
  55. ryanofsky commented at 12:51 pm on October 1, 2024: contributor
    Code review ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab. Just comment and test cleanup since last review
  56. achow101 commented at 10:51 pm on October 2, 2024: member
    ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab
  57. achow101 merged this on Oct 2, 2024
  58. achow101 closed this on Oct 2, 2024

  59. maflcko deleted the branch on Oct 7, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 07:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me