Merge pull request #12604 from NixOS/issue-12599

Fix chopped up repl output

Author: roberth

src/libcmd/repl.cc

@@ -102,8 +102,7 @@ struct NixRepl
                               unsigned int maxDepth = std::numeric_limits<unsigned int>::max())
     {
         // Hide the progress bar during printing because it might interfere
-        logger->pause();
-        Finally resumeLoggerDefer([]() { logger->resume(); });
+        auto suspension = logger->suspend();
         ::nix::printValue(*state, str, v, PrintOptions {
             .ansiColors = true,
             .force = true,
@@ -180,18 +179,20 @@ ReplExitStatus NixRepl::mainLoop()
 
     while (true) {
         // Hide the progress bar while waiting for user input, so that it won't interfere.
-        logger->pause();
-        // When continuing input from previous lines, don't print a prompt, just align to the same
-        // number of chars as the prompt.
-        if (!interacter->getLine(input, input.empty() ? ReplPromptType::ReplPrompt : ReplPromptType::ContinuationPrompt)) {
-            // Ctrl-D should exit the debugger.
-            state->debugStop = false;
-            logger->cout("");
-            // TODO: Should Ctrl-D exit just the current debugger session or
-            // the entire program?
-            return ReplExitStatus::QuitAll;
+        {
+            auto suspension = logger->suspend();
+            // When continuing input from previous lines, don't print a prompt, just align to the same
+            // number of chars as the prompt.
+            if (!interacter->getLine(input, input.empty() ? ReplPromptType::ReplPrompt : ReplPromptType::ContinuationPrompt)) {
+                // Ctrl-D should exit the debugger.
+                state->debugStop = false;
+                logger->cout("");
+                // TODO: Should Ctrl-D exit just the current debugger session or
+                // the entire program?
+                return ReplExitStatus::QuitAll;
+            }
+            // `suspension` resumes the logger
         }
-        logger->resume();
         try {
             switch (processLine(input)) {
                 case ProcessLineResult::Quit:
@@ -586,6 +587,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
     else if (command == ":p" || command == ":print") {
         Value v;
         evalString(arg, v);
+        auto suspension = logger->suspend();
         if (v.type() == nString) {
             std::cout << v.string_view();
         } else {
@@ -694,6 +696,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
         } else {
             Value v;
             evalString(line, v);
+            auto suspension = logger->suspend();
             printValue(std::cout, v, 1);
             std::cout << std::endl;
         }

src/libfetchers/git.cc

@@ -351,8 +351,7 @@ struct GitInputScheme : InputScheme
 
             if (commitMsg) {
                 // Pause the logger to allow for user input (such as a gpg passphrase) in `git commit`
-                logger->pause();
-                Finally restoreLogger([]() { logger->resume(); });
+                auto suspension = logger->suspend();
                 runProgram("git", true,
                     { "-C", repoPath->string(), "--git-dir", repoInfo.gitDir, "commit", std::string(path.rel()), "-F", "-" },
                     *commitMsg);

src/libmain/progress-bar.cc

@@ -73,8 +73,13 @@ class ProgressBar : public Logger
         uint64_t corruptedPaths = 0, untrustedPaths = 0;
 
         bool active = true;
-        bool paused = false;
+        size_t suspensions = 0;
         bool haveUpdate = true;
+
+        bool isPaused() const
+        {
+            return suspensions > 0;
+        }
     };
 
     /** Helps avoid unnecessary redraws, see `redraw()` */
@@ -130,18 +135,30 @@ class ProgressBar : public Logger
 
     void pause() override {
         auto state (state_.lock());
-        state->paused = true;
+        state->suspensions++;
+        if (state->suspensions > 1) {
+            // already paused
+            return;
+        }
+
         if (state->active)
             writeToStderr("\r\e[K");
     }
 
     void resume() override {
         auto state (state_.lock());
-        state->paused = false;
-        if (state->active)
-            writeToStderr("\r\e[K");
-        state->haveUpdate = true;
-        updateCV.notify_one();
+        if (state->suspensions == 0) {
+            log(lvlError, "nix::ProgressBar: resume() called without a matching preceding pause(). This is a bug.");
+            return;
+        } else {
+            state->suspensions--;
+        }
+        if (state->suspensions == 0) {
+            if (state->active)
+                writeToStderr("\r\e[K");
+            state->haveUpdate = true;
+            updateCV.notify_one();
+        }
     }
 
     bool isVerbose() override
@@ -383,7 +400,7 @@ class ProgressBar : public Logger
         auto nextWakeup = std::chrono::milliseconds::max();
 
         state.haveUpdate = false;
-        if (state.paused || !state.active) return nextWakeup;
+        if (state.isPaused() || !state.active) return nextWakeup;
 
         std::string line;
 

src/libstore/ssh.cc

@@ -117,10 +117,10 @@ std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(
     ProcessOptions options;
     options.dieWithParent = false;
 
+    std::unique_ptr<Logger::Suspension> loggerSuspension;
     if (!fakeSSH && !useMaster) {
-        logger->pause();
+        loggerSuspension = std::make_unique<Logger::Suspension>(logger->suspend());
     }
-    Finally cleanup = [&]() { logger->resume(); };
 
     conn->sshPid = startProcess([&]() {
         restoreProcessContext();
@@ -199,8 +199,7 @@ Path SSHMaster::startMaster()
     ProcessOptions options;
     options.dieWithParent = false;
 
-    logger->pause();
-    Finally cleanup = [&]() { logger->resume(); };
+    auto suspension = logger->suspend();
 
     if (isMasterRunning())
         return state->socketPath;

src/libutil/logging.cc

@@ -43,6 +43,19 @@ void Logger::writeToStdout(std::string_view s)
     writeFull(standard_out, "\n");
 }
 
+Logger::Suspension Logger::suspend()
+{
+    pause();
+    return Suspension { ._finalize = {[this](){this->resume();}} };
+}
+
+std::optional<Logger::Suspension> Logger::suspendIf(bool cond)
+{
+    if (cond)
+        return suspend();
+    return {};
+}
+
 class SimpleLogger : public Logger
 {
 public:

src/libutil/logging.hh

@@ -4,6 +4,7 @@
 #include "error.hh"
 #include "config.hh"
 #include "file-descriptor.hh"
+#include "finally.hh"
 
 #include <nlohmann/json_fwd.hpp>
 
@@ -75,6 +76,17 @@ public:
 
     virtual void stop() { };
 
+    /**
+     * Guard object to resume the logger when done.
+     */
+    struct Suspension {
+        Finally<std::function<void()>> _finalize;
+    };
+
+    Suspension suspend();
+
+    std::optional<Suspension> suspendIf(bool cond);
+
     virtual void pause() { };
     virtual void resume() { };
 

src/libutil/unix/processes.cc

@@ -306,15 +306,7 @@ void runProgram2(const RunOptions & options)
     // case), so we can't use it if we alter the environment
     processOptions.allowVfork = !options.environment;
 
-    std::optional<Finally<std::function<void()>>> resumeLoggerDefer;
-    if (options.isInteractive) {
-        logger->pause();
-        resumeLoggerDefer.emplace(
-            []() {
-                logger->resume();
-            }
-        );
-    }
+    auto suspension = logger->suspendIf(options.isInteractive);
 
     /* Fork. */
     Pid pid = startProcess([&] {

src/libutil/windows/processes.cc

@@ -312,11 +312,7 @@ void runProgram2(const RunOptions & options)
     // TODO: Implement shebang / program interpreter lookup on Windows
     auto interpreter = getProgramInterpreter(realProgram);
 
-    std::optional<Finally<std::function<void()>>> resumeLoggerDefer;
-    if (options.isInteractive) {
-        logger->pause();
-        resumeLoggerDefer.emplace([]() { logger->resume(); });
-    }
+    auto suspension = logger->suspendIf(options.isInteractive);
 
     Pid pid = spawnProcess(interpreter.has_value() ? *interpreter : realProgram, options, out, in);