Bug #887

- Fixed circular dependency detection by implementing a basic DFS

Signed-off-by: Paul Beckingham <paul@beckingham.net>

AUTHORS

@@ -62,6 +62,7 @@ The following submitted code, packages or analysis, and deserve special thanks:
   Greg Grossmeier
   Barton Meeks
   Martin Klepsch
+  Wim Schuermann
 
 Thanks to the following, who submitted detailed bug reports and excellent
 suggestions:
@@ -105,7 +106,6 @@ suggestions:
   Jonathan Hankins
   Andreas Kalex
   Adam Wolk
-  Wim Schuermann
   Tom Duffy
   Miguel de Val Borro
   Yann Davin

src/dependency.cpp

@@ -25,12 +25,12 @@
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-
 #define L10N                                           // Localization complete.
 
 #include <algorithm>
 #include <iostream>
 #include <sstream>
+#include <stack>
 #include <Context.h>
 #include <text.h>
 #include <util.h>
@@ -39,9 +39,6 @@
 
 extern Context context;
 
-////////////////////////////////////////////////////////////////////////////////
-static bool followUpstream (const Task&, const Task&, std::vector <std::string>&);
-
 ////////////////////////////////////////////////////////////////////////////////
 // A task is blocked if it depends on tasks that are pending or waiting.
 //
@@ -116,49 +113,38 @@ void dependencyGetBlocking (const Task& task, std::vector <Task>& blocking)
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// Terminology:
-//   -->    if a depends on b, then it can be said that a --> b
-//   Head   if a --> b, then b is the head
-//   Tail   if a --> b, then a is the tail
-//
-// Algorithm:
-//   Keep walking the chain, recording the links (a --> b, b --> c, ...) until
-//   either the end of the chain is found (therefore not circular), or the chain
-//   loops and a repeat link is spotted (therefore circular).
+// Returns true if the supplied task adds a cycle to the dependency chain.
 bool dependencyIsCircular (const Task& task)
 {
-  std::vector <std::string> seen;
-  return followUpstream (task, task, seen);
-}
+  std::stack <Task> s;
+  std::vector <int> deps_current;
 
-////////////////////////////////////////////////////////////////////////////////
-// Returns true if a task is encountered twice in a chain walk, and therefore
-// indicates circularity.  Recursive.
-static bool followUpstream (
-  const Task& task,
-  const Task& original,
-  std::vector <std::string>& seen)
-{
-  std::vector <Task> blocking;
-  dependencyGetBlocking (task, blocking);
-  std::vector <Task>::iterator b;
-  for (b = blocking.begin (); b != blocking.end (); ++b)
+  std::string task_uuid = task.get ("uuid");
+
+  s.push (task);
+  while (!s.empty ())
   {
-    std::string link = task.get ("uuid") + " -> " + b->get ("uuid");
+    Task& current = s.top ();
+    current.getDependencies (deps_current);
 
-    // Have we seen this link before?  If so, circularity has been detected.
-    if (std::find (seen.begin (), seen.end (), link) != seen.end ())
+    // This is a basic depth first search that always terminates given the
+    // assumption that any cycles in the dependency graph must have been
+    // introduced by the task that is being checked.
+    // Since any previous cycles would have been prevented by this very
+    // function, this is a reasonable assumption.
+    for (unsigned int i = 0; i < deps_current.size (); i++)
+    {
+      context.tdb2.get (deps_current[i], current);
+
+      if (task_uuid == current.get ("uuid"))
+        {
+          // Cycle found, initial task reached for the second time!
           return true;
+        }
 
-    seen.push_back (link);
-
-    // Use 'original' over '*b' if they both refer to the same task, otherwise
-    // '*b' is from TDB2's committed list, and lacks recent modifications.
-    if (followUpstream (
-          (b->get ("uuid") == original.get ("uuid") ? original : *b),
-          original,
-          seen))
-      return true;
+      s.push (current);
+    }
+    s.pop ();
   }
 
   return false;