Code review comment for lp:~axwalk/juju-core/localstorage-to-httpstorage

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go
File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go#newcode54
environs/httpstorage/backend.go:54: defer readcloser.Close()
On 2013/09/18 04:26:28, thumper wrote:
> Why defer? There is no harm in calling Close right now.

Indeed. I'll keep the defer, but move it closer to Get/before ReadAll.

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go#newcode75
environs/httpstorage/backend.go:75: if req.ContentLength <= -1 {
On 2013/09/18 04:26:28, thumper wrote:
> isn't " < 0 " more readable and understandable?

Done.

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend_test.go
File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend_test.go#newcode109
environs/httpstorage/backend_test.go:109: // Get on a directory returns
a 500 as it is
On 2013/09/18 04:26:28, thumper wrote:
> I argue with this status code.

> 5xx means it is our fault. 4xx means it is your fault. This is
certainly a bad
> request.

Yeah, I'll change the implementation (of filestorage's Get) and put this
back. Thanks.

https://codereview.appspot.com/13736043/diff/4001/environs/testing/storage.go
File environs/testing/storage.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/testing/storage.go#newcode33
environs/testing/storage.go:33: listener, err :=
httpstorage.Serve("localhost:0", underlying)
On 2013/09/18 04:26:28, thumper wrote:
> Given this is the second or third time I have seen these lines of
code, perhaps
> a testing helper function would be good.

This is a testing helper function ;)
I will modify the other code to use it where it makes sense.

https://codereview.appspot.com/13736043/