#5662: Error handling

plugins/vcs/Commit.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "Repository.h"
+#include "GitException.h"
 #include "Tree.h"
 #include <git2.h>
 
@@ -30,10 +31,21 @@ class Commit final
     std::shared_ptr<Tree> getTree()
     {
         git_tree* tree;
-        git_commit_tree(&tree, _commit);
+        auto error = git_commit_tree(&tree, _commit);
+        GitException::ThrowOnError(error);
 
         return std::make_shared<Tree>(tree);
     }
+
+    static Ptr CreateFromOid(git_repository* repository, git_oid* oid)
+    {
+        git_commit* commit;
+
+        auto error = git_commit_lookup(&commit, repository, oid);
+        GitException::ThrowOnError(error);
+
+        return std::make_shared<Commit>(commit);
+    }
 };
 
 }

plugins/vcs/GitException.h

@@ -0,0 +1,46 @@
+#pragma once
+
+#include <stdexcept>
+#include <git2.h>
+#include "itextstream.h"
+
+namespace vcs
+{
+
+namespace git
+{
+
+class GitException :
+    public std::runtime_error
+{
+public:
+    GitException(const std::string& message) :
+        runtime_error(GetLastErrorMessage() + "\n" + message)
+    {
+        rError() << "Git Exception: " << what() << std::endl;
+    }
+
+    GitException(int gitErrorCode) :
+        runtime_error(GetLastErrorMessage())
+    {
+        rError() << "Git Exception: " << what() << std::endl;
+    }
+
+    static std::string GetLastErrorMessage()
+    {
+        auto error = git_error_last();
+
+        return error != nullptr ? error->message : "";
+    }
+
+    static void ThrowOnError(int errorCode)
+    {
+        if (errorCode == 0) return;
+
+        throw GitException(errorCode);
+    }
+};
+
+}
+
+}

plugins/vcs/Reference.h

@@ -57,7 +57,7 @@ class Reference final
     Ptr getUpstream() const
     {
         git_reference* upstream = nullptr;
-        git_branch_upstream(&upstream, _reference);
+        auto error = git_branch_upstream(&upstream, _reference);
 
         return upstream != nullptr ? std::make_shared<Reference>(upstream) : Ptr();
     }

plugins/vcs/Repository.cpp

@@ -6,6 +6,7 @@
 #include "Commit.h"
 #include "Tree.h"
 #include "Diff.h"
+#include "GitException.h"
 #include "os/path.h"
 
 namespace vcs
@@ -78,7 +79,8 @@ std::string Repository::getUpstreamRemoteName(const Reference& reference)
     git_buf buf;
     memset(&buf, 0, sizeof(git_buf));
 
-    git_branch_upstream_remote(&buf, _repository, reference.getName().c_str());
+    auto error = git_branch_upstream_remote(&buf, _repository, reference.getName().c_str());
+    GitException::ThrowOnError(error);
 
     std::string upstreamRemote = buf.ptr;
     git_buf_dispose(&buf);
@@ -100,6 +102,12 @@ void Repository::fetchFromTrackedRemote()
 
     rMessage() << head->getShorthandName() << " is set up to track " << (trackedBranch ? trackedBranch->getShorthandName() : "-") << std::endl;
 
+    if (!trackedBranch)
+    {
+        rWarning() << "No tracked remote branch configured, cannot fetch" << std::endl;
+        return;
+    }
+
     auto remoteName = getUpstreamRemoteName(*head);
     rMessage() << head->getShorthandName() << " is set up to track remote " << remoteName << std::endl;
 
@@ -120,6 +128,8 @@ RefSyncStatus Repository::getSyncStatusOfBranch(const Reference& reference)
 
     auto trackedBranch = reference.getUpstream();
 
+    if (!trackedBranch) throw GitException("No tracked branch, cannot check sync status");
+
     git_revwalk* walker;
     git_revwalk_new(&walker, _repository);
 
@@ -189,11 +199,12 @@ unsigned int Repository::getFileStatus(const std::string& relativePath)
 
     unsigned int statusFlags = 0;
 
-    git_status_foreach_ext(_repository, &options, [](const char* path, unsigned int flags, void* payload)
+    auto error = git_status_foreach_ext(_repository, &options, [](const char* path, unsigned int flags, void* payload)
     {
         *reinterpret_cast<unsigned int*>(payload) = flags;
         return 0;
     }, &statusFlags);
+    GitException::ThrowOnError(error);
 
     return statusFlags;
 }
