Patchwork [V2] parsers: fix invariant bug in find_deepest

login
register
mail settings
Submitter Sune Foldager
Date July 14, 2017, 11:49 a.m.
Message ID <21158392118c6ffd8c60.1500032949@firefly.edlund.dk>
Download mbox | patch
Permalink /patch/22317/
State Accepted
Headers show

Comments

Sune Foldager - July 14, 2017, 11:49 a.m.
# HG changeset patch
# User Sune Foldager <cryo@cyanite.org>
# Date 1500032897 -7200
#      Fri Jul 14 13:48:17 2017 +0200
# Branch stable
# Node ID 21158392118c6ffd8c60dd627a338f044726b9cb
# Parent  c1994c986d77f071e338de35a2c69d7bb87e39a3
parsers: fix invariant bug in find_deepest

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.
Augie Fackler - July 14, 2017, 4:51 p.m.
On Fri, Jul 14, 2017 at 01:49:09PM +0200, Sune Foldager wrote:
> # HG changeset patch
> # User Sune Foldager <cryo@cyanite.org>
> # Date 1500032897 -7200
> #      Fri Jul 14 13:48:17 2017 +0200
> # Branch stable
> # Node ID 21158392118c6ffd8c60dd627a338f044726b9cb
> # Parent  c1994c986d77f071e338de35a2c69d7bb87e39a3
> parsers: fix invariant bug in find_deepest

queued, thanks

(Though with a pretty big rebase and a commit message tweak for the
bugzilla bot)
Sune Foldager - July 14, 2017, 9:14 p.m.
Rebase? Didn’t I put in on stable? I realize the files are different on default, which is where I initially developed it.

As for the commit message change, I assume you mean the issue reference. Yeah, I didn’t think that would be correct ;). I’m out of the loop.

-Sune

> On 14 Jul 2017, at 18.51, Augie Fackler <raf@durin42.com> wrote:
> 
>> On Fri, Jul 14, 2017 at 01:49:09PM +0200, Sune Foldager wrote:
>> # HG changeset patch
>> # User Sune Foldager <cryo@cyanite.org>
>> # Date 1500032897 -7200
>> #      Fri Jul 14 13:48:17 2017 +0200
>> # Branch stable
>> # Node ID 21158392118c6ffd8c60dd627a338f044726b9cb
>> # Parent  c1994c986d77f071e338de35a2c69d7bb87e39a3
>> parsers: fix invariant bug in find_deepest
> 
> queued, thanks
> 
> (Though with a pretty big rebase and a commit message tweak for the
> bugzilla bot)
Augie Fackler - July 14, 2017, 9:15 p.m.
> On Jul 14, 2017, at 17:14, Sune Foldager <sune.foldager@me.com> wrote:
> 
> Rebase? Didn’t I put in on stable? I realize the files are different on default, which is where I initially developed it.  

Ah, you didn't have --flag stable and I missed the branch. Next release is 4.3 anyway, so it'll be there in any case.

> 
> As for the commit message change, I assume you mean the issue reference. Yeah, I didn’t think that would be correct ;). I’m out of the loop.
> 
> -Sune
> 
>> On 14 Jul 2017, at 18.51, Augie Fackler <raf@durin42.com> wrote:
>> 
>>> On Fri, Jul 14, 2017 at 01:49:09PM +0200, Sune Foldager wrote:
>>> # HG changeset patch
>>> # User Sune Foldager <cryo@cyanite.org>
>>> # Date 1500032897 -7200
>>> #      Fri Jul 14 13:48:17 2017 +0200
>>> # Branch stable
>>> # Node ID 21158392118c6ffd8c60dd627a338f044726b9cb
>>> # Parent  c1994c986d77f071e338de35a2c69d7bb87e39a3
>>> parsers: fix invariant bug in find_deepest
>> 
>> queued, thanks
>> 
>> (Though with a pretty big rebase and a commit message tweak for the
>> bugzilla bot)
Sune Foldager - July 14, 2017, 9:18 p.m.
Right, sorry about that. So you ended up un-rebasing the rebase I did before submitting :p. Always good to keep in shape, I suppose. Thanks. 

-Sune

> On 14 Jul 2017, at 23.15, Augie Fackler <raf@durin42.com> wrote:
> 
> 
>> On Jul 14, 2017, at 17:14, Sune Foldager <sune.foldager@me.com> wrote:
>> 
>> Rebase? Didn’t I put in on stable? I realize the files are different on default, which is where I initially developed it.  
> 
> Ah, you didn't have --flag stable and I missed the branch. Next release is 4.3 anyway, so it'll be there in any case.
> 
>> 
>> As for the commit message change, I assume you mean the issue reference. Yeah, I didn’t think that would be correct ;). I’m out of the loop.
>> 
>> -Sune
>> 
>>>> On 14 Jul 2017, at 18.51, Augie Fackler <raf@durin42.com> wrote:
>>>> 
>>>> On Fri, Jul 14, 2017 at 01:49:09PM +0200, Sune Foldager wrote:
>>>> # HG changeset patch
>>>> # User Sune Foldager <cryo@cyanite.org>
>>>> # Date 1500032897 -7200
>>>> #      Fri Jul 14 13:48:17 2017 +0200
>>>> # Branch stable
>>>> # Node ID 21158392118c6ffd8c60dd627a338f044726b9cb
>>>> # Parent  c1994c986d77f071e338de35a2c69d7bb87e39a3
>>>> parsers: fix invariant bug in find_deepest
>>> 
>>> queued, thanks
>>> 
>>> (Though with a pretty big rebase and a commit message tweak for the
>>> bugzilla bot)
>

Patch

diff -r c1994c986d77 -r 21158392118c mercurial/parsers.c
--- a/mercurial/parsers.c	Wed Jul 05 11:24:22 2017 -0400
+++ b/mercurial/parsers.c	Fri Jul 14 13:48:17 2017 +0200
@@ -2040,7 +2040,7 @@ 
 		goto bail;
 	}
 
-	interesting = calloc(sizeof(*interesting), 2 << revcount);
+	interesting = calloc(sizeof(*interesting), 1 << revcount);
 	if (interesting == NULL) {
 		PyErr_NoMemory();
 		goto bail;
@@ -2057,6 +2057,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--) {
@@ -2099,8 +2101,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;
 			}
 		}
diff -r c1994c986d77 -r 21158392118c tests/test-ancestor.py
--- a/tests/test-ancestor.py	Wed Jul 05 11:24:22 2017 -0400
+++ b/tests/test-ancestor.py	Fri Jul 14 13:48:17 2017 +0200
@@ -212,14 +212,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, 'gca%d' % i, create=1)
         cl = repo.changelog
         if not util.safehasattr(cl.index, 'ancestors'):
@@ -230,15 +232,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