Patchwork [3,of,3,stable,V3] ancestor.deepest: ignore ninteresting while building result (issue3984)

login
register
mail settings
Submitter elson
Date July 26, 2013, 3:25 a.m.
Message ID <CA+jbiHH5Wg6J73kaP71BoWnAq8ixbKYDGUz7OD+8RfovM3+E0A@mail.gmail.com>
Download mbox | patch
Permalink /patch/1974/
State Accepted
Commit 2fa303619b4d33b12465829bafa181ea2e0f2ab5
Headers show

Comments

elson - July 26, 2013, 3:25 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1374788595 25200
#      Thu Jul 25 14:43:15 2013 -0700
# Branch stable
# Node ID a0c9e0bda2228899bb21f3fe56b626379798ec41
# Parent  f2dfda6ac1520ec241f35b636af5d1198a77d611
ancestor.deepest: ignore ninteresting while building result (issue3984)

ninteresting indicates the number of non-zero elements in the interesting
array, not the number of elements in the final list. Since elements in
interesting can stand for more than one gca, limiting the number of results
to
ninteresting is an error.

Tests for issue3984 are included.

 if __name__ == '__main__':
     test_missingancestors()
     test_lazyancestors()



2013/7/26 elson <elson.wei+hg@gmail.com>

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1374788595 25200
> #      Thu Jul 25 14:43:15 2013 -0700
> # Branch stable
> # Node ID 2c7781fc506ba1946d262b19b3c378dac6116e75
> # Parent  f2dfda6ac1520ec241f35b636af5d1198a77d611
>
> ancestor.deepest: ignore ninteresting while building result (issue3984)
>
> ninteresting indicates the number of non-zero elements in the interesting
> array, not the number of elements in the final list. Since elements in
> interesting can stand for more than one gca, limiting the number of
> results to
> ninteresting is an error.
>
> Tests for issue3984 are included.
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1388,8 +1388,7 @@
>      if (dict == NULL)
>          goto bail;
>
> -    j = ninteresting;
> -    for (i = 0; i < revcount && j > 0; i++) {
> +    for (i = 0; i < revcount; i++) {
>          PyObject *key;
>
>          if ((final & (1 << i)) == 0)
> @@ -1403,7 +1402,6 @@
>              Py_DECREF(Py_None);
>              goto bail;
>          }
> -        j -= 1;
>      }
>
>      keys = PyDict_Keys(dict);
> diff --git a/tests/test-ancestor.py b/tests/test-ancestor.py
> --- a/tests/test-ancestor.py
> +++ b/tests/test-ancestor.py
> @@ -1,4 +1,4 @@
> -from mercurial import ancestor
> +from mercurial import ancestor, commands, hg, ui, util
>
>  # graph is a dict of child->parent adjacency lists for this graph:
>  # o  13
> @@ -101,6 +101,34 @@
>
>      s = genlazyancestors([11, 13], stoprev=6, inclusive=True)
>      printlazyancestors(s, [11, 13, 7, 9, 8, 3, 6, 4, 1, -1, 0])
>
> +# The C gca algorithm requires a real repo. These are textual
> descriptions of
> +# dags that have been known to be problematic.
> +dagtests = [
> +    '+2*2*2/*3/2',
> +    '+3*3/*2*2/*4*4/*4/2*4/2*2',
> +]
> +def test_gca():
> +    u = ui.ui()
> +    for i, dag in enumerate(dagtests):
> +        repo = hg.repository(u, 'gca%d' % i, create=1)
> +        cl = repo.changelog
> +        if not hasattr(cl.index, 'ancestors'):
>
> +            # C version not available
> +            return
> +
> +        commands.debugbuilddag(u, repo, dag)
> +        # 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.
> +        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:
> +                    print "test_gca: for dag %s, gcas for %d, %d:" %
> (dag, a, b)
> +                    print "  C returned:      %s" % cgcas
> +                    print "  Python returned: %s" % pygcas
> +
>  if __name__ == '__main__':
>      test_missingancestors()
>      test_lazyancestors()
>
>
Siddharth Agarwal - July 26, 2013, 6:40 a.m.
On 07/25/2013 08:25 PM, elson wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com <mailto:sid0@fb.com>>
> # Date 1374788595 25200
> #      Thu Jul 25 14:43:15 2013 -0700
> # Branch stable
> # Node ID a0c9e0bda2228899bb21f3fe56b626379798ec41
> # Parent  f2dfda6ac1520ec241f35b636af5d1198a77d611
> ancestor.deepest: ignore ninteresting while building result (issue3984)

