Fix StackOverflowError in RevCommit.carryFlags on deep side graphs

Copying flags through a graph with deep side branches can cause
StackOverflowError. The recursive step to visit the 2nd parent of
a merge commit can overflow the stack if these are themselves very
deep histories with many branches.

Rewrite the loop to iterate up to 500 recursive steps deep before
unwinding the stack and running the remaining parts of the graph
using a dynamically allocated FIFORevQueue.

This approach still allows simple graphs that mostly merge short
lived topic branches into master to copy flags with no dynamic
memory allocation, relying only on temporary stack extensions.
Larger more complex graphs only pay the allocation penalities
if copying has to extend outwards "very far" in the graph, which
is unlikely for many coloring based algorithms.

Change-Id: I1882e6832c916e27dd5f6b7602d9caf66fb39c84

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
index 93d28aaf5a222181e14f9802d38037d28f010441..fac4f809f2ed6f44b74559c22d57593348024292 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
@@ -63,6 +63,8 @@ import org.eclipse.jgit.util.StringUtils;
 
 /** A commit reference to a commit in the DAG. */
 public class RevCommit extends RevObject {
+       private static final int STACK_DEPTH = 500;
+
        /**
         * Parse a commit from its canonical format.
         *
@@ -213,27 +215,72 @@ public class RevCommit extends RevObject {
                return Constants.OBJ_COMMIT;
        }
 
-       static void carryFlags(RevCommit c, final int carry) {
-               for (;;) {
-                       final RevCommit[] pList = c.parents;
-                       if (pList == null)
-                               return;
-                       final int n = pList.length;
-                       if (n == 0)
-                               return;
-
-                       for (int i = 1; i < n; i++) {
-                               final RevCommit p = pList[i];
-                               if ((p.flags & carry) == carry)
-                                       continue;
-                               p.flags |= carry;
-                               carryFlags(p, carry);
+       static void carryFlags(RevCommit c, int carry) {
+               FIFORevQueue q = carryFlags1(c, carry, 0);
+               if (q != null)
+                       slowCarryFlags(q, carry);
+       }
+
+       private static FIFORevQueue carryFlags1(RevCommit c, int carry, int depth) {
+               for(;;) {
+                       RevCommit[] pList = c.parents;
+                       if (pList == null || pList.length == 0)
+                               return null;
+                       if (pList.length != 1) {
+                               if (depth == STACK_DEPTH)
+                                       return defer(c);
+                               for (int i = 1; i < pList.length; i++) {
+                                       RevCommit p = pList[i];
+                                       if ((p.flags & carry) == carry)
+                                               continue;
+                                       p.flags |= carry;
+                                       FIFORevQueue q = carryFlags1(c, carry, depth + 1);
+                                       if (q != null)
+                                               return defer(q, carry, pList, i + 1);
+                               }
                        }
 
                        c = pList[0];
                        if ((c.flags & carry) == carry)
-                               return;
+                               return null;
+                       c.flags |= carry;
+               }
+       }
+
+       private static FIFORevQueue defer(RevCommit c) {
+               FIFORevQueue q = new FIFORevQueue();
+               q.add(c);
+               return q;
+       }
+
+       private static FIFORevQueue defer(FIFORevQueue q, int carry,
+                       RevCommit[] pList, int i) {
+               // In normal case the caller will run pList[0] in a tail recursive
+               // fashion by updating the variable. However the caller is unwinding
+               // the stack and will skip that pList[0] execution step.
+               carryOneStep(q, carry, pList[0]);
+
+               // Remaining parents (if any) need to have flags checked and be
+               // enqueued if they have ancestors.
+               for (; i < pList.length; i++)
+                       carryOneStep(q, carry, pList[i]);
+               return q;
+       }
+
+       private static void slowCarryFlags(FIFORevQueue q, int carry) {
+               // Commits in q have non-null parent arrays and have set all
+               // flags in carry. This loop finishes copying over the graph.
+               for (RevCommit c; (c = q.next()) != null;) {
+                       for (RevCommit p : c.parents)
+                               carryOneStep(q, carry, p);
+               }
+       }
+
+       private static void carryOneStep(FIFORevQueue q, int carry, RevCommit c) {
+               if ((c.flags & carry) != carry) {
                        c.flags |= carry;
+                       if (c.parents != null)
+                               q.add(c);
                }
        }