[MERGE] DirState pyrex helpers

[Martin Pool]

Martin Pool has voted +0.
Status is now: Semi-approved
Comment:
(These are in kind of random order as I tried to page around to find the
logical top.)

+      read the entire DirState (atm, all commands that effect the 
working
+      tree, ``diff``, ``status``, etc).

Please spell out 'at the moment', and it's 'affect' for 'has an effect
on'.  It would be good to also give indicative performance numbers.

+class TestUsingCompiledIfAvailable(tests.TestCase):
+    """Check that any compiled functions that are available are the 
default.
+
+    It is possible to have typos, etc in the import line, such that
+    _dirstate_helpers_c is actually available, but the compiled 
functions are
+    not being used.
+    """

I'm glad you tested it, this is a pretty tidy way to guard against a
possible mistake.  If there were many more interfaces done this way
it might be better to have a list of names and iterate over them, but I
think for just four this is simpler.  Indeed maybe we should have a test
utility for "check there are parallel python and C implementations of
foo.{bar,qux}"

+    _test_needs_features = [CompiledDirstateHelpersFeature]

(Incidentally) this is the kind of case where I'd like to have
TestDependencyMissing, and have that be disallowed by selftest --strict.
(See my other rfc mail.)

+
+class TestCompiledCmpByDirs(TestCmpByDirs):
+    """Test the pyrex implementation of cmp_by_dirs"""
+
+    _test_needs_features = [CompiledDirstateHelpersFeature]
+
+    def get_cmp_by_dirs(self):
+        from bzrlib._dirstate_helpers_c import cmp_by_dirs_c
+        return cmp_by_dirs_c

Could you please add a comment just explaining that this parameterizes 
the
tests by overriding get_cmp_by_dirs - like in 
TestCompiledBisectPathLeft.

+class TestCompiledCmpPathByDirblock(TestCmpPathByDirblock):
+    """Test the pyrex implementation of _cmp_path_by_dirblock"""
+
+    _test_needs_features = [CompiledDirstateHelpersFeature]
+
+    def get_cmp_by_dirs(self):
+        from bzrlib._dirstate_helpers_c import _cmp_path_by_dirblock_c
+        return _cmp_path_by_dirblock_c
+
+

Likewise.

+
+    def assertPositive(self, val):
+        """Assert that val is greater than 0."""
+        self.assertTrue(val > 0, 'expected a positive value, but got 
%s' % val)
+
+    def assertNegative(self, val):
+        """Assert that val is less than 0."""
+        self.assertTrue(val < 0, 'expected a negative value, but got 
%s' % val)
+

Could you please put these on TestCase, so they'll be done only once.


+# We cannot import the dirstate module, because it loads this module
+# All we really need is the IN_MEMORY_MODIFIED constant
+from bzrlib.dirstate import DirState
+
+

I don't understand, won't the from/import fail if there's a circular
import?

I haven't finished reading the pyrex but I'll just post this to get it 
out
there.  I didn't see any severe problems.

 > Anyway, I don't really know how to do this sort of testing in a 
platform
 > independent manner. (On some forms of Linux you can read 
/proc/PID/status to
> find your memory usage, etc)

I would recommend valgrind.  I'm not sure how well it will interact with
Python's gc but it'd be worth a try.

The other thing I would suggest would be to check that the dirstate
docstring is totally clear about the ordering, since this has caused
problems in the past.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C4696AFBF.7010405%40arbash-meinel.com%3E