[MERGE] readv access to pack files

[Robert Collins]

On Thu, 2007-08-02 at 15:02 +1000, Andrew Bennetts wrote:
> !tweak
> 

> > +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.

Perhaps just renaming to TestMakeReadvReader ?

> > +    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.

Sure.

> > +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.

Ok, I'll do 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?

Yup. Theres no way short of massive index corruption or file corruption
that we'll ask for ranges that don't match records, and in the match
case its ok, and in the non match case we don't have any defined
behaviour higher up the stack at the moment.

>   It seems like it would be good to have a
> clear error if a caller did get confused and tried to misuse this.

In terms of errors, I think a more important error to handle are IO
errors, to make them diagnosable. Unfortunately I don't really know what
is needed to diagnose them inside the container code; and readv failures
are pretty opaque anyway, because you are deep in iterator - or even
thread for sftp - foo.

>   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'll add a comment saying that the error cases are deliberately
unspecified at this point.

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

Thanks,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070802/c70fb871/attachment-0001.pgp