Utils: Merge QtcProcess line reading functions

- Pass everything through the codec.
- Always emit even incomplete last lines when the process finishes.
- Don't store raw output when line-wise processing is requested.

Change-Id: I5cc30ad0d7ab79387bfb00b48ff957468a1bd004
Reviewed-by: hjk <hjk@qt.io>
Reviewed-by: Orgad Shaneh <orgads@gmail.com>
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/libs/utils/qtcprocess.cpp

@@ -81,17 +81,17 @@ class ChannelBuffer
 public:
     void clearForRun();
 
-    QString linesRead();
+    void handleRest();
-    Utils::optional<QString> takeFirstLine();
     void append(const QByteArray &text);
 
     QByteArray rawData;
     QString incompleteLineBuffer; // lines not yet signaled
     QTextCodec *codec = nullptr; // Not owner
     std::unique_ptr<QTextCodec::ConverterState> codecState;
-    int rawDataPos = 0;
     std::function<void(const QString &lines)> outputCallback;
-    std::function<void(const QString &line)> outputLineCallback;
+
+    bool emitSingleLines = true;
+    bool keepRawData = true;
 };
 
 class ProcessHelper : public QProcess
@@ -194,7 +194,6 @@ public:
     QtcProcess::Result m_result = QtcProcess::StartFailed;
     QProcess::ExitStatus m_exitStatus = QProcess::NormalExit;
     int m_exitCode = -1;
-    FilePath m_binary;
     ChannelBuffer m_stdOut;
     ChannelBuffer m_stdErr;
     ExitCodeInterpreter m_exitCodeInterpreter;
@@ -218,7 +217,6 @@ void QtcProcessPrivate::clearForRun()
     m_result = QtcProcess::StartFailed;
     m_exitCode = -1;
     m_startFailure = false;
-    m_binary = {};
 }
 
 QtcProcess::Result QtcProcessPrivate::interpretExitCode(int exitCode)