I don't know why you resent these patches, but this one's bogus since it 
doesn't include the last hunk. There's no need to send the patches again 
to fix this, since the series I sent should apply.
elson - July 26, 2013, 7:14 a.m.
Just append *.diff files.
It's more convenient to import to the repo


2013/7/26 Siddharth Agarwal <sid0@fb.com>

> On 07/25/2013 08:25 PM, elson wrote:
>
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com <mailto:sid0@fb.com>>
>>
>> # Date 1374788595 25200
>> #      Thu Jul 25 14:43:15 2013 -0700
>> # Branch stable
>> # Node ID a0c9e0bda2228899bb21f3fe56b626**379798ec41
>> # Parent  f2dfda6ac1520ec241f35b636af5d1**198a77d611
>> ancestor.deepest: ignore ninteresting while building result (issue3984)
>>
>
> I don't know why you resent these patches, but this one's bogus since it
> doesn't include the last hunk. There's no need to send the patches again to
> fix this, since the series I sent should apply.
>
Christian Ebert - July 26, 2013, 9:48 a.m.
* elson on Friday, July 26, 2013 at 15:14:52 +0800
> Just append *.diff files.

Quote 
http://mercurial.selenic.com/wiki/ContributingChanges#Emailing_patches

"We prefer patches *in the message body* so we can review them (no
attachments or URLs!)"
Chingis Dugarzhapov - July 26, 2013, 11:44 a.m.
>
> Quote
> http://mercurial.selenic.com/wiki/ContributingChanges#Emailing_patches
>

I prefer both, body + attachment.

@Siddharth, patch is OK for me

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1388,8 +1388,7 @@ 
     if (dict == NULL)
         goto bail;

-    j = ninteresting;
-    for (i = 0; i < revcount && j > 0; i++) {
+    for (i = 0; i < revcount; i++) {
         PyObject *key;

         if ((final & (1 << i)) == 0)
@@ -1403,7 +1402,6 @@ 
             Py_DECREF(Py_None);
             goto bail;
         }
-        j -= 1;
     }

     keys = PyDict_Keys(dict);
diff --git a/tests/test-ancestor.py b/tests/test-ancestor.py
--- a/tests/test-ancestor.py
+++ b/tests/test-ancestor.py
@@ -1,4 +1,4 @@ 
-from mercurial import ancestor
+from mercurial import ancestor, commands, hg, ui, util

 # graph is a dict of child->parent adjacency lists for this graph:
 # o  13
@@ -101,6 +101,34 @@ 
     s = genlazyancestors([11, 13], stoprev=6, inclusive=True)
     printlazyancestors(s, [11, 13, 7, 9, 8, 3, 6, 4, 1, -1, 0])

+# The C gca algorithm requires a real repo. These are textual descriptions
of
+# dags that have been known to be problematic.
+dagtests = [
+    '+2*2*2/*3/2',
+    '+3*3/*2*2/*4*4/*4/2*4/2*2',
+]
+def test_gca():
+    u = ui.ui()
+    for i, dag in enumerate(dagtests):
+        repo = hg.repository(u, 'gca%d' % i, create=1)
+        cl = repo.changelog
+        if not util.safehasattr(cl.index, 'ancestors'):
+            # C version not available
+            return
+
+        commands.debugbuilddag(u, repo, dag)
+        # 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.
+        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:
+                    print "test_gca: for dag %s, gcas for %d, %d:" % (dag,
a, b)
+                    print "  C returned:      %s" % cgcas
+                    print "  Python returned: %s" % pygcas
+