Merge lp:~gz/brz/lt_path_by_dirblock into lp:brz

[Martin Packman]

Replace dirstate helper _cmp_path_by_dirblock with _lt_path_by_dirblock

Again, the bisect usage of _cmp_path_by_dirblock does not actually require cmp semantics, lt is fine. Changes the exposed interface, but doesn't update the pyrex internal implementation quite yet.

Side note, due to an error in the test method name, the compiled version of this function wasn't being tested till now.

[Jelmer Vernooij (jelmer)]

review: Approve

=== modified file 'breezy/_dirstate_helpers_py.py'
--- breezy/_dirstate_helpers_py.py	2017-06-04 18:09:30 +0000
+++ breezy/_dirstate_helpers_py.py	2017-06-10 01:16:43 +0000
@@ -94,7 +94,7 @@
         mid = (lo + hi) // 2
         # Grab the dirname for the current dirblock
         cur = paths[mid]
-        if _cmp_path_by_dirblock(cur, path) < 0:
+        if _lt_path_by_dirblock(cur, path):
             lo = mid + 1
         else:
             hi = mid
@@ -125,7 +125,7 @@
         mid = (lo+hi)//2
         # Grab the dirname for the current dirblock
         cur = paths[mid]
-        if _cmp_path_by_dirblock(path, cur) < 0:
+        if _lt_path_by_dirblock(path, cur):
             hi = mid
         else:
             lo = mid + 1
@@ -189,7 +189,7 @@
     return cmp(path1.split('/'), path2.split('/'))
 
 
-def _cmp_path_by_dirblock(path1, path2):
+def _lt_path_by_dirblock(path1, path2):
     """Compare two paths based on what directory they are in.
 
     This generates a sort order, such that all children of a directory are
@@ -198,9 +198,7 @@
 
     :param path1: first path
     :param path2: the second path
-    :return: negative number if ``path1`` comes first,
-        0 if paths are equal
-        and a positive number if ``path2`` sorts first
+    :return: True if path1 comes first, otherwise False
     """
     if not isinstance(path1, str):
         raise TypeError("'path1' must be a plain string, not %s: %r"
@@ -212,7 +210,7 @@
     key1 = (dirname1.split('/'), basename1)
     dirname2, basename2 = os.path.split(path2)
     key2 = (dirname2.split('/'), basename2)
-    return cmp(key1, key2)
+    return key1 < key2
 
 
 def _read_dirblocks(state):

=== modified file 'breezy/_dirstate_helpers_pyx.pyx'
--- breezy/_dirstate_helpers_pyx.pyx	2017-06-10 00:09:13 +0000
+++ breezy/_dirstate_helpers_pyx.pyx	2017-06-10 01:16:43 +0000
@@ -273,7 +273,7 @@
                         PyString_Size(path2))
 
 
