Improve cycle avoidance involving groups earlier than default

src/api/sorting/plugin_sorter.cpp

@@ -394,6 +394,17 @@ bool shouldIgnorePlugin(
   return false;
 }
 
+bool shouldIgnoreGroupEdge(
+    const PluginSortingData& fromPlugin,
+    const PluginSortingData& toPlugin,
+    const std::map<std::string, std::unordered_set<std::string>>&
+        groupPluginsToIgnore) {
+  return shouldIgnorePlugin(
+             fromPlugin.GetGroup(), toPlugin.GetName(), groupPluginsToIgnore) ||
+         shouldIgnorePlugin(
+             toPlugin.GetGroup(), fromPlugin.GetName(), groupPluginsToIgnore);
+}
+
 void ignorePlugin(const std::string& pluginName,
                   const std::unordered_set<std::string>& groups,
                   std::map<std::string, std::unordered_set<std::string>>&
@@ -488,29 +499,38 @@ void PluginSorter::AddGroupEdges() {
     for (const auto& pluginName : graph_[vertex].GetAfterGroupPlugins()) {
       vertex_t parentVertex;
       if (GetVertexByName(pluginName, parentVertex)) {
-        bool ignore = shouldIgnorePlugin(graph_[vertex].GetGroup(),
-                                         graph_[parentVertex].GetName(),
-                                         groupPluginsToIgnore);
+        if (EdgeCreatesCycle(parentVertex, vertex)) {
+          auto& fromPlugin = graph_[parentVertex];
+          auto& toPlugin = graph_[vertex];
 
-        if (ignore || EdgeCreatesCycle(parentVertex, vertex)) {
           if (logger_) {
             logger_->trace(
                 "Skipping edge from \"{}\" to \"{}\" as it would "
                 "create a cycle.",
-                graph_[parentVertex].GetName(),
-                graph_[vertex].GetName());
+                fromPlugin.GetName(),
+                toPlugin.GetName());
           }
-          // Get the groups of the two plugins, and find all groups that are
-          // in paths that sit between those two groups. Group-derived edges
-          // from the plugin at parentVertex to any plugins in those groups
-          // should then be ignored as they also create cycles.
-          auto groupsInPaths = getGroupsInPaths(groups_,
-                                                graph_[parentVertex].GetGroup(),
-                                                graph_[vertex].GetGroup());
 
-          ignorePlugin(graph_[parentVertex].GetName(),
-                       groupsInPaths,
-                       groupPluginsToIgnore);
+          // The default group is a special case, as it's given to plugins
+          // with no metadata. If a plugin in the default group causes
+          // a cycle due to its group, ignore that plugin's group for all
+          // groups in the group graph paths between default and the other
+          // plugin's group.
+          std::string pluginToIgnore;
+          if (toPlugin.GetGroup() == Group().GetName()) {
+            pluginToIgnore = toPlugin.GetName();
+          } else if (fromPlugin.GetGroup() == Group().GetName()) {
+            pluginToIgnore = fromPlugin.GetName();
+          } else {
+            // If neither plugin is in the default group, it's impossible
+            // to decide which group to ignore, so ignore neither of them.
+            continue;
+          }
+
+          auto groupsInPaths = getGroupsInPaths(
+              groups_, fromPlugin.GetGroup(), toPlugin.GetGroup());
+
+          ignorePlugin(pluginToIgnore, groupsInPaths, groupPluginsToIgnore);
 
           continue;
         }
@@ -525,17 +545,19 @@ void PluginSorter::AddGroupEdges() {
         "Adding group edges that don't individually introduce cycles.");
   }
   for (const auto& edgePair : acyclicEdgePairs) {
-    bool ignore = shouldIgnorePlugin(graph_[edgePair.second].GetGroup(),
-                                     graph_[edgePair.first].GetName(),
-                                     groupPluginsToIgnore);
+    auto& fromPlugin = graph_[edgePair.first];
+    auto& toPlugin = graph_[edgePair.second];
+    bool ignore =
+        shouldIgnoreGroupEdge(fromPlugin, toPlugin, groupPluginsToIgnore);
+
     if (!ignore) {
       AddEdge(edgePair.first, edgePair.second);
     } else if (logger_) {
       logger_->trace(
           "Skipping edge from \"{}\" to \"{}\" as it would "
           "create a multi-group cycle.",
-          graph_[edgePair.first].GetName(),
-          graph_[edgePair.second].GetName());
+          fromPlugin.GetName(),
+          toPlugin.GetName());
     }
   }
 }

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

@@ -83,6 +83,13 @@ protected:
 
     boost::filesystem::ofstream masterlist(masterlistPath_);
     masterlist << "groups:" << endl
+               << "  - name: earliest" << endl
+               << "  - name: earlier" << endl
+               << "    after:" << endl
+               << "      - earliest" << endl
+               << "  - name: default" << endl
+               << "    after:" << endl
+               << "      - earlier" << endl
                << "  - name: group1" << endl
                << "  - name: group2" << endl
                << "    after:" << endl
@@ -248,27 +255,24 @@ TEST_P(PluginSorterTest,
 
 TEST_P(
     PluginSorterTest,
-    sortingShouldIgnoreGroupsThatContradictAnotherGroupInCombinationWithMoreSpecificMetadata) {
+    sortingShouldIgnoreGroupEdgesInvolvedInABackCycleOfAGroupEdgeFromADefaultGroupPlugin) {
   ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
 
   GenerateMasterlist();
   game_.GetDatabase()->LoadLists(masterlistPath_.string());
 
   PluginMetadata plugin(blankEsp);
-  plugin.SetGroup("group1");
-  game_.GetDatabase()->SetPluginUserMetadata(plugin);
 
   plugin = PluginMetadata(blankDifferentMasterDependentEsp);
-  plugin.SetGroup("group1");
   plugin.SetLoadAfterFiles(std::set<File>({File(blankMasterDependentEsp)}));
   game_.GetDatabase()->SetPluginUserMetadata(plugin);
 
   plugin = PluginMetadata(blankDifferentEsp);
-  plugin.SetGroup("group2");
+  plugin.SetGroup("group1");
   game_.GetDatabase()->SetPluginUserMetadata(plugin);
 
   plugin = PluginMetadata(blankMasterDependentEsp);
-  plugin.SetGroup("group3");
+  plugin.SetGroup("group2");
   game_.GetDatabase()->SetPluginUserMetadata(plugin);
 
   PluginSorter ps;
@@ -294,6 +298,69 @@ TEST_P(
   EXPECT_EQ(expectedSortedOrder, sorted);
 }
 
+TEST_P(
+    PluginSorterTest,
+    sortingShouldIgnoreGroupEdgesInvolvedInABackCycleOfAGroupEdgeToADefaultGroupPlugin) {
+  ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
+
+  GenerateMasterlist();
+  game_.GetDatabase()->LoadLists(masterlistPath_.string());
+
+  PluginMetadata plugin(blankMasterDependentEsm);
+  plugin.SetGroup("earliest");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankDifferentEsm);
+  plugin.SetGroup("earlier");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  PluginSorter ps;
+  std::vector<std::string> expectedSortedOrder({
+      blankEsm,
+      blankMasterDependentEsm,
+      blankDifferentEsm,
+      masterFile,
+      blankDifferentMasterDependentEsm,
+      blankEsp,
+      blankDifferentEsp,
+      blankMasterDependentEsp,
+      blankDifferentMasterDependentEsp,
+      blankPluginDependentEsp,
+      blankDifferentPluginDependentEsp,
+  });
+
+  if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) {
+    expectedSortedOrder.insert(expectedSortedOrder.begin() + 5, blankEsl);
+  }
+
+  std::vector<std::string> sorted = ps.Sort(game_);
+  EXPECT_EQ(expectedSortedOrder, sorted);
+}
+
+TEST_P(
+    PluginSorterTest,
+    sortingShouldThrowForAGroupEdgeThatCausesAMultiGroupCycleBetweenTwoNonDefaultGroups) {
+  ASSERT_NO_THROW(loadInstalledPlugins(game_, false));
+
+  GenerateMasterlist();
+  game_.GetDatabase()->LoadLists(masterlistPath_.string());
+
+  PluginMetadata plugin(blankMasterDependentEsm);
+  plugin.SetGroup("earliest");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankDifferentEsm);
+  plugin.SetGroup("earlier");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  plugin = PluginMetadata(blankEsm);
+  plugin.SetGroup("group4");
+  game_.GetDatabase()->SetPluginUserMetadata(plugin);
+
+  PluginSorter ps;
+  EXPECT_THROW(ps.Sort(game_), CyclicInteractionError);
+}
+
 TEST_P(
     PluginSorterTest,
     sortingShouldNotIgnorePluginsInTheSameGroupAsTheTargetPluginOfAGroupEdgeThatCausesACycleInIsolation) {