Patchwork [3,of,3,STABLE] rust-cpython: raising error.WdirUnsupported

login
register
mail settings
Submitter Georges Racinet
Date Jan. 24, 2019, 4:23 a.m.
Message ID <47d5458a4c32e20de4af.1548303833@ishtar>
Download mbox | patch
Permalink /patch/37953/
State Accepted
Headers show

Comments

Georges Racinet - Jan. 24, 2019, 4:23 a.m.
# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1548247776 18000
#      Wed Jan 23 07:49:36 2019 -0500
# Branch stable
# Node ID 47d5458a4c32e20de4af1237cfeb157153586cbc
# Parent  a35cfd592a90ae325b452c56fe8bff86cac097dd
# EXP-Topic rust-wdirunsupported
rust-cpython: raising error.WdirUnsupported

The Graph implementation of hg-cpython returns the appropriate error
upon encounter with the working directory special revision number, and
this gives us in particular a code path to test from test-rust-ancestors.py

In the current implementation, the exception is actually raised from
the iterator instantiation; we are nonetheless consuming the iterator
in the test with `list()` in order not to depend on implementation details.
Josef 'Jeff' Sipek - Jan. 24, 2019, 4:11 p.m.
On Wed, Jan 23, 2019 at 23:23:53 -0500, Georges Racinet wrote:
...
> diff -r a35cfd592a90 -r 47d5458a4c32 tests/test-rust-ancestor.py
> --- a/tests/test-rust-ancestor.py	Wed Jan 23 07:47:04 2019 -0500
> +++ b/tests/test-rust-ancestor.py	Wed Jan 23 07:49:36 2019 -0500
> @@ -1,6 +1,10 @@
>  from __future__ import absolute_import
>  import sys
>  import unittest
> +from mercurial import (
> +    error,
> +    node,
> +    )

Is there supposed to be whitespace before the ) ?  I'm not all that familiar
with the python coding style.

I know, such important feedback... :)

Jeff.


>  
>  try:
>      from mercurial import rustext
> @@ -153,6 +157,12 @@
>          # rust-cpython issues appropriate str instances for Python 2 and 3
>          self.assertEqual(exc.args, ('ParentOutOfRange', 1))
>  
> +    def testwdirunsupported(self):
> +        # trying to access ancestors of the working directory raises
> +        # WdirUnsupported directly
> +        idx = self.parseindex()
> +        with self.assertRaises(error.WdirUnsupported):
> +            list(AncestorsIterator(idx, [node.wdirrev], -1, False))
>  
>  if __name__ == '__main__':
>      import silenttestrunner
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Georges Racinet - Jan. 24, 2019, 8:41 p.m.
On 1/24/19 11:11 AM, Josef 'Jeff' Sipek wrote:

> On Wed, Jan 23, 2019 at 23:23:53 -0500, Georges Racinet wrote:
> ...
>> diff -r a35cfd592a90 -r 47d5458a4c32 tests/test-rust-ancestor.py
>> --- a/tests/test-rust-ancestor.py	Wed Jan 23 07:47:04 2019 -0500
>> +++ b/tests/test-rust-ancestor.py	Wed Jan 23 07:49:36 2019 -0500
>> @@ -1,6 +1,10 @@
>>  from __future__ import absolute_import
>>  import sys
>>  import unittest
>> +from mercurial import (
>> +    error,
>> +    node,
>> +    )
> Is there supposed to be whitespace before the ) ?  I'm not all that familiar
> with the python coding style.

I'm not sure about that, but the coding style [1] doesn't seem to say
much about that case, and, since it's cited as a lose reference, pep8
accepts both.

The indentation you're seeing is just what my editor does by default. I
didn't do anything else than making test-check-* happy

[1] https://www.mercurial-scm.org/wiki/CodingStyle#Whitespace_and_syntax

> I know, such important feedback... :)

No worries ;-)
Josef 'Jeff' Sipek - Jan. 24, 2019, 9:31 p.m.
On Thu, Jan 24, 2019 at 15:41:56 -0500, Georges Racinet wrote:
> On 1/24/19 11:11 AM, Josef 'Jeff' Sipek wrote:
> 
> > On Wed, Jan 23, 2019 at 23:23:53 -0500, Georges Racinet wrote:
> > ...
> >> diff -r a35cfd592a90 -r 47d5458a4c32 tests/test-rust-ancestor.py
> >> --- a/tests/test-rust-ancestor.py	Wed Jan 23 07:47:04 2019 -0500
> >> +++ b/tests/test-rust-ancestor.py	Wed Jan 23 07:49:36 2019 -0500
> >> @@ -1,6 +1,10 @@
> >>  from __future__ import absolute_import
> >>  import sys
> >>  import unittest
> >> +from mercurial import (
> >> +    error,
> >> +    node,
> >> +    )
> > Is there supposed to be whitespace before the ) ?  I'm not all that familiar
> > with the python coding style.
> 
> I'm not sure about that, but the coding style [1] doesn't seem to say
> much about that case, and, since it's cited as a lose reference, pep8
> accepts both.
> 
> The indentation you're seeing is just what my editor does by default. I
> didn't do anything else than making test-check-* happy