@@ -316,6 +314,8 @@ void QtcProcess::start(const QString &cmd, const QStringList &args)
 
 void QtcProcess::start()
 {
+    d->clearForRun();
+
     QTC_CHECK(d->m_writeData.isEmpty()); // FIXME: Use it.
 
     if (d->m_commandLine.executable().needsDevice()) {
@@ -729,6 +729,23 @@ void QtcProcess::close()
     d->m_process->close();
 }
 
+void QtcProcess::beginFeed()
+{
+    d->clearForRun();
+}
+
+void QtcProcess::endFeed()
+{
+    d->slotFinished(0, QProcess::NormalExit);
+}
+
+void QtcProcess::feedStdOut(const QByteArray &data)
+{
+    d->m_stdOut.append(data);
+    d->m_hangTimerCount = 0;
+    emit readyReadStandardOutput();
+}
+
 QString QtcProcess::locateBinary(const QString &binary)
 {
     const QByteArray path = qgetenv("PATH");
@@ -788,6 +805,8 @@ QString QtcProcess::exitMessage()
 
 QByteArray QtcProcess::allRawOutput() const
 {
+    QTC_CHECK(d->m_stdOut.keepRawData);
+    QTC_CHECK(d->m_stdErr.keepRawData);
     if (!d->m_stdOut.rawData.isEmpty() && !d->m_stdErr.rawData.isEmpty()) {
         QByteArray result = d->m_stdOut.rawData;
         if (!result.endsWith('\n'))
@@ -800,6 +819,8 @@ QByteArray QtcProcess::allRawOutput() const
 
 QString QtcProcess::allOutput() const
 {
+    QTC_CHECK(d->m_stdOut.keepRawData);
+    QTC_CHECK(d->m_stdErr.keepRawData);
     const QString out = stdOut();
     const QString err = stdErr();
 
@@ -815,16 +836,19 @@ QString QtcProcess::allOutput() const
 
 QString QtcProcess::stdOut() const
 {
+    QTC_CHECK(d->m_stdOut.keepRawData);
     return normalizeNewlines(d->m_codec->toUnicode(d->m_stdOut.rawData));
 }
 
 QString QtcProcess::stdErr() const
 {
+    QTC_CHECK(d->m_stdErr.keepRawData);
     return normalizeNewlines(d->m_codec->toUnicode(d->m_stdErr.rawData));
 }
 
 QByteArray QtcProcess::rawStdOut() const
 {
+    QTC_CHECK(d->m_stdOut.keepRawData);
     return d->m_stdOut.rawData;
 }
 
@@ -839,7 +863,6 @@ QTCREATOR_UTILS_EXPORT QDebug operator<<(QDebug str, const QtcProcess &r)
 
 void ChannelBuffer::clearForRun()
 {
-    rawDataPos = 0;
     rawData.clear();
     codecState.reset(new QTextCodec::ConverterState);
     incompleteLineBuffer.clear();
@@ -847,61 +870,66 @@ void ChannelBuffer::clearForRun()
 
 /* Check for complete lines read from the device and return them, moving the
  * buffer position. */
-QString ChannelBuffer::linesRead()
-{
-    // Convert and append the new input to the buffer of incomplete lines
-    const char *start = rawData.constData() + rawDataPos;
-    const int len = rawData.size() - rawDataPos;
-    incompleteLineBuffer.append(codec->toUnicode(start, len, codecState.get()));
-    rawDataPos = rawData.size();
-
-    // Any completed lines in the incompleteLineBuffer?
-    const int lastLineIndex = qMax(incompleteLineBuffer.lastIndexOf('\n'),
-                                   incompleteLineBuffer.lastIndexOf('\r'));
-    if (lastLineIndex == -1)
-        return QString();
-
-    // Get completed lines and remove them from the incompleteLinesBuffer:
-    const QString lines = QtcProcess::normalizeNewlines(incompleteLineBuffer.left(lastLineIndex + 1));
-    incompleteLineBuffer = incompleteLineBuffer.mid(lastLineIndex + 1);
-
-    return lines;
-}
-
-// Check for first complete line inside the rawData and return it, removing the line from the buffer
-Utils::optional<QString> ChannelBuffer::takeFirstLine()
-{
-    const int firstLineEnd = qMax(rawData.indexOf('\n'), rawData.indexOf('\r'));
-    if (firstLineEnd == -1)
-        return Utils::nullopt;
-
-    const QString line = QString::fromUtf8(rawData.left(firstLineEnd));
-    rawData.remove(0, firstLineEnd + 1);
-    return line;
-}
-
 void ChannelBuffer::append(const QByteArray &text)
 {
     if (text.isEmpty())
         return;
+
+    if (keepRawData)
         rawData += text;
 
-    // Buffered. Emit complete lines?
+    // Line-wise operation below:
-    if (outputCallback) {
+    if (!outputCallback)
-        const QString lines = linesRead();
+        return;
-        if (!lines.isEmpty())
+
-            outputCallback(lines);
+    // Convert and append the new input to the buffer of incomplete lines
-    }
+    incompleteLineBuffer.append(codec->toUnicode(text.constData(), text.size(), codecState.get()));
-    if (outputLineCallback) {
+
     do {
-            const Utils::optional<QString> line = takeFirstLine();
+        // Any completed lines in the incompleteLineBuffer?
-            if (!line.has_value())
+        int pos = -1;
-                break;
+        if (emitSingleLines) {
-            outputLineCallback(line.value());
+            const int posn = incompleteLineBuffer.indexOf('\n');
-        } while (true);
+            const int posr = incompleteLineBuffer.indexOf('\r');
+            if (posn != -1) {
+                if (posr != -1) {
+                    if (posn == posr + 1)
+                        pos = posn; // \r followed by \n -> line end, use the \n.
+                    else
+                        pos = qMin(posr, posn); // free floating \r and \n: Use the first one.
+                } else {
+                    pos = posn;
                 }
+            } else {
+                pos = posr; // Make sure internal '\r' triggers a line output
+            }
+        } else {
+            pos = qMax(incompleteLineBuffer.lastIndexOf('\n'),
+                       incompleteLineBuffer.lastIndexOf('\r'));
         }
 
+        if (pos == -1)
+            break;
+
+        // Get completed lines and remove them from the incompleteLinesBuffer:
+        const QString line = QtcProcess::normalizeNewlines(incompleteLineBuffer.left(pos + 1));
+        incompleteLineBuffer = incompleteLineBuffer.mid(pos + 1);
+
+        QTC_ASSERT(outputCallback, return);
+        outputCallback(line);
+
+        if (!emitSingleLines)
+            break;
+    } while (true);
+}
+
+void ChannelBuffer::handleRest()
+{
+    if (outputCallback && !incompleteLineBuffer.isEmpty()) {
+        outputCallback(incompleteLineBuffer);
+        incompleteLineBuffer.clear();
+    }
+}
 
 // ----------- SynchronousProcess
 SynchronousProcess::SynchronousProcess()
@@ -980,10 +1008,6 @@ void SynchronousProcess::runBlocking()
         << " process user events: " << d->m_processUserEvents;
     ExecuteOnDestruction logResult([this] { qCDebug(processLog) << *this; });
 
-    d->clearForRun();
-
-    d->m_binary = d->m_commandLine.executable();
-
     if (d->m_processUserEvents) {
         if (!d->m_writeData.isEmpty()) {
             connect(d->m_process, &QProcess::started, this, [this] {
@@ -1042,22 +1066,31 @@ void QtcProcess::setStdOutCallback(const std::function<void (const QString &)> &
 {
     QTC_CHECK(d->m_isSynchronousProcess);
     d->m_stdOut.outputCallback = callback;
+    d->m_stdOut.emitSingleLines = false;
+    d->m_stdOut.emitSingleLines = false;
+    d->m_stdOut.keepRawData = false;
 }
 
 void QtcProcess::setStdOutLineCallback(const std::function<void (const QString &)> &callback)
 {
-    d->m_stdOut.outputLineCallback = callback;
+    d->m_stdOut.outputCallback = callback;
+    d->m_stdOut.emitSingleLines = true;
+    d->m_stdOut.keepRawData = false;
 }
 
 void QtcProcess::setStdErrCallback(const std::function<void (const QString &)> &callback)
 {
     QTC_CHECK(d->m_isSynchronousProcess);
     d->m_stdErr.outputCallback = callback;
+    d->m_stdErr.emitSingleLines = false;
+    d->m_stdErr.keepRawData = false;
 }
 
 void QtcProcess::setStdErrLineCallback(const std::function<void (const QString &)> &callback)
 {
-    d->m_stdErr.outputLineCallback = callback;
+    d->m_stdErr.outputCallback = callback;
+    d->m_stdErr.emitSingleLines = true;
+    d->m_stdErr.keepRawData = false;
 }
 
 void QtcProcessPrivate::slotTimeout()
@@ -1066,7 +1099,8 @@ void QtcProcessPrivate::slotTimeout()
         if (debug)
             qDebug() << Q_FUNC_INFO << "HANG detected, killing";
         m_waitingForUser = true;
-        const bool terminate = !m_timeOutMessageBoxEnabled || askToKill(m_binary.toString());
+        const bool terminate = !m_timeOutMessageBoxEnabled
+            || askToKill(m_commandLine.executable().toString());
         m_waitingForUser = false;
         if (terminate) {
             q->stopProcess();
@@ -1100,6 +1134,10 @@ void QtcProcessPrivate::slotFinished(int exitCode, QProcess::ExitStatus status)
         break;
     }
     m_eventLoop.quit();
+
+    m_stdOut.handleRest();
+    m_stdErr.handleRest();
+
     emit q->finished();
 }
 

src/libs/utils/qtcprocess.h

@@ -37,6 +37,8 @@
 
 QT_FORWARD_DECLARE_CLASS(QDebug)
 
+class tst_QtcProcess;
+
 namespace Utils {
 
 class CommandLine;
@@ -186,6 +188,11 @@ private:
 
     Internal::QtcProcessPrivate *d = nullptr;
 
+    friend tst_QtcProcess;
+    void beginFeed();
+    void feedStdOut(const QByteArray &data);
+    void endFeed();
+
     void setProcessEnvironment(const QProcessEnvironment &environment) = delete;
     QProcessEnvironment processEnvironment() const = delete;
 };

tests/auto/utils/qtcprocess/tst_qtcprocess.cpp

@@ -23,14 +23,23 @@
 **
 ****************************************************************************/
 
-#include <utils/qtcprocess.h>
-#include <utils/hostosinfo.h>
-#include <utils/stringutils.h>
 #include <utils/environment.h>
+#include <utils/hostosinfo.h>
+#include <utils/porting.h>
+#include <utils/qtcprocess.h>
+#include <utils/stringutils.h>
 
+#include <QRegularExpression>
 #include <QtTest>
 
 #include <iostream>
+#include <fstream>
+
+#ifdef Q_OS_WIN
+  #include <io.h>
+  #include <fcntl.h>
+#endif
+
 
 using namespace Utils;
 
@@ -39,13 +48,13 @@ const char kRunBlockingStdOutSubProcessMagicWord[] = "42";
 const char kRunBlockingStdOutSubProcessWithEndl[] = "QTC_TST_QTCPROCESS_RUNBLOCKINGSTDOUT_WITHENDL";
 const char kLineCallback[] = "QTC_TST_QTCPROCESS_LINECALLBACK";
 
-Q_GLOBAL_STATIC_WITH_ARGS(const QStringList, lineCallbackData,
+// Expect ending lines detected at '|':
-                          ({
+const char lineCallbackData[] =
-                               "This is the first line\r",
+       "This is the first line\r\n|"
-                               "Here comes the second one\r",
+       "Here comes the second one\r\n|"
-                               "Let's also have a third one\r",
+       "And a line without LF\n|"
-                               "Actually four are better\r",
+       "Rebasing (1/10)\r| <delay> Rebasing (2/10)\r| <delay> ...\r\n|"
-                           }))
+       "And no end";
 
 static void exitCodeSubProcessMain()
 {
@@ -68,8 +77,11 @@ static void blockingStdOutSubProcessMain()
 
 static void lineCallbackMain()
 {
-    for (const QString &line : *lineCallbackData())
+#ifdef Q_OS_WIN
-        std::cerr << qPrintable(line);
+    // Prevent \r\n -> \r\r\n translation.
+    setmode(fileno(stderr), O_BINARY);
+#endif
+    fprintf(stderr, "%s", QByteArray(lineCallbackData).replace('|', "").data());
     exit(0);
 }
 
@@ -119,6 +131,7 @@ private slots:
     void runBlockingStdOut_data();
     void runBlockingStdOut();
     void lineCallback();
+    void lineCallbackIntern();
 
 private:
     void iteratorEditsHelper(OsType osType);
@@ -885,8 +898,6 @@ void tst_QtcProcess::runBlockingStdOut()
     QEXPECT_FAIL("Unterminated stdout lost: early timeout", "", Continue);
     QVERIFY2(sp.result() != QtcProcess::Hang, "Process run did not time out.");
 
-    QEXPECT_FAIL("Unterminated stdout lost: early timeout", "", Continue);
-    QEXPECT_FAIL("Unterminated stdout lost: hanging", "", Continue);
     QVERIFY2(readLastLine, "Last line was read.");
 }
 
@@ -899,14 +910,34 @@ void tst_QtcProcess::lineCallback()
     Environment env = Environment::systemEnvironment();
     env.set(kLineCallback, "Yes");
     process.setEnvironment(env);
+    QStringList lines = QString(lineCallbackData).split('|');
     int lineNumber = 0;
-    process.setStdErrLineCallback([&lineNumber](const QString &actual) {
+    process.setStdErrLineCallback([lines, &lineNumber](const QString &actual) {
-        const QString expected = lineCallbackData()->at(lineNumber++).trimmed();
+        QString expected = lines.at(lineNumber++);
+        expected.replace("\r\n", "\n");
         QCOMPARE(actual, expected);
     });
     process.start();
     process.waitForFinished();
+    QCOMPARE(lineNumber, lines.size());
 }
+
+void tst_QtcProcess::lineCallbackIntern()
+{
+    QtcProcess process;
+    QStringList lines = QString(lineCallbackData).split('|');
+    int lineNumber = 0;
+    process.setStdOutLineCallback([lines, &lineNumber](const QString &actual) {
+        QString expected = lines.at(lineNumber++);
+        expected.replace("\r\n", "\n");
+        QCOMPARE(actual, expected);
+    });
+    process.beginFeed();
+    process.feedStdOut(QByteArray(lineCallbackData).replace('|', ""));
+    process.endFeed();
+    QCOMPARE(lineNumber, lines.size());
+}
+
 QTEST_MAIN(tst_QtcProcess)
 
 #include "tst_qtcprocess.moc"