@@ -211,40 +222,37 @@ bool Repository::fileHasUncommittedChanges(const std::string& relativePath)
 Commit::Ptr Repository::findMergeBase(const Reference& first, const Reference& second)
 {
     git_oid firstOid;
-    git_reference_name_to_id(&firstOid, _repository, first.getName().c_str());
+    auto error = git_reference_name_to_id(&firstOid, _repository, first.getName().c_str());
+    GitException::ThrowOnError(error);
 
     git_oid secondOid;
-    git_reference_name_to_id(&secondOid, _repository, second.getName().c_str());
+    error = git_reference_name_to_id(&secondOid, _repository, second.getName().c_str());
+    GitException::ThrowOnError(error);
 
     git_oid mergeBase;
-    if (git_merge_base(&mergeBase, _repository, &firstOid, &secondOid) == 0)
-    {
-        git_commit* commit;
-        git_commit_lookup(&commit, _repository, &mergeBase);
+    error = git_merge_base(&mergeBase, _repository, &firstOid, &secondOid);
+    GitException::ThrowOnError(error);
 
-        return std::make_shared<Commit>(commit);
-    }
+    git_commit* commit;
+    error = git_commit_lookup(&commit, _repository, &mergeBase);
+    GitException::ThrowOnError(error);
 
-    return Commit::Ptr();
+    return std::make_shared<Commit>(commit);
 }
 
 std::shared_ptr<Diff> Repository::getDiff(const Reference& ref, Commit& commit)
 {
     git_oid refOid;
-    git_reference_name_to_id(&refOid, _repository, ref.getName().c_str());
+    auto error = git_reference_name_to_id(&refOid, _repository, ref.getName().c_str());
+    GitException::ThrowOnError(error);
 
-    git_commit* refCommit;
-    git_commit_lookup(&refCommit, _repository, &refOid);
-
-    git_tree* refTree;
-    git_commit_tree(&refTree, refCommit);
+    auto refCommit = Commit::CreateFromOid(_repository, &refOid);
+    auto refTree = refCommit->getTree();
 
     git_diff* diff;
     auto baseTree = commit.getTree();
-    git_diff_tree_to_tree(&diff, _repository, baseTree->_get(), refTree, nullptr);
-
-    git_tree_free(refTree);
-    git_commit_free(refCommit);
+    error = git_diff_tree_to_tree(&diff, _repository, baseTree->_get(), refTree->_get(), nullptr);
+    GitException::ThrowOnError(error);
 
     return std::make_shared<Diff>(diff);
 }

plugins/vcs/ui/VcsStatus.cpp

@@ -13,6 +13,7 @@
 #include "os/path.h"
 #include "../GitModule.h"
 #include "../Diff.h"
+#include "../GitException.h"
 
 namespace vcs
 {
@@ -166,47 +167,47 @@ void VcsStatus::performFetch(std::shared_ptr<git::Repository> repository)
 
     GlobalUserInterface().dispatch([this]() { _text->SetLabel(_("Fetching...")); });
 
-    repository->fetchFromTrackedRemote();
-
-    std::lock_guard<std::mutex> guard(_taskLock);
-    _fetchInProgress = false;
-
-    auto status = repository->getSyncStatusOfBranch(*repository->getHead());
-
     std::string text;
 
-    if (status.localIsUpToDate)
-    {
-        text = _("Up to date");
-    }
-    else if (status.localCanBePushed)
+    try
     {
-        text = fmt::format(_("{0} to push"), status.localCommitsAhead);
-    }
-    else if (status.localCommitsAhead == 0)
-    {
-        text = fmt::format(_("{0} to integrate"), status.remoteCommitsAhead);
-    }
-    else
-    {
-        text = fmt::format(_("{0} to push, {1} to integrate"), status.localCommitsAhead, status.remoteCommitsAhead);
-    }
+        repository->fetchFromTrackedRemote();
 
-    if (status.remoteCommitsAhead > 0)
-    {
-        auto mapPath = getRepositoryRelativePath(GlobalMapModule().getMapName(), repository);
+        std::lock_guard<std::mutex> guard(_taskLock);
+        _fetchInProgress = false;
+
+        auto status = repository->getSyncStatusOfBranch(*repository->getHead());
 
-        if (!mapPath.empty())
+        if (status.localIsUpToDate)
+        {
+            text = _("Up to date");
+        }
+        else if (status.localCanBePushed)
+        {
+            text = fmt::format(_("{0} to push"), status.localCommitsAhead);
+        }
+        else if (status.localCommitsAhead == 0)
         {
-            // Check the incoming commits for modifications of the loaded map
-            auto head = repository->getHead();
-            auto upstream = head->getUpstream();
+            text = fmt::format(_("{0} to integrate"), status.remoteCommitsAhead);
+        }
+        else
+        {
+            text = fmt::format(_("{0} to push, {1} to integrate"), status.localCommitsAhead, status.remoteCommitsAhead);
+        }
 
-            // Find the merge base for this ref and its upstream
-            auto mergeBase = repository->findMergeBase(*head, *upstream);
+        if (status.remoteCommitsAhead > 0)
+        {
+            auto mapPath = getRepositoryRelativePath(GlobalMapModule().getMapName(), repository);
 
-            if (mergeBase)
+            if (!mapPath.empty())
             {
+                // Check the incoming commits for modifications of the loaded map
+                auto head = repository->getHead();
+                auto upstream = head->getUpstream();
+
+                // Find the merge base for this ref and its upstream
+                auto mergeBase = repository->findMergeBase(*head, *upstream);
+
                 auto diffAgainstBase = repository->getDiff(*upstream, *mergeBase);
 
                 if (diffAgainstBase->containsFile(mapPath))
@@ -218,12 +219,12 @@ void VcsStatus::performFetch(std::shared_ptr<git::Repository> repository)
                     text = fmt::format(_("{0} no conflicts"), status.remoteCommitsAhead);
                 }
             }
-            else
-            {
-                text = _("No merge base found");
-            }
         }
     }
