Simplify masterlist repository interaction code

- Refactor all direct libgit2 usage into GitHelper.
- Simplify the masterlist update logic, don't bother trying to merge branches or checkout an up-to-date local branch if the working copy has edits, just delete the branch and checkout a new copy from the remote.

src/api/helpers/git_helper.cpp

@@ -24,6 +24,9 @@
 
 #include "api/helpers/git_helper.h"
 
+#include <iomanip>
+#include <sstream>
+
 #include <boost/format.hpp>
 
 #include "api/helpers/logging.h"
@@ -87,15 +90,58 @@ GitHelper::GitData::~GitData() {
   git_tree_free(tree);
   git_diff_free(diff);
   git_buf_free(&buffer);
-
-  // Also free any path strings in the checkout options.
-  for (size_t i = 0; i < checkout_options.paths.count; ++i) {
-    delete[] checkout_options.paths.strings[i];
-  }
+  git_strarray_free(&checkout_options.paths);
 
   git_libgit2_shutdown();
 }
 
+void GitHelper::InitialiseOptions(const std::string& branch,
+                                  const std::string& filenameToCheckout) {
+  if (logger_) {
+    logger_->debug(
+        "Setting up checkout options uUsing branch {} and filename {}.",
+        branch,
+        filenameToCheckout);
+  }
+
+  data_.checkout_options.checkout_strategy =
+      GIT_CHECKOUT_FORCE | GIT_CHECKOUT_DONT_REMOVE_EXISTING;
+
+  char** paths = new char*[1];
+  paths[0] = new char[filenameToCheckout.length() + 1];
+  strcpy(paths[0], filenameToCheckout.c_str());
+
+  data_.checkout_options.paths.strings = paths;
+  data_.checkout_options.paths.count = 1;
+
+  // Initialise clone options.
+  data_.clone_options.checkout_opts = data_.checkout_options;
+  data_.clone_options.bare = 0;
+  data_.clone_options.checkout_branch = branch.c_str();
+}
+
+void GitHelper::Open(const boost::filesystem::path& repoRoot) {
+  if (logger_) {
+    logger_->info("Attempting to open Git repository at: {}",
+                  repoRoot.string());
+  }
+  Call(git_repository_open(&data_.repo, repoRoot.string().c_str()));
+}
+
+void GitHelper::SetRemoteUrl(const std::string& remote,
+                             const std::string& url) {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot set remote URL for repository that has not been opened.");
+  }
+
+  if (logger_) {
+    logger_->info("Setting URL for remote {} to {}", remote, url);
+  }
+
+  Call(git_remote_set_url(data_.repo, remote.c_str(), url.c_str()));
+}
+
 void GitHelper::Call(int error_code) {
   if (!error_code)
     return;
@@ -332,7 +378,140 @@ void GitHelper::CheckoutRevision(const std::string& revision) {
   data_.object = nullptr;
 }
 
-std::string GitHelper::GetHeadShortId() {
+void GitHelper::DeleteBranch(const std::string& branch) {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot delete branch for repository that has not been opened.");
+  } else if (data_.reference != nullptr) {
+    throw GitStateError(
+        "Cannot delete branch, reference memory already allocated.");
+  }
+
+  Call(git_branch_lookup(
+      &data_.reference, data_.repo, branch.c_str(), GIT_BRANCH_LOCAL));
+
+  int ret = git_branch_is_head(data_.reference);
+  if (ret == 1) {
+    if (logger_) {
+      logger_->debug("Detaching HEAD before deleting branch.");
+    }
+    git_repository_detach_head(data_.repo);
+  } else {
+    Call(ret);
+  }
+
+  if (logger_) {
+    logger_->debug("Deleting branch.");
+  }
+
+  Call(git_branch_delete(data_.reference));
+
+  git_reference_free(data_.reference);
+  data_.reference = nullptr;
+}
+
+bool GitHelper::BranchExists(const std::string& branch) {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence for repository that has not been "
+        "opened.");
+  } else if (data_.reference != nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence, reference memory already allocated.");
+  }
+
+  int ret = git_branch_lookup(
+      &data_.reference, data_.repo, branch.c_str(), GIT_BRANCH_LOCAL);
+  if (ret != GIT_ENOTFOUND) {
+    // Handle other errors from preceding branch lookup.
+    Call(ret);
+
+    git_reference_free(data_.reference);
+    data_.reference = nullptr;
+
+    return true;
+  }
+
+  return false;
+}
+
+bool GitHelper::IsBranchUpToDate(const std::string& branch) {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence for repository that has not been "
+        "opened.");
+  } else if (data_.reference != nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence, reference memory already allocated.");
+  } else if (data_.reference2 != nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence, reference2 memory already allocated.");
+  }
+
+  Call(git_branch_lookup(
+      &data_.reference, data_.repo, branch.c_str(), GIT_BRANCH_LOCAL));
+
+  // Get remote branch reference.
+  Call(git_branch_lookup(&data_.reference2,
+                         data_.repo,
+                         ("origin/" + branch).c_str(),
+                         GIT_BRANCH_REMOTE));
+
+  // Get the branch tips' commit IDs.
+  auto local_commit_id = GetCommitId(data_.reference);
+  auto remote_commit_id = GetCommitId(data_.reference2);
+
+  bool upToDate = memcmp(local_commit_id->id, remote_commit_id->id, 20) == 0;
+
+  // Free the remote branch reference.
+  git_reference_free(data_.reference2);
+  data_.reference2 = nullptr;
+
+  git_reference_free(data_.reference);
+  data_.reference = nullptr;
+
+  return upToDate;
+}
+
+bool GitHelper::IsBranchCheckedOut(const std::string& branch) {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence for repository that has not been "
+        "opened.");
+  } else if (data_.reference != nullptr) {
+    throw GitStateError(
+        "Cannot check branch existence, reference memory already allocated.");
+  }
+
+  Call(git_branch_lookup(
+      &data_.reference, data_.repo, branch.c_str(), GIT_BRANCH_LOCAL));
+
+  bool isCheckedOut = git_branch_is_checked_out(data_.reference);
+
+  git_reference_free(data_.reference);
+  data_.reference = nullptr;
+
+  return isCheckedOut;
+}
+
+const git_oid* GitHelper::GetCommitId(git_reference* reference) {
+  if (reference == nullptr)
+    throw GitStateError(
+        "Cannot get the commit ID of a null git_reference pointer.");
+  else if (data_.object != nullptr)
+    throw GitStateError(
+        "Cannot fetch repository updates, object memory already allocated.");
+
+  Call(git_reference_peel(&data_.object, reference, GIT_OBJ_COMMIT));
+  const git_oid* remote_commit_id = git_object_id(data_.object);
+
+  git_object_free(data_.object);
+  data_.object = nullptr;
+
+  return remote_commit_id;
+}
+
+std::string GitHelper::GetHeadCommitId(bool shortId) {
   if (data_.repo == nullptr)
     throw GitStateError(
         "Cannot checkout revision for repository that has not been opened.");
@@ -350,25 +529,70 @@ std::string GitHelper::GetHeadShortId() {
     logger_->trace("Getting the Git object for HEAD.");
   }
   Call(git_repository_head(&data_.reference, data_.repo));
-  Call(git_reference_peel(&data_.object, data_.reference, GIT_OBJ_COMMIT));
 
-  if (logger_) {
-    logger_->trace("Generating hex string for Git object ID.");
+  std::string id;
+  if (shortId) {
+    Call(git_reference_peel(&data_.object, data_.reference, GIT_OBJ_COMMIT));
+
+    if (logger_) {
+      logger_->trace("Generating hex string for Git object ID.");
+    }
+    Call(git_object_short_id(&data_.buffer, data_.object));
+    id = data_.buffer.ptr;
+
+    git_object_free(data_.object);
+    git_buf_free(&data_.buffer);
+    data_.object = nullptr;
+    data_.buffer = {0};
+  } else {
+    auto commit_id = GetCommitId(data_.reference);
+    char c_rev[GIT_OID_HEXSZ + 1];
+
+    id = git_oid_tostr(c_rev, GIT_OID_HEXSZ + 1, commit_id);
   }
-  Call(git_object_short_id(&data_.buffer, data_.object));
-  string revision = data_.buffer.ptr;
 
   git_reference_free(data_.reference);
-  git_object_free(data_.object);
-  git_buf_free(&data_.buffer);
   data_.reference = nullptr;
-  data_.object = nullptr;
-  data_.buffer = {0};
 
-  return revision;
+  return id;
 }
 
-GitHelper::GitData& GitHelper::GetData() { return data_; }
+std::string GitHelper::GetHeadCommitDate() {
+  if (data_.repo == nullptr) {
+    throw GitStateError(
+        "Cannot get HEAD commit date for repository that has not been opened.");
+  } else if (data_.commit != nullptr) {
+    throw GitStateError(
+        "Cannot get HEAD commit date, commit memory already allocated.");
+  } else if (data_.reference != nullptr) {
+    throw GitStateError(
+        "Cannot get HEAD commit date, reference memory already allocated.");
+  }
+
+  if (logger_) {
+    logger_->trace("Getting the Git reference for HEAD.");
+  }
+  Call(git_repository_head(&data_.reference, data_.repo));
+
+  auto commit_id = GetCommitId(data_.reference);
+
+  git_reference_free(data_.reference);
+  data_.reference = nullptr;
+
+  if (logger_) {
+    logger_->trace("Getting commit for ID.");
+  }
+
+  Call(git_commit_lookup(&data_.commit, data_.repo, commit_id));
+  git_time_t time = git_commit_time(data_.commit);
+
+  git_commit_free(data_.commit);
+  data_.commit = nullptr;
+
+  std::ostringstream out;
+  out << std::put_time(std::gmtime(&time), "%Y-%m-%d");
+  return out.str();
+}
 
 bool GitHelper::IsFileDifferent(const boost::filesystem::path& repoRoot,
                                 const std::string& filename) {

src/api/helpers/git_helper.h

@@ -34,6 +34,35 @@
 namespace loot {
 class GitHelper {
 public:
+  GitHelper();
+  ~GitHelper();
+
+  void InitialiseOptions(const std::string& branch,
+                         const std::string& filenameToCheckout);
+  void Open(const boost::filesystem::path& repoRoot);
+  void SetRemoteUrl(const std::string& remote, const std::string& url);
+
+  static bool IsRepository(const boost::filesystem::path& path);
+  static bool IsFileDifferent(const boost::filesystem::path& repoRoot,
+                              const std::string& filename);
+
+  void Clone(const boost::filesystem::path& path, const std::string& url);
+  void Fetch(const std::string& remote);
+
+  void CheckoutNewBranch(const std::string& remote, const std::string& branch);
+  void CheckoutRevision(const std::string& revision);
+
+  // Deletes the branch, detaching HEAD if it's currently set to the branch.
+  void DeleteBranch(const std::string& branch);
+
+  bool BranchExists(const std::string& branch);
+  bool IsBranchUpToDate(const std::string& branch);
+  bool IsBranchCheckedOut(const std::string& branch);
+
+  std::string GetHeadCommitId(bool shortId);
+  std::string GetHeadCommitDate();
+
+private:
   struct DiffPayload {
     bool fileFound;
     const char* fileToFind;
@@ -60,32 +89,18 @@ public:
     git_clone_options clone_options;
   };
 
-  GitHelper();
-  ~GitHelper();
-
-  void Call(int error_code);
-
-  static bool IsRepository(const boost::filesystem::path& path);
-  static bool IsFileDifferent(const boost::filesystem::path& repoRoot,
-                              const std::string& filename);
   static int DiffFileCallback(const git_diff_delta* delta,
                               float progress,
                               void* payload);
 
-  void Clone(const boost::filesystem::path& path, const std::string& url);
-  void Fetch(const std::string& remote);
-
-  void CheckoutNewBranch(const std::string& remote, const std::string& branch);
-  void CheckoutRevision(const std::string& revision);
-
-  std::string GetHeadShortId();
-  GitData& GetData();
-
-private:
   // Removes the read-only flag from some files in git repositories
   // created by libgit2.
   void FixRepoPermissions(const boost::filesystem::path& path);
 
+  void Call(int error_code);
+
+  const git_oid* GetCommitId(git_reference* reference);
+
   GitData data_;
   std::shared_ptr<spdlog::logger> logger_;
 };

src/api/masterlist.cpp

@@ -24,9 +24,6 @@
 
 #include "api/masterlist.h"
 
-#include <iomanip>
-#include <sstream>
-
 #include <boost/format.hpp>
 
 #include "api/game/game.h"
@@ -63,42 +60,10 @@ MasterlistInfo Masterlist::GetInfo(const boost::filesystem::path& path,
                         "\" is not a Git repository.");
   }
 
-  if (logger) {
-    logger->debug("Existing repository found, attempting to open it.");
-  }
-  git.Call(git_repository_open(&git.GetData().repo,
-                               path.parent_path().string().c_str()));
+  git.Open(path.parent_path());
 
-  // Need to get the HEAD object, because the individual file has a different
-  // SHA.
-  if (logger) {
-    logger->info("Getting the Git object for the tree at HEAD.");
-  }
-  git.Call(
-      git_revparse_single(&git.GetData().object, git.GetData().repo, "HEAD"));
-
-  if (logger) {
-    logger->trace("Generating hex string for Git object ID.");
-  }
-  if (shortID) {
-    git.Call(git_object_short_id(&git.GetData().buffer, git.GetData().object));
-    info.revision_id = git.GetData().buffer.ptr;
-  } else {
-    char c_rev[GIT_OID_HEXSZ + 1];
-    info.revision_id = git_oid_tostr(
-        c_rev, GIT_OID_HEXSZ + 1, git_object_id(git.GetData().object));
-  }
-
-  if (logger) {
-    logger->trace("Getting date for Git object.");
-  }
-  const git_oid* oid = git_object_id(git.GetData().object);
-  git.Call(git_commit_lookup(&git.GetData().commit, git.GetData().repo, oid));
-  git_time_t time = git_commit_time(git.GetData().commit);
-
-  std::ostringstream out;
-  out << std::put_time(std::gmtime(&time), "%Y-%m-%d");
-  info.revision_date = out.str();
+  info.revision_id = git.GetHeadCommitId(shortID);
+  info.revision_date = git.GetHeadCommitDate();
 
   if (logger) {
     logger->trace("Diffing masterlist HEAD and working copy.");
@@ -126,26 +91,12 @@ bool Masterlist::IsLatest(const boost::filesystem::path& path,
                         "\" is not a Git repository.");
   }
 
-  if (logger) {
-    logger->info("Attempting to open repository.");
-  }
-  git.Call(git_repository_open(&git.GetData().repo,
-                               path.parent_path().string().c_str()));
+  git.Open(path.parent_path());
 
   git.Fetch("origin");
 
-  // Get the remote branch's commit ID.
-  git_oid branchOid;
-  git.Call(git_reference_name_to_id(
-      &branchOid,
-      git.GetData().repo,
-      (string("refs/remotes/origin/") + repoBranch).c_str()));
-
-  // Get HEAD's commit ID.
-  git_oid headOid;
-  git.Call(git_reference_name_to_id(&headOid, git.GetData().repo, "HEAD"));
-
-  return memcmp(branchOid.id, headOid.id, 20) == 0;
+  return git.BranchExists(repoBranch) && git.IsBranchUpToDate(repoBranch) &&
+         git.IsBranchCheckedOut(repoBranch);
 }
 
 bool Masterlist::Update(const boost::filesystem::path& path,
@@ -159,223 +110,72 @@ bool Masterlist::Update(const boost::filesystem::path& path,
   if (repoUrl.empty() || repoBranch.empty())
     throw std::invalid_argument("Repository URL and branch must not be empty.");
 
-  // Initialise checkout options.
   if (logger) {
     logger->debug("Setting up checkout options.");
+    logger->debug("Using remote URL {} and branch {}", repoUrl, repoBranch);
   }
-  char* paths = new char[filename.length() + 1];
-  strcpy(paths, filename.c_str());
-  git.GetData().checkout_options.checkout_strategy =
-      GIT_CHECKOUT_FORCE | GIT_CHECKOUT_DONT_REMOVE_EXISTING;
-  git.GetData().checkout_options.paths.strings = &paths;
-  git.GetData().checkout_options.paths.count = 1;
-
-  // Initialise clone options.
-  git.GetData().clone_options.checkout_opts = git.GetData().checkout_options;
-  git.GetData().clone_options.bare = 0;
-  git.GetData().clone_options.checkout_branch = repoBranch.c_str();
+  git.InitialiseOptions(repoBranch, filename);
 
   // Now try to access the repository if it exists, or clone one if it doesn't.
   if (logger) {
-    logger->trace("Attempting to open the Git repository at: {}",
-                  repoPath.string());
+    logger->trace("Checking for Git repository at: {}", repoPath.string());
   }
-  if (!git.IsRepository(repoPath))
+  if (!GitHelper::IsRepository(repoPath)) {
     git.Clone(repoPath, repoUrl);
-  else {
-    // Repository exists: check settings are correct, then pull updates.
+  } else {
+    git.Open(repoPath);
 
-    // Open the repository.
-    if (logger) {
-      logger->info("Existing repository found, attempting to open it.");
-    }
-    git.Call(
-        git_repository_open(&git.GetData().repo, repoPath.string().c_str()));
-
-    // Set the remote URL.
-    if (logger) {
-      logger->info("Using remote URL: {}", repoUrl);
-    }
-    git.Call(git_remote_set_url(git.GetData().repo, "origin", repoUrl.c_str()));
+    // Set the remote URL. This assumes a single-URL remote called "origin"
+    // exists.
+    git.SetRemoteUrl("origin", repoUrl);
 
     // Now fetch updates from the remote.
     git.Fetch("origin");
 
-    // Check that a local branch with the correct name exists.
-    int ret = git_branch_lookup(&git.GetData().reference,
-                                git.GetData().repo,
-                                repoBranch.c_str(),
-                                GIT_BRANCH_LOCAL);
-    if (ret == GIT_ENOTFOUND)
-      // Branch doesn't exist. Create a new branch using the remote branch's
-      // latest commit.
-      git.CheckoutNewBranch("origin", repoBranch);
-    else {
-      // The local branch exists. Need to merge the remote branch
-      // into it.
-      git.Call(ret);  // Handle other errors from preceding branch lookup.
-
-      // Check if HEAD points to the desired branch and set it to if not.
-      if (!git_branch_is_head(git.GetData().reference)) {
-        if (logger) {
-          logger->trace("Setting HEAD to follow branch: {}", repoBranch);
-        }
-        git.Call(git_repository_set_head(
-            git.GetData().repo, (string("refs/heads/") + repoBranch).c_str()));
-      }
-
-      // Get remote branch reference.
-      git.Call(git_branch_upstream(&git.GetData().reference2,
-                                   git.GetData().reference));
-
-      if (logger) {
-        logger->trace("Checking HEAD and remote branch's mergeability.");
-      }
-      git_merge_analysis_t analysis;
-      git_merge_preference_t pref;
-      git.Call(git_annotated_commit_from_ref(&git.GetData().annotated_commit,
-                                             git.GetData().repo,
-                                             git.GetData().reference2));
-      git.Call(git_merge_analysis(
-          &analysis,
-          &pref,
-          git.GetData().repo,
-          (const git_annotated_commit**)&git.GetData().annotated_commit,
-          1));
-
-      if ((analysis & GIT_MERGE_ANALYSIS_FASTFORWARD) == 0 &&
-          (analysis & GIT_MERGE_ANALYSIS_UP_TO_DATE) == 0) {
-        // The local branch can't be easily merged. Best just to delete and
-        // recreate it.
-        if (logger) {
-          logger->trace(
-              "Local branch cannot be easily merged with remote branch.");
-        }
-
-        if (logger) {
-          logger->trace("Detaching HEAD so that the branch can be recreated.");
-        }
-        git.Call(git_repository_detach_head(git.GetData().repo));
-
-        // Need to free ref before calling git.CheckoutNewBranch()
-        git_reference_free(git.GetData().reference);
-        git.GetData().reference = nullptr;
-        git_reference_free(git.GetData().reference2);
-        git.GetData().reference2 = nullptr;
-
-        git.CheckoutNewBranch("origin", repoBranch);
-      } else {
-        // Get remote branch commit ID.
-        git.Call(git_reference_peel(
-            &git.GetData().object, git.GetData().reference2, GIT_OBJ_COMMIT));
-        const git_oid* remote_commit_id = git_object_id(git.GetData().object);
-
-        git_object_free(git.GetData().object);
-        git.GetData().object = nullptr;
-        git_reference_free(git.GetData().reference2);
-        git.GetData().reference2 = nullptr;
-
-        bool updateBranchHead = true;
-        if ((analysis & GIT_MERGE_ANALYSIS_UP_TO_DATE) != 0) {
-          // No merge is required, but HEAD might be ahead of the remote branch.
-          // Check to see if that's the case, and move HEAD back to match the
-          // remote branch if so.
-          if (logger) {
-            logger->trace(
-                "Local branch is up-to-date with remote branch. Checking to "
-                "see if local and remote branch heads are equal.");
-          }
-
-          // Get local branch commit ID.
-          git.Call(git_reference_peel(
-              &git.GetData().object, git.GetData().reference, GIT_OBJ_COMMIT));
-          const git_oid* local_commit_id = git_object_id(git.GetData().object);
-
-          git_object_free(git.GetData().object);
-          git.GetData().object = nullptr;
-
-          updateBranchHead = local_commit_id->id != remote_commit_id->id;
-
-          // If the masterlist in
-          // HEAD also matches the masterlist file, no further
-          // action needs to be taken. Otherwise, a checkout
-          // must be performed and the checked-out file parsed.
-          if (!updateBranchHead) {
-            if (logger) {
-              logger->trace("Local and remote branch heads are equal.");
-            }
-            if (!GitHelper::IsFileDifferent(repoPath, filename)) {
-              if (logger) {
-                logger->info(
-                    "Local branch and masterlist file are already up to date.");
-              }
-              return false;
-            }
-          } else if (logger) {
-            logger->trace("Local branch heads is ahead of remote branch head.");
-          }
-        } else if (logger) {
-          logger->trace("Local branch can be fast-forwarded to remote branch.");
-        }
-
-        if (updateBranchHead) {
-          // The remote branch reference points to a particular
-          // commit. Update the local branch reference to point
-          // to the same commit.
-          if (logger) {
-            logger->trace("Syncing local branch head with remote branch head.");
-          }
-          git.Call(git_reference_set_target(&git.GetData().reference2,
-                                            git.GetData().reference,
-                                            remote_commit_id,
-                                            "Setting branch reference."));
-
-          git_reference_free(git.GetData().reference2);
-          git.GetData().reference2 = nullptr;
-        }
-
-        git_reference_free(git.GetData().reference);
-        git.GetData().reference = nullptr;
-
-        if (logger) {
-          logger->trace("Performing a Git checkout of HEAD.");
-        }
-        git.Call(git_checkout_head(git.GetData().repo,
-                                   &git.GetData().checkout_options));
-      }
+    if (logger) {
+      logger->debug(
+          "Checking if branch {} is up to date and checked out without edits",
+          repoBranch);
     }
+
+    if (git.BranchExists(repoBranch)) {
+      if (git.IsBranchUpToDate(repoBranch) &&
+          git.IsBranchCheckedOut(repoBranch) &&
+          !GitHelper::IsFileDifferent(repoPath, filename)) {
+        if (logger) {
+          logger->info(
+              "Local branch and masterlist file are already up to date.");
+        }
+        return false;
+      }
+
+      git.DeleteBranch(repoBranch);
+    }
+
+    // No local branch exists, create and checkout a new one from the remote.
+    git.CheckoutNewBranch("origin", repoBranch);
   }
 
   // Now whether the repository was cloned or updated, the working directory
   // contains the latest masterlist. Try parsing it: on failure, detach the HEAD
   // back one commit and try again.
-
-  bool parsingFailed = false;
-  do {
-    // Get the HEAD revision's short ID.
-    string revision = git.GetHeadShortId();
-
-    // Now try parsing the masterlist.
-    if (logger) {
-      logger->debug("Testing masterlist parsing.");
-    }
+  while (true) {
     try {
       this->Load(path);
 
-      parsingFailed = false;
+      return true;
     } catch (std::exception& e) {
-      parsingFailed = true;
-
-      // There was an error, roll back one revision.
       if (logger) {
         logger->error("Masterlist parsing failed. Masterlist revision {}: {}",
-                      revision,
+                      git.GetHeadCommitId(true),
                       e.what());
       }
       git.CheckoutRevision("HEAD^");
     }
-  } while (parsingFailed);
+  }
 
+  // This should never be reached as git.CheckoutRevision() will throw if it
+  // tries to go one back from the start of history.
   return true;
 }
 }

src/tests/api/internals/helpers/git_helper_test.h

@@ -82,10 +82,6 @@ private:
   }
 };
 
-TEST_F(GitHelperTest, repoShouldInitialiseAsANullPointer) {
-  EXPECT_EQ(nullptr, git_.GetData().repo);
-}
-
 TEST_F(GitHelperTest, destructorShouldCallLibgit2CleanupFunction) {
   ASSERT_EQ(2, git_libgit2_init());
 
@@ -96,15 +92,6 @@ TEST_F(GitHelperTest, destructorShouldCallLibgit2CleanupFunction) {
   EXPECT_EQ(2, git_libgit2_shutdown());
 }
 
-TEST_F(GitHelperTest, callShouldNotThrowIfPassedAZeroValue) {
-  EXPECT_NO_THROW(git_.Call(0));
-}
-
-TEST_F(GitHelperTest, callShouldThrowIfPassedANonZeroValue) {
-  EXPECT_THROW(git_.Call(1), std::system_error);
-  EXPECT_THROW(git_.Call(-1), std::system_error);
-}
-
 TEST_F(GitHelperTest, isRepositoryShouldReturnTrueForARepositoryRoot) {
   EXPECT_TRUE(GitHelper::IsRepository(parentRepoRoot));
 }