[MERGE] readv access to pack files

[Andrew Bennetts]

!tweak

Robert Collins wrote:
> === modified file 'bzrlib/tests/test_pack.py'
> --- bzrlib/tests/test_pack.py	2007-07-03 04:05:08 +0000
> +++ bzrlib/tests/test_pack.py	2007-08-02 03:17:46 +0000
> @@ -54,7 +54,8 @@
>          output = StringIO()
>          writer = pack.ContainerWriter(output.write)
>          writer.begin()
> -        writer.add_bytes_record('abc', names=[])
> +        offset, length = writer.add_bytes_record('abc', names=[])
> +        self.assertEqual((42, 7), (offset, length))

It's a pity that these numbers are so opaque to the casual human reader, but
there isn't much to do about that.  You've made it pretty clear what their
purpose is already, I just wish it was immediately obvious to humans that "42"
really is the right answer!  I think what you've done is the least worst option.

> +class TestReadvReader(tests.TestCaseWithTransport):
> +

A docstring to make it clear that this test case is for exercising
make_readv_reader would be good, just to be completely explicit.

> +    def test_read_skipping_records(self):
> +        pack_data = StringIO()
> +        writer = pack.ContainerWriter(pack_data.write)
> +        writer.begin()
> +        memos = []
> +        memos.append(writer.add_bytes_record('abc', names=[]))
> +        memos.append(writer.add_bytes_record('def', names=['name1']))
> +        memos.append(writer.add_bytes_record('ghi', names=['name2']))
> +        memos.append(writer.add_bytes_record('jkl', names=[]))
> +        writer.end()
> +        transport = self.get_transport()
> +        pack_data.seek(0)
> +        transport.put_file('mypack', pack_data)

It might be a little simpler to use: "transport.put_bytes('mypack',
pack_data.getvalue())" here, you'd be able to avoid the seek(0) line.

> +class TestReadvFile(tests.TestCaseWithTransport):
> +
> +    def test_read_bytes(self):
> +        # this tests byte reading - solo, all in a hunk at once
[...]
> +
> +    def test_readline(self):
> +        # this tests readline as the ContainerReader needs it - 
> +        # just within a record, never across.
[...]
> +
> +    def test_readline_and_read(self):
> +        # this tests read, then readline, then read again 
[...]

The existing tests in this file all have proper """Docstrings.""" with fully
punctuated sentences, it would be good to be consistent with that.

You don't test what happens if you try to read past the end of the current
record... Judging from the hint in the comments, I guess you're intentionally
leaving that undefined, rather than specifying a particular error?  Is that just
to make the implementation easier?  It seems like it would be good to have a
clear error if a caller did get confused and tried to misuse this.  If it's just
a case of "it's not very important, haven't bothered yet" then I'm ok with that,
but it would be good to have an explicit comment (even just a brief one) to make
it clear that this is so, rather than intentionally unspecified.  Intentions are
much harder to divine after the fact.  Or just write tests for the error cases,
you've already got some (untested) code for those cases...

I have no real complaints though, just comments about comments! :)

-Andrew.