+    catch (const git::GitException& ex)
+    {
+        text = ex.what();
+    }
 
     GlobalUserInterface().dispatch([this, text]() { _text->SetLabel(text); });
 }
@@ -254,31 +255,38 @@ void VcsStatus::performMapFileStatusCheck(std::shared_ptr<git::Repository> repos
 {
     setMapFileStatus(_("Checking map status..."));
 
-    if (GlobalMapModule().isUnnamed())
+    try
     {
-        setMapFileStatus(_("Map not saved yet"));
-        return;
-    }
+        if (GlobalMapModule().isUnnamed())
+        {
+            setMapFileStatus(_("Map not saved yet"));
+            return;
+        }
 
-    auto relativePath = getRepositoryRelativePath(GlobalMapModule().getMapName(), repository);
+        auto relativePath = getRepositoryRelativePath(GlobalMapModule().getMapName(), repository);
 
-    if (relativePath.empty())
-    {
-        setMapFileStatus(_("Map not in VCS"));
-        return;
-    }
+        if (relativePath.empty())
+        {
+            setMapFileStatus(_("Map not in VCS"));
+            return;
+        }
 
-    if (repository->fileHasUncommittedChanges(relativePath))
-    {
-        setMapFileStatus(_("Map saved, pending commit"));
-    }
-    else if (repository->fileIsIndexed(relativePath))
-    {
-        setMapFileStatus(_("Map committed"));
+        if (repository->fileHasUncommittedChanges(relativePath))
+        {
+            setMapFileStatus(_("Map saved, pending commit"));
+        }
+        else if (repository->fileIsIndexed(relativePath))
+        {
+            setMapFileStatus(_("Map committed"));
+        }
+        else
+        {
+            setMapFileStatus(_("Map saved"));
+        }
     }
-    else
+    catch (const git::GitException& ex)
     {
-        setMapFileStatus(_("Map saved"));
+        setMapFileStatus(std::string("ERROR: ") + ex.what());
     }
 }
 

tools/msvc/vcs.vcxproj

@@ -183,6 +183,7 @@
     <ClInclude Include="..\..\plugins\vcs\Commit.h" />
     <ClInclude Include="..\..\plugins\vcs\CredentialManager.h" />
     <ClInclude Include="..\..\plugins\vcs\Diff.h" />
+    <ClInclude Include="..\..\plugins\vcs\GitException.h" />
     <ClInclude Include="..\..\plugins\vcs\GitModule.h" />
     <ClInclude Include="..\..\plugins\vcs\Reference.h" />
     <ClInclude Include="..\..\plugins\vcs\Remote.h" />

tools/msvc/vcs.vcxproj.filters

@@ -47,5 +47,8 @@
     <ClInclude Include="..\..\plugins\vcs\Tree.h">
       <Filter>src</Filter>
     </ClInclude>
+    <ClInclude Include="..\..\plugins\vcs\GitException.h">
+      <Filter>src</Filter>
+    </ClInclude>
   </ItemGroup>
 </Project>