parsers: fix invariant bug in find_deepest (issue5623)
authorSune Foldager <cryo@cyanite.org>
Fri, 14 Jul 2017 13:48:17 +0200
changeset 33475 f501322512b6
parent 33474 c514b4fb5e27
child 33476 c0d8de2724ce
parsers: fix invariant bug in find_deepest (issue5623) find_deepest is used to find the "best" ancestors given a list. In the main loop it keeps an invariant called 'ninteresting' which is supposed to contain the number of non-zero entries in the 'interesting' array. This invariant is incorrectly maintained, however, which leads the the algorithm returning an empty result for certain graphs. This has been fixed. Also, the 'interesting' array is supposed to fit 2^ancestors values, but is incorrectly allocated to twice that size. This has been fixed as well. The tests in test-ancestor.py compare the Python and C versions of the code, and report the error correctly, since the Python version works correct. Even so, I have added an additional test against the expected result, in the event that both algorithms have an identical error in the future. This fixes issue5623.
mercurial/cext/revlog.c
tests/test-ancestor.py
--- a/mercurial/cext/revlog.c	Fri Jun 30 03:45:57 2017 +0200
+++ b/mercurial/cext/revlog.c	Fri Jul 14 13:48:17 2017 +0200
@@ -1464,7 +1464,7 @@
 		goto bail;
 	}
 
-	interesting = calloc(sizeof(*interesting), 2 << revcount);
+	interesting = calloc(sizeof(*interesting), 1 << revcount);
 	if (interesting == NULL) {
 		PyErr_NoMemory();
 		goto bail;
@@ -1481,6 +1481,8 @@
 		interesting[b] = 1;
 	}
 
+	/* invariant: ninteresting is the number of non-zero entries in
+	 * interesting. */
 	ninteresting = (int)revcount;
 
 	for (v = maxrev; v >= 0 && ninteresting > 1; v--) {
@@ -1523,8 +1525,10 @@
 					continue;
 				seen[p] = nsp;
 				interesting[sp] -= 1;
-				if (interesting[sp] == 0 && interesting[nsp] > 0)
+				if (interesting[sp] == 0)
 					ninteresting -= 1;
+				if (interesting[nsp] == 0)
+					ninteresting += 1;
 				interesting[nsp] += 1;
 			}
 		}
--- a/tests/test-ancestor.py	Fri Jun 30 03:45:57 2017 +0200
+++ b/tests/test-ancestor.py	Fri Jul 14 13:48:17 2017 +0200
@@ -217,14 +217,16 @@
 
 
 # The C gca algorithm requires a real repo. These are textual descriptions of
-# DAGs that have been known to be problematic.
+# DAGs that have been known to be problematic, and, optionally, known pairs
+# of revisions and their expected ancestor list.
 dagtests = [
-    '+2*2*2/*3/2',
-    '+3*3/*2*2/*4*4/*4/2*4/2*2',
+    ('+2*2*2/*3/2', {}),
+    ('+3*3/*2*2/*4*4/*4/2*4/2*2', {}),
+    ('+2*2*/2*4*/4*/3*2/4', {(6, 7): [3, 5]}),
 ]
 def test_gca():
     u = uimod.ui.load()
-    for i, dag in enumerate(dagtests):
+    for i, (dag, tests) in enumerate(dagtests):
         repo = hg.repository(u, b'gca%d' % i, create=1)
         cl = repo.changelog
         if not util.safehasattr(cl.index, 'ancestors'):
@@ -235,15 +237,21 @@
         # Compare the results of the Python and C versions. This does not
         # include choosing a winner when more than one gca exists -- we make
         # sure both return exactly the same set of gcas.
+        # Also compare against expected results, if available.
         for a in cl:
             for b in cl:
                 cgcas = sorted(cl.index.ancestors(a, b))
                 pygcas = sorted(ancestor.ancestors(cl.parentrevs, a, b))
-                if cgcas != pygcas:
+                expected = None
+                if (a, b) in tests:
+                    expected = tests[(a, b)]
+                if cgcas != pygcas or (expected and cgcas != expected):
                     print("test_gca: for dag %s, gcas for %d, %d:"
                           % (dag, a, b))
                     print("  C returned:      %s" % cgcas)
                     print("  Python returned: %s" % pygcas)
+                    if expected:
+                        print("  expected:        %s" % expected)
 
 def main():
     seed = None