-def _cmp_path_by_dirblock(path1, path2):
+def _lt_path_by_dirblock(path1, path2):
     """Compare two paths based on what directory they are in.
 
     This generates a sort order, such that all children of a directory are
@@ -285,9 +285,7 @@
 
     :param path1: first path
     :param path2: the second path
-    :return: negative number if ``path1`` comes first,
-        0 if paths are equal
-        and a positive number if ``path2`` sorts first
+    :return: True if path1 comes first, otherwise False.
     """
     if not PyString_CheckExact(path1):
         raise TypeError("'path1' must be a plain string, not %s: %r"
@@ -295,10 +293,11 @@
     if not PyString_CheckExact(path2):
         raise TypeError("'path2' must be a plain string, not %s: %r"
                         % (type(path2), path2))
-    return _cmp_path_by_dirblock_intern(PyString_AsString(path1),
-                                        PyString_Size(path1),
-                                        PyString_AsString(path2),
-                                        PyString_Size(path2))
+    # GZ 2017-06-09: This internal function really only needs lt as well.
+    return (_cmp_path_by_dirblock_intern(PyString_AsString(path1),
+                                         PyString_Size(path1),
+                                         PyString_AsString(path2),
+                                         PyString_Size(path2)) < 0)
 
 
 cdef int _cmp_path_by_dirblock_intern(char *path1, int path1_len,

=== modified file 'breezy/tests/test__dirstate_helpers.py'
--- breezy/tests/test__dirstate_helpers.py	2017-05-22 00:56:52 +0000
+++ breezy/tests/test__dirstate_helpers.py	2017-06-10 01:16:43 +0000
@@ -498,22 +498,22 @@
         return cmp_by_dirs
 
 
-class TestCmpPathByDirblock(tests.TestCase):
-    """Test an implementation of _cmp_path_by_dirblock()
+class TestLtPathByDirblock(tests.TestCase):
+    """Test an implementation of _lt_path_by_dirblock()
 
-    _cmp_path_by_dirblock() compares two paths using the sort order used by
+    _lt_path_by_dirblock() compares two paths using the sort order used by
     DirState. All paths in the same directory are sorted together.
 
-    Child test cases can override ``get_cmp_path_by_dirblock`` to test a specific
+    Child test cases can override ``get_lt_path_by_dirblock`` to test a specific
     implementation.
     """
 
-    def get_cmp_path_by_dirblock(self):
-        """Get a specific implementation of _cmp_path_by_dirblock."""
-        from breezy._dirstate_helpers_py import _cmp_path_by_dirblock
-        return _cmp_path_by_dirblock
+    def get_lt_path_by_dirblock(self):
+        """Get a specific implementation of _lt_path_by_dirblock."""
+        from breezy._dirstate_helpers_py import _lt_path_by_dirblock
+        return _lt_path_by_dirblock
 
-    def assertCmpPathByDirblock(self, paths):
+    def assertLtPathByDirblock(self, paths):
         """Compare all paths and make sure they evaluate to the correct order.
 
         This does N^2 comparisons. It is assumed that ``paths`` is properly
@@ -527,33 +527,22 @@
             return dirname.split('/'), basename
         self.assertEqual(sorted(paths, key=_key), paths)
 
-        cmp_path_by_dirblock = self.get_cmp_path_by_dirblock()
+        lt_path_by_dirblock = self.get_lt_path_by_dirblock()
         for idx1, path1 in enumerate(paths):
             for idx2, path2 in enumerate(paths):
-                cmp_val = cmp_path_by_dirblock(path1, path2)
-                if idx1 < idx2:
-                    self.assertTrue(cmp_val < 0,
-                        '%s did not state that %r came before %r, cmp=%s'
-                        % (cmp_path_by_dirblock.__name__,
-                           path1, path2, cmp_val))
-                elif idx1 > idx2:
-                    self.assertTrue(cmp_val > 0,
-                        '%s did not state that %r came after %r, cmp=%s'
-                        % (cmp_path_by_dirblock.__name__,
-                           path1, path2, cmp_val))
-                else: # idx1 == idx2
-                    self.assertTrue(cmp_val == 0,
-                        '%s did not state that %r == %r, cmp=%s'
-                        % (cmp_path_by_dirblock.__name__,
-                           path1, path2, cmp_val))
+                lt_result = lt_path_by_dirblock(path1, path2)
+                self.assertEqual(idx1 < idx2, lt_result,
+                        '%s did not state that %r < %r, lt=%s'
+                        % (lt_path_by_dirblock.__name__,
+                           path1, path2, lt_result))
 
     def test_cmp_simple_paths(self):
         """Compare against the empty string."""
-        self.assertCmpPathByDirblock(['', 'a', 'ab', 'abc', 'a/b/c', 'b/d/e'])
-        self.assertCmpPathByDirblock(['kl', 'ab/cd', 'ab/ef', 'gh/ij'])
+        self.assertLtPathByDirblock(['', 'a', 'ab', 'abc', 'a/b/c', 'b/d/e'])
+        self.assertLtPathByDirblock(['kl', 'ab/cd', 'ab/ef', 'gh/ij'])
 
     def test_tricky_paths(self):
-        self.assertCmpPathByDirblock([
+        self.assertLtPathByDirblock([
             # Contents of ''
             '', 'a', 'a-a', 'a=a', 'b',
             # Contents of 'a'
@@ -581,7 +570,7 @@
             # Contents of 'b',
             'b/a', 'b/b',
             ])
-        self.assertCmpPathByDirblock([
+        self.assertLtPathByDirblock([
                  # content of '/'
                  '', 'a', 'a-a', 'a-z', 'a=a', 'a=z',
                  # content of 'a/'
@@ -612,16 +601,16 @@
                 ])
 
     def test_unicode_not_allowed(self):
-        cmp_path_by_dirblock = self.get_cmp_path_by_dirblock()
-        self.assertRaises(TypeError, cmp_path_by_dirblock, u'Uni', 'str')
-        self.assertRaises(TypeError, cmp_path_by_dirblock, 'str', u'Uni')
-        self.assertRaises(TypeError, cmp_path_by_dirblock, u'Uni', u'Uni')
-        self.assertRaises(TypeError, cmp_path_by_dirblock, u'x/Uni', 'x/str')
-        self.assertRaises(TypeError, cmp_path_by_dirblock, 'x/str', u'x/Uni')
-        self.assertRaises(TypeError, cmp_path_by_dirblock, u'x/Uni', u'x/Uni')
+        lt_path_by_dirblock = self.get_lt_path_by_dirblock()
+        self.assertRaises(TypeError, lt_path_by_dirblock, u'Uni', 'str')
+        self.assertRaises(TypeError, lt_path_by_dirblock, 'str', u'Uni')
+        self.assertRaises(TypeError, lt_path_by_dirblock, u'Uni', u'Uni')
+        self.assertRaises(TypeError, lt_path_by_dirblock, u'x/Uni', 'x/str')
+        self.assertRaises(TypeError, lt_path_by_dirblock, 'x/str', u'x/Uni')
+        self.assertRaises(TypeError, lt_path_by_dirblock, u'x/Uni', u'x/Uni')
 
     def test_nonascii(self):
-        self.assertCmpPathByDirblock([
+        self.assertLtPathByDirblock([
             # content of '/'
             '', 'a', '\xc2\xb5', '\xc3\xa5',
             # content of 'a'
@@ -639,14 +628,14 @@
             ])
 
 
-class TestCompiledCmpPathByDirblock(TestCmpPathByDirblock):
-    """Test the pyrex implementation of _cmp_path_by_dirblock"""
+class TestCompiledLtPathByDirblock(TestLtPathByDirblock):
+    """Test the pyrex implementation of _lt_path_by_dirblock"""
 
     _test_needs_features = [compiled_dirstate_helpers_feature]
 
-    def get_cmp_by_dirs(self):
-        from breezy._dirstate_helpers_pyx import _cmp_path_by_dirblock
-        return _cmp_path_by_dirblock
+    def get_lt_path_by_dirblock(self):
+        from breezy._dirstate_helpers_pyx import _lt_path_by_dirblock
+        return _lt_path_by_dirblock
 
 
 class TestMemRChr(tests.TestCase):