Git: Return Result<QString> from GitClient::synchronizedLog()

Plan is to simplify the overall logic with less "shared"
out-parameters.

Complication in this change here is the magic in msgCannotRun() to
show or not show the error immediately, depending on whether
errorMessage is nullptr or not.

Since we now always pass a non-null errorMessage argument, some
VcsOutputWindow::appendError(...) calls have to be moved to the
caller side.

Change-Id: Ibd20d7ba0811ec34f86dc96914d4fbe7ae446ada
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/git/branchmodel.cpp

@@ -707,7 +707,6 @@ QModelIndex BranchModel::addBranch(const QString &name, bool track, const QModel
     const QString trackedBranch = fullName(startPoint);
     const QString fullTrackedBranch = fullName(startPoint, true);
     QString startSha;
-    QString output;
     QString errorMessage;
     QDateTime branchDateTime;
 
@@ -717,15 +716,20 @@ QModelIndex BranchModel::addBranch(const QString &name, bool track, const QModel
         startSha = sha(startPoint);
         branchDateTime = dateTime(startPoint);
     } else {
-        const QStringList arguments({"-n1", "--format=%H %ct"});
-        if (gitClient().synchronousLog(d->workingDirectory, arguments, &output, &errorMessage,
-                                       RunFlags::SuppressCommandLogging)) {
-            const QStringList values = output.split(' ');
+        const Result<QString> res = gitClient().synchronousLog(d->workingDirectory,
+                                                               {"-n1", "--format=%H %ct"},
+                                                               RunFlags::SuppressCommandLogging);
+        if (res) {
+            const QStringList values = res.value().split(' ');
             startSha = values[0];
             branchDateTime = QDateTime::fromSecsSinceEpoch(values[1].toLongLong());
+        } else {
+            errorMessage = res.error();
         }
     }
 
+    QString output;
+
     if (!gitClient().synchronousBranchCmd(d->workingDirectory, args, &output, &errorMessage)) {
         VcsOutputWindow::appendError(errorMessage);
         return {};
@@ -998,13 +1002,9 @@ void BranchModel::Private::updateAllUpstreamStatus(BranchNode *node)
 QString BranchModel::toolTip(const QString &sha) const
 {
     // Show the sha description excluding diff as toolTip
-    QString output;
-    QString errorMessage;
-    if (!gitClient().synchronousLog(d->workingDirectory, {"-n1", sha}, &output, &errorMessage,
-                                    RunFlags::SuppressCommandLogging)) {
-        return errorMessage;
-    }
-    return output;
+    const Result<QString> res = gitClient().synchronousLog(d->workingDirectory, {"-n1", sha},
+                                                           RunFlags::SuppressCommandLogging);
+    return res ? res.value() : res.error();
 }
 
 } // Git::Internal

src/plugins/git/gitclient.cpp

@@ -1481,8 +1481,9 @@ void GitClient::addFile(const FilePath &workingDirectory, const QString &fileNam
     vcsExec(workingDirectory, {"add", fileName});
 }
 
-bool GitClient::synchronousLog(const FilePath &workingDirectory, const QStringList &arguments,
-                               QString *output, QString *errorMessageIn, RunFlags flags)
+Result<QString> GitClient::synchronousLog(const FilePath &workingDirectory,
+                                          const QStringList &arguments,
+                                          RunFlags flags)
 {
     QStringList allArguments = {"log", noColorOption};
 
@@ -1490,13 +1491,13 @@ bool GitClient::synchronousLog(const FilePath &workingDirectory, const QStringLi
 
     const CommandResult result = vcsSynchronousExec(workingDirectory, allArguments, flags,
                         vcsTimeoutS(), encoding(EncodingLogOutput, workingDirectory));
-    if (result.result() == ProcessResult::FinishedWithSuccess) {
-        *output = result.cleanedStdOut();
-        return true;
-    }
+    if (result.result() == ProcessResult::FinishedWithSuccess)
+        return result.cleanedStdOut();
+
+    QString errorMessage;
     msgCannotRun(Tr::tr("Cannot obtain log of \"%1\": %2")
-                 .arg(workingDirectory.toUserOutput(), result.cleanedStdErr()), errorMessageIn);
-    return false;
+                 .arg(workingDirectory.toUserOutput(), result.cleanedStdErr()), &errorMessage);
+    return ResultError(errorMessage);
 }
 
 bool GitClient::synchronousAdd(const FilePath &workingDirectory,
@@ -2757,8 +2758,12 @@ Result<CommitData> GitClient::getCommitData(CommitType commitType, const FilePat
     QString errorMessage;
     QString output;
     if (commitData.commitType == FixupCommit) {
-        synchronousLog(repoDirectory, {HEAD, "--not", "--remotes", "-n1"}, &output, &errorMessage,
-                       RunFlags::SuppressCommandLogging);
+        const Result<QString> res = synchronousLog(repoDirectory, {HEAD, "--not", "--remotes", "-n1"},
+                                                   RunFlags::SuppressCommandLogging);
+        if (res)
+            output = res.value();
+        else
+            errorMessage = res.error();
         if (output.isEmpty())
             return ResultError(msgNoCommits(false));
     } else {
@@ -3710,10 +3715,14 @@ QString GitClient::suggestedLocalBranchName(
     if (targetType == BranchTargetType::Remote) {
         initialName = target.mid(target.lastIndexOf('/') + 1);
     } else {
-        QString subject;
-        gitClient().synchronousLog(workingDirectory, {"-n", "1", "--format=%s", target},
-                                   &subject, nullptr, RunFlags::NoOutput);
-        initialName = subject.trimmed();
+        const Result<QString> res =
+                gitClient().synchronousLog(workingDirectory,
+                                           {"-n", "1", "--format=%s", target},
+                                           RunFlags::NoOutput);
+        if (res)
+            initialName = res.value().trimmed();
+        else
+            VcsOutputWindow::appendError(res.error());
     }
     QString suggestedName = initialName;
     int i = 2;

src/plugins/git/gitclient.h

@@ -177,9 +177,9 @@ public:
     void removeStaleRemoteBranches(const Utils::FilePath &workingDirectory, const QString &remote);
     void recoverDeletedFiles(const Utils::FilePath &workingDirectory);
     void addFile(const Utils::FilePath &workingDirectory, const QString &fileName);
-    bool synchronousLog(const Utils::FilePath &workingDirectory, const QStringList &arguments,
-                        QString *output, QString *errorMessage = nullptr,
-                        VcsBase::RunFlags flags = VcsBase::RunFlags::None);
+    Utils::Result<QString> synchronousLog(const Utils::FilePath &workingDirectory,
+                                          const QStringList &arguments,
+                                          VcsBase::RunFlags flags = VcsBase::RunFlags::None);
     bool synchronousAdd(const Utils::FilePath &workingDirectory, const QStringList &files,
                         const QStringList &extraOptions = {});
     bool synchronousDelete(const Utils::FilePath &workingDirectory,

src/plugins/git/logchangedialog.cpp

@@ -167,12 +167,14 @@ bool LogChangeWidget::populateLog(const FilePath &repository, const QString &com
         arguments << "--not" << remotesFlag;
     }
     arguments << "--";
-    QString output;
-    if (!gitClient().synchronousLog(
-                repository, arguments, &output, nullptr, RunFlags::NoOutput)) {
+
+    const Result<QString> res = gitClient().synchronousLog(repository, arguments, RunFlags::NoOutput);
+    if (!res) {
+        VcsOutputWindow::appendError(res.error());
         return false;
     }
-    const QStringList lines = output.split('\n');
+
+    const QStringList lines = res.value().split('\n');
     for (const QString &line : lines) {
         const int colonPos = line.indexOf(':');
         if (colonPos != -1) {