From be30bc500f8b65d66f96856af183af9d31d399cd Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 1 Aug 2021 13:32:23 +0100 Subject: [PATCH] Remove masterlist update loading and rollback LOOT's ability to roll back to a working masterlist version hasn't been exercised in years, as our CI testing has gotten better at catching issues before they reach users. Coupling updating a masterlist with loading it also complicates the introduction of a common prelude, so given the very low usefulness of the functionality it's best to drop it entirely . --- include/loot/database_interface.h | 6 ++---- src/api/api_database.cpp | 8 +------- src/api/helpers/git_helper.cpp | 27 --------------------------- src/api/helpers/git_helper.h | 1 - src/api/masterlist.cpp | 20 +------------------- src/api/masterlist.h | 2 +- 6 files changed, 5 insertions(+), 59 deletions(-) diff --git a/include/loot/database_interface.h b/include/loot/database_interface.h index 4d18175e..e1905545 100644 --- a/include/loot/database_interface.h +++ b/include/loot/database_interface.h @@ -95,8 +95,7 @@ public: * @details Uses Git to update the given masterlist to a given remote. * If the masterlist doesn't exist, this will create it. This * function also initialises a Git repository in the given - * masterlist's parent folder. If the masterlist was not already - * up-to-date, it will be re-loaded, but not re-evaluated. + * masterlist's parent folder. * * If a Git repository is already present, it will be used to * perform a diff-only update, but if for any reason a @@ -120,8 +119,7 @@ public: * check their repositories to see which is the latest release branch. * @returns `true` if the masterlist was updated. `false` if no update was * necessary, ie. it was already up-to-date. If `true`, the - * masterlist will have been re-loaded, but will need to be - * re-evaluated separately. + * masterlist will need to be re-loaded and re-evaluated separately. */ virtual bool UpdateMasterlist(const std::filesystem::path& masterlist_path, const std::string& remote_url, diff --git a/src/api/api_database.cpp b/src/api/api_database.cpp index 805416a3..97f536cb 100644 --- a/src/api/api_database.cpp +++ b/src/api/api_database.cpp @@ -95,13 +95,7 @@ bool ApiDatabase::UpdateMasterlist(const std::filesystem::path& masterlistPath, masterlistPath.u8string() + "\" does not have a valid parent directory."); - Masterlist masterlist; - if (masterlist.Update(masterlistPath, remoteURL, remoteBranch)) { - masterlist_ = masterlist; - return true; - } - - return false; + return Masterlist::Update(masterlistPath, remoteURL, remoteBranch); } MasterlistInfo ApiDatabase::GetMasterlistRevision( diff --git a/src/api/helpers/git_helper.cpp b/src/api/helpers/git_helper.cpp index 06150213..dfd7bfc7 100644 --- a/src/api/helpers/git_helper.cpp +++ b/src/api/helpers/git_helper.cpp @@ -346,33 +346,6 @@ void GitHelper::CheckoutNewBranch(const std::string& remote, data_.reference = nullptr; } -void GitHelper::CheckoutRevision(const std::string& revision) { - if (data_.repo == nullptr) - throw GitStateError( - "Cannot checkout revision for repository that has not been opened."); - else if (data_.object != nullptr) - throw GitStateError( - "Cannot fetch repository updates, object memory already allocated."); - - // Get an object ID for 'HEAD^'. - Call(git_revparse_single(&data_.object, data_.repo, revision.c_str())); - const git_oid* oid = git_object_id(data_.object); - - // Detach HEAD to HEAD~1. This will roll back HEAD by one commit each time it - // is called. - Call(git_repository_set_head_detached(data_.repo, oid)); - - // Checkout the new HEAD. - auto logger = getLogger(); - if (logger) { - logger->trace("Performing a Git checkout of HEAD."); - } - Call(git_checkout_head(data_.repo, &data_.checkout_options)); - - git_object_free(data_.object); - data_.object = nullptr; -} - void GitHelper::DeleteBranch(const std::string& branch) { if (data_.repo == nullptr) { throw GitStateError( diff --git a/src/api/helpers/git_helper.h b/src/api/helpers/git_helper.h index 2a5d43e4..14650b27 100644 --- a/src/api/helpers/git_helper.h +++ b/src/api/helpers/git_helper.h @@ -47,7 +47,6 @@ public: 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); diff --git a/src/api/masterlist.cpp b/src/api/masterlist.cpp index 4456d039..02fd76d2 100644 --- a/src/api/masterlist.cpp +++ b/src/api/masterlist.cpp @@ -150,25 +150,7 @@ bool Masterlist::Update(const std::filesystem::path& path, } // 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. - while (true) { - try { - this->Load(path); - - return true; - } catch (std::exception& e) { - if (logger) { - logger->error("Masterlist parsing failed. Masterlist revision {}: {}", - git.GetHeadCommitId(true), - e.what()); - } - git.CheckoutRevision("HEAD^"); - } - } - - // This should never be reached as git.CheckoutRevision() will throw if it - // tries to go one back from the start of history. + // contains the latest masterlist. return true; } } diff --git a/src/api/masterlist.h b/src/api/masterlist.h index 78aa477b..63821f09 100644 --- a/src/api/masterlist.h +++ b/src/api/masterlist.h @@ -34,7 +34,7 @@ namespace loot { class Masterlist : public MetadataList { public: - bool Update(const std::filesystem::path& path, + static bool Update(const std::filesystem::path& path, const std::string& repoURL, const std::string& repoBranch);