Rev 3692: Respond to Martin's review comments. in http://bzr.arbash-meinel.com/branches/bzr/1.8-dev/sftp_chunked

[John Arbash Meinel]

At http://bzr.arbash-meinel.com/branches/bzr/1.8-dev/sftp_chunked

------------------------------------------------------------
revno: 3692
revision-id: john at arbash-meinel.com-20080905212334-69j0qvvr9trvfk1b
parent: john at arbash-meinel.com-20080904030327-6smkelvc2rpjk3k6
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: sftp_chunked
timestamp: Fri 2008-09-05 16:23:34 -0500
message:
  Respond to Martin's review comments.
-------------- next part --------------
=== modified file 'bzrlib/errors.py'
--- a/bzrlib/errors.py	2008-09-03 22:03:10 +0000
+++ b/bzrlib/errors.py	2008-09-05 21:23:34 +0000
@@ -666,16 +666,6 @@
         self.actual = actual
 
 
-class OverlappingReadv(BzrError):
-    """Raised when a readv() requests overlapping chunks of data.
-
-    Not all transports supports this, so the api should generally forbid it.
-    (It isn't a feature we need anyway.
-    """
-
-    _fmt = 'Requested readv ranges overlap'
-
-
 class PathNotChild(PathError):
 
     _fmt = 'Path "%(path)s" is not a child of path "%(base)s"%(extra)s'

=== modified file 'bzrlib/tests/test_sftp_transport.py'
--- a/bzrlib/tests/test_sftp_transport.py	2008-09-03 23:26:35 +0000
+++ b/bzrlib/tests/test_sftp_transport.py	2008-09-05 21:23:34 +0000
@@ -464,20 +464,20 @@
 
 class Test_SFTPReadvHelper(tests.TestCase):
 
-    def assertGetRequests(self, expected_requests, offsets):
+    def checkGetRequests(self, expected_requests, offsets):
         helper = _mod_sftp._SFTPReadvHelper(offsets, 'artificial_test')
         self.assertEqual(expected_requests, helper._get_requests())
 
     def test__get_requests(self):
         # Small single requests become a single readv request
-        self.assertGetRequests([(0, 100)], [(0, 20), (30, 50), (20, 10),
-                                            (80, 20)])
+        self.checkGetRequests([(0, 100)],
+                              [(0, 20), (30, 50), (20, 10), (80, 20)])
         # Non-contiguous ranges are given as multiple requests
-        self.assertGetRequests([(0, 20), (30, 50)],
-                               [(10, 10), (30, 20), (0, 10), (50, 30)])
+        self.checkGetRequests([(0, 20), (30, 50)],
+                              [(10, 10), (30, 20), (0, 10), (50, 30)])
         # Ranges larger than _max_request_size (32kB) are broken up into
         # multiple requests, even if it actually spans multiple logical
         # requests
-        self.assertGetRequests([(0, 32768), (32768, 32768), (65536, 464)],
-                               [(0, 40000), (40000, 100), (40100, 1900),
-                                (42000, 24000)])
+        self.checkGetRequests([(0, 32768), (32768, 32768), (65536, 464)],
+                              [(0, 40000), (40000, 100), (40100, 1900),
+                               (42000, 24000)])

=== modified file 'bzrlib/transport/__init__.py'
--- a/bzrlib/transport/__init__.py	2008-09-03 22:03:10 +0000
+++ b/bzrlib/transport/__init__.py	2008-09-05 21:23:34 +0000
@@ -751,14 +751,18 @@
         return offsets
 
     @staticmethod
-    def _coalesce_offsets(offsets, limit=0, fudge_factor=0, max_size=0,
-                          allow_overlap=False):
+    def _coalesce_offsets(offsets, limit=0, fudge_factor=0, max_size=0):
         """Yield coalesced offsets.
 
         With a long list of neighboring requests, combine them
         into a single large request, while retaining the original
         offsets.
         Turns  [(15, 10), (25, 10)] => [(15, 20, [(0, 10), (10, 10)])]
+        Note that overlapping requests are not permitted. (So [(15, 10), (20,
+        10)] will raise a ValueError.) This is because the data we access never
+        overlaps, and it allows callers to trust that we only need any byte of
+        data for 1 request (so nothing needs to be buffered to fulfill a second
+        request.)
 
         :param offsets: A list of (start, length) pairs
         :param limit: Only combine a maximum of this many pairs Some transports
@@ -772,8 +776,6 @@
                 When a single offset is bigger than 'max_size', it will keep
                 its size and be alone in the coalesced offset.
                 0 means no maximum size.
-        :param allow_overlap: If False, raise an error if requested ranges
-            overlap.
         :return: return a list of _CoalescedOffset objects, which have members
             for where to start, how much to read, and how to split those chunks
             back up
@@ -789,8 +791,10 @@
                 and start >= cur.start
                 and (limit <= 0 or len(cur.ranges) < limit)
                 and (max_size <= 0 or end - cur.start <= max_size)):
-                if not allow_overlap and start < last_end:
-                    raise errors.OverlappingReadv()
+                if start < last_end:
+                    raise errors.ValueError('Overlapping range not allowed:'
+                        ' last range ended at %s, new one starts at %s'
+                        % (last_end, start))
                 cur.length = end - cur.start
                 cur.ranges.append((start-cur.start, size))
             else: