Fix avoidance of group cycles involving user metadata

This bug was introduced in 53e2dbba1f,
and affects libloot v0.19.1.

src/api/api_database.cpp

@@ -139,38 +139,11 @@ std::vector<Message> ApiDatabase::GetGeneralMessages(
 }
 
 std::vector<Group> ApiDatabase::GetGroups(bool includeUserMetadata) const {
-  auto groups = masterlist_.Groups();
-
   if (includeUserMetadata) {
-    std::vector<Group> newGroups;
-    for (const auto& userlistGroup : userlist_.Groups()) {
-      auto groupIt = std::find_if(
-          groups.begin(), groups.end(), [&](const Group& existingGroup) {
-            return existingGroup.GetName() == userlistGroup.GetName();
-          });
-
-      if (groupIt == groups.end()) {
-        newGroups.push_back(userlistGroup);
-      } else {
-        // Replace the masterlist group description with the userlist group
-        // description if the latter is not empty.
-        auto description = userlistGroup.GetDescription().empty()
-                               ? groupIt->GetDescription()
-                               : userlistGroup.GetDescription();
-
-        auto afterGroups = groupIt->GetAfterGroups();
-        auto userAfterGroups = userlistGroup.GetAfterGroups();
-        afterGroups.insert(
-            afterGroups.end(), userAfterGroups.begin(), userAfterGroups.end());
-
-        *groupIt = Group(userlistGroup.GetName(), afterGroups, description);
-      }
-    }
-
-    groups.insert(groups.end(), newGroups.cbegin(), newGroups.cend());
+    return MergeGroups(masterlist_.Groups(), userlist_.Groups());
   }
 
-  return groups;
+  return masterlist_.Groups();
 }
 
 std::vector<Group> ApiDatabase::GetUserGroups() const {

src/api/sorting/group_sort.cpp

@@ -327,4 +327,40 @@ std::vector<Vertex> GetGroupsPath(const std::vector<Group>& masterlistGroups,
 
   return path;
 }
+
+std::vector<Group> MergeGroups(const std::vector<Group>& masterlistGroups,
+                               const std::vector<Group>& userGroups) {
+  auto mergedGroups = masterlistGroups;
+
+  std::vector<Group> newGroups;
+  for (const auto& userGroup : userGroups) {
+    auto groupIt =
+        std::find_if(mergedGroups.begin(),
+                     mergedGroups.end(),
+                     [&](const Group& existingGroup) {
+                       return existingGroup.GetName() == userGroup.GetName();
+                     });
+
+    if (groupIt == mergedGroups.end()) {
+      newGroups.push_back(userGroup);
+    } else {
+      // Replace the masterlist group description with the userlist group
+      // description if the latter is not empty.
+      auto description = userGroup.GetDescription().empty()
+                             ? groupIt->GetDescription()
+                             : userGroup.GetDescription();
+
+      auto afterGroups = groupIt->GetAfterGroups();
+      auto userAfterGroups = userGroup.GetAfterGroups();
+      afterGroups.insert(
+          afterGroups.end(), userAfterGroups.begin(), userAfterGroups.end());
+
+      *groupIt = Group(userGroup.GetName(), afterGroups, description);
+    }
+  }
+
+  mergedGroups.insert(mergedGroups.end(), newGroups.cbegin(), newGroups.cend());
+
+  return mergedGroups;
+}
 }

src/api/sorting/group_sort.h

@@ -47,5 +47,8 @@ std::vector<Vertex> GetGroupsPath(const std::vector<Group>& masterlistGroups,
                                   const std::vector<Group>& userGroups,
                                   const std::string& fromGroupName,
                                   const std::string& toGroupName);
+
+std::vector<Group> MergeGroups(const std::vector<Group>& masterlistGroups,
+                               const std::vector<Group>& userGroups);
 }
 #endif

src/api/sorting/plugin_sort.cpp

@@ -90,11 +90,10 @@ std::vector<std::string> GetPluginsWithHardcodedPositions(
 std::unordered_map<std::string, Group> GetGroupsMap(
     const std::vector<Group> masterlistGroups,
     const std::vector<Group> userGroups) {
+  const auto mergedGroups = MergeGroups(masterlistGroups, userGroups);
+
   std::unordered_map<std::string, Group> groupsMap;
-  for (const auto& group : masterlistGroups) {
-    groupsMap.emplace(group.GetName(), group);
-  }
-  for (const auto& group : userGroups) {
+  for (const auto& group : mergedGroups) {
     groupsMap.emplace(group.GetName(), group);
   }
 

src/tests/api/internals/sorting/plugin_sort_test.h

@@ -266,6 +266,60 @@ TEST_P(PluginSortTest, sortingShouldResolveGroupsAsTransitiveLoadAfterSets) {
   EXPECT_EQ(expectedSortedOrder, sorted);
 }
 
+TEST_P(PluginSortTest,
+       sortingShouldAccountForUserGroupMetadataWhenTryingToAvoidCycles) {
+  using std::endl;
+
+  ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
+
+  const auto masterlistPath = metadataFilesPath / "masterlist.yaml";
+  std::ofstream masterlist(masterlistPath);
+  masterlist << "groups:" << endl
+             << "  - name: default" << endl
+             << "  - name: B" << endl
+             << "    after:" << endl
+             << "      - default" << endl;
+  masterlist.close();
+
+  game_.GetDatabase().LoadLists(masterlistPath);
+
+  game_.GetDatabase().SetUserGroups(
+      {Group("A", {"default"}), Group("B", {"A"})});
+
+  PluginMetadata plugin(blankEsm);
+  plugin.SetGroup("B");
+  game_.GetDatabase().SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankMasterDependentEsm);
+  plugin.SetGroup("default");
+  game_.GetDatabase().SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankDifferentEsm);
+  plugin.SetGroup("A");
+  game_.GetDatabase().SetPluginUserMetadata(plugin);
+
+  std::vector<std::string> expectedSortedOrder({
+      masterFile,
+      blankDifferentEsm,
+      blankDifferentMasterDependentEsm,
+      blankEsm,
+      blankMasterDependentEsm,
+      blankEsp,
+      blankDifferentEsp,
+      blankMasterDependentEsp,
+      blankDifferentMasterDependentEsp,
+      blankPluginDependentEsp,
+      blankDifferentPluginDependentEsp,
+  });
+
+  if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) {
+    expectedSortedOrder.insert(expectedSortedOrder.begin() + 1, blankEsl);
+  }
+
+  std::vector<std::string> sorted = SortPlugins(game_, game_.GetLoadOrder());
+  EXPECT_EQ(expectedSortedOrder, sorted);
+}
+
 TEST_P(PluginSortTest, sortingShouldThrowIfAPluginHasAGroupThatDoesNotExist) {
   ASSERT_NO_THROW(loadInstalledPlugins(game_, false));