Don't catch AssertionError

[John Arbash Meinel]

Robey Pointer wrote:
> 
> On 21 Dec 2006, at 15:08, John Arbash Meinel wrote:
> 
>> I just wanted to remind everyone that if you find yourself writing a
>> test that catches AssertionError, it is usually a sign that something is
>> wrong with the code.
>>
>> I know of one place where we *have* to. But it is because paramiko has
>> an assert, rather than something we are doing.
> 
> I just grepped bzrlib for this and I think the assert was in
> transport/sftp.py, not paramiko, so we can easily avoid doing it. 
> Here's a quick bundle that replaces the (commented-out) assert with a
> PathNotChild exception, and uncomments the corresponding unit tests. 
> The tests all still pass.
> 
> robey

Well, you are correct that this can be changed as you say it is.

But my comment was regarding around line 455 in sftp_file.py:

    def _async_response(self, t, msg):
        if t == CMD_STATUS:
            # save exception and re-raise it on next file operation
            try:
                self.sftp._convert_status(msg)
            except Exception, x:
                self._saved_exception = x
            return
        if t != CMD_DATA:
            raise SFTPError('Expected data')
        data = msg.get_string()
        offset, length = self._prefetch_reads.pop(0)
        assert length == len(data)
        self._prefetch_data[offset] = data
        if len(self._prefetch_reads) == 0:
            self._prefetch_done = True


The 'assert length == len(data)' is making sure that the returned data
is the exact same length as what was requested. Which could be violated
if you allowed requests >32KB, or if there was a readv for 100 bytes,
and there is only 80 bytes left in the file.

paramiko shouldn't be using 'assert' to check an invariant in something
that it is reading from the real world. It should be doing a real check,
and raising a real error if it fails.

But I'm +1 on your patch, regardless.

John
=:->