[MERGE][1.6][#254797] Fix NotImplementedError when probing for smart protocol via HTTP.

[Andrew Bennetts]

John Arbash Meinel wrote:
[...]
> The only thing I don't really like is:
> 
> +    def _read_line(self):
> +        line, excess = medium._get_line(self._response_body.read)
> +        if excess != '':
> +            raise AssertionError(
> +                '_get_line returned excess bytes, but this mediumrequest '
> +                'cannot handle excess. (%r)' % (excess,))
> +        return line
> +
> 
> ^- you are using AssertionError to validate data from the wire, and the code
> path is completely untested.
> 
> I'll let it pass, but I personally like to see failure code *exercised* by the
> test suite.

Don't worry, it doesn't rely on data on the wire at all.  This assertion would
only happen if self._response_body.read(1) ever returned more than 1 byte; i.e.
if someone changes the implementation of RangeFile.read to do that.  That's
unlikely (because then RangeFile wouldn't meet the contract of a file-like
object), but it is worth having an AssertionError for, just in case. 

_get_line is intentionally written so that it will ask for one byte at a time,
but also so that it's ok if it receives more bytes than asked for.

In most medium implementations, there's a push back buffer so that _get_line and
other things can return unconsumed bytes to the medium for the next consumer.
This is so that media like sockets, where its easy to try read 64k bytes without
blocking, can always read large blocks for better performance.

The HTTP medium isn't structured in a way that makes it easy to reuse the push
back buffer logic that other media can use.  But the _response_body the medium
request reads from doesn't ever return more bytes than asked for, so that
doesn't matter.

> The test also seems a bit indirect. I realize it probably would fail to
> BzrDir.open('smart+http://') but it doesn't really feel like you are testing
> that Medium's are implementing the appropriate interface.

Yes, it would be good to have per-medium tests.  That's on a todo list of mine
somewhere...

> Anyway, this seems like a minimal fix for 1.6, and there isn't anything I
> would strictly block on. And it mostly comes back to wanting
> "smart_medium_implementations/" testing.

Right, exactly :)

It would be good to find time to work on those tests, although I think the
medium layer is pretty settled now (despite this bug), so it's hard to feel like
it's worth doing before other bugs and features.  If I do find myself doing
non-trivial work in it I will try to make the time to do implementations tests
first, though.

> So...
> 
> BB:approve
> 
> And I'll get it queued up for 1.6.

Thanks!

-Andrew.