Fair enough.  Sorry for the noise. :)

For the little it is worth, I didn't see anything wrong with the rest of the
series.  

Jeff.
Yuya Nishihara - Jan. 25, 2019, 8:42 a.m.
On Thu, 24 Jan 2019 11:11:41 -0500, Josef 'Jeff' Sipek wrote:
> On Wed, Jan 23, 2019 at 23:23:53 -0500, Georges Racinet wrote:
> ...
> > diff -r a35cfd592a90 -r 47d5458a4c32 tests/test-rust-ancestor.py
> > --- a/tests/test-rust-ancestor.py	Wed Jan 23 07:47:04 2019 -0500
> > +++ b/tests/test-rust-ancestor.py	Wed Jan 23 07:49:36 2019 -0500
> > @@ -1,6 +1,10 @@
> >  from __future__ import absolute_import
> >  import sys
> >  import unittest
> > +from mercurial import (
> > +    error,
> > +    node,
> > +    )
> 
> Is there supposed to be whitespace before the ) ?  I'm not all that familiar
> with the python coding style.

Queued, thanks.

Actually we prefer ")" at the same level of the starting statement. Emacs
can be configured as such:

  (setq py-closing-list-dedents-bos t)

Patch

diff -r a35cfd592a90 -r 47d5458a4c32 mercurial/revlog.py
--- a/mercurial/revlog.py	Wed Jan 23 07:47:04 2019 -0500
+++ b/mercurial/revlog.py	Wed Jan 23 07:49:36 2019 -0500
@@ -896,8 +896,6 @@ 
             common = [nullrev]
 
         if rustext is not None:
-            # TODO: WdirUnsupported should be raised instead of GraphError
-            # if common includes wdirrev
             return rustext.ancestor.MissingAncestors(self.index, common)
         return ancestor.incrementalmissingancestors(self.parentrevs, common)
 
diff -r a35cfd592a90 -r 47d5458a4c32 rust/hg-cpython/src/cindex.rs
--- a/rust/hg-cpython/src/cindex.rs	Wed Jan 23 07:47:04 2019 -0500
+++ b/rust/hg-cpython/src/cindex.rs	Wed Jan 23 07:49:36 2019 -0500
@@ -16,7 +16,7 @@ 
 
 use self::python_sys::PyCapsule_Import;
 use cpython::{PyClone, PyErr, PyObject, PyResult, Python};
-use hg::{Graph, GraphError, Revision};
+use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
 use libc::c_int;
 use std::ffi::CStr;
 use std::mem::transmute;
@@ -86,6 +86,9 @@ 
 impl Graph for Index {
     /// wrap a call to the C extern parents function
     fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
+        if rev == WORKING_DIRECTORY_REVISION {
+            return Err(GraphError::WorkingDirectoryUnsupported);
+        }
         let mut res: [c_int; 2] = [0; 2];
         let code = unsafe {
             (self.parents)(
diff -r a35cfd592a90 -r 47d5458a4c32 tests/test-rust-ancestor.py
--- a/tests/test-rust-ancestor.py	Wed Jan 23 07:47:04 2019 -0500
+++ b/tests/test-rust-ancestor.py	Wed Jan 23 07:49:36 2019 -0500
@@ -1,6 +1,10 @@ 
 from __future__ import absolute_import
 import sys
 import unittest
+from mercurial import (
+    error,
+    node,
+    )
 
 try:
     from mercurial import rustext
@@ -153,6 +157,12 @@ 
         # rust-cpython issues appropriate str instances for Python 2 and 3
         self.assertEqual(exc.args, ('ParentOutOfRange', 1))
 
+    def testwdirunsupported(self):
+        # trying to access ancestors of the working directory raises
+        # WdirUnsupported directly
+        idx = self.parseindex()
+        with self.assertRaises(error.WdirUnsupported):
+            list(AncestorsIterator(idx, [node.wdirrev], -1, False))
 
 if __name__ == '__main__':
     import silenttestrunner