Imminent MERGE request: 12 steps towards a high performance server

[Andrew Bennetts]

On Wed, Sep 13, 2006 at 12:10:18PM -0500, John Arbash Meinel wrote:
> ...
> 
> >    http://people.ubuntu.com/~andrew/bzr/extra-transport-tests/
> >      this adds a bunch of tests for transports, particularly about how the path
> >      handling at the root of a url works -- and fixes the bugs they reveal.
> 
> -            self.assertEquals(str(e),
> -                    'Transport error: ~janneke: invalid port number ')
> +            self.assertEquals(str(e),
> +                    'Transport error: '
> +                    'invalid port number ~janneke in url '
> +                    'sftp://lilypond.org:~janneke/public_html/bzr/gub ')
> +##             self.assertEquals(str(e),
> +##                     'Transport error: ~janneke: invalid port number ')
> 
> Don't comment out the old lines, just delete them. (I understand
> commenting while you are getting it written, but in the long term, just
> get rid of it)

Oops, thanks.

> Also, URLs are usually long things, such that if they are going to be
> user-facing, I would rather see them on a line of their own. Something like:
> 
> Transport error: invalid port number ~janneke in url:
> sftp://lilypond.org:~janneke/public_html/bzr/gub

Ok.

> Also, why is there a blank space at the end of the error string? you have:
> 
> '.../bzr/gub '
>             ^- this is the space in question.

That's weird.  Oh, it's because the TransportError is only being constructed
with one argument, but the format string is: """Transport error: %(msg)s
%(orig_error)s""".  The constructor of TransportError copes with either msg or
orig_error being omitted, but doesn't bother about fixing the trailing
whitespace.  Given that it's likely to just be displayed to a console, it's
probably not worth caring about.

> I'm not sure that this test is valid:
> +        self.assertRaises(NoSuchFile,
> +                          t.put, 'path/doesnt/exist/c',
> StringIO('contents'))
> +
> 
> At the very least, since put() is now deprecated, this will be spewing a
> deprecated() parameter to the screen. I don't think it is really needed
> anymore, but you can tell me if you feel differently.

You're right, it should use applyDeprecated (this work predating put being
deprecated).  Fixed.

I don't know why you think it's invalid -- you think the behaviour of putting a
file into a non-existent location is undefined?  Expecting it to raise
NoSuchFile seems uncontroversial to me.

I think it's useful to improve testing of deprecated code -- just because we
intend to rip it out "soon" shouldn't excuse it from being reasonable quality.

> You seem to be adding a 'put_permissions' test, even though 'put()' is a
> deprecated API. I think this is unnecessary. If you want, you can do a
> single test, to make sure that put() is taking a mode, and passing it to
> put_file() or put_bytes(). But I don't think we need to test the full
> set of modes.

Yeah, since that test was written put has been deprecated in favour of put_file
and put_bytes, and tests have been added for those methods' permission handling.
However it's possible that while put is there that an implementation of a
transport might implement it in some way other than just forwarding to
put_bytes or put_file, so I think we still want to test that put itself, however
it works internally, deals with permissions correctly.

I don't think this extra test is a significant burden, and it has more value as
is than trying to cut it down.

> The http/__init__.py changes:
> 
> I'm not sure that it should be disallowed for the path passed to
> 'abspath()' to be a directory. The associated message seems to be:
> 
> 	- http url fixes suggested by Robey Pointer, and tests
> 
> And committed by Martin.

Well, the changes in this branch don't add to that, that issue already existed.
In fact it's relaxing the existing restrictions so that a trailing slash is at
least acceptable when you're at the root.

So I don't think this concern is a reason to block this merge, as it doesn't
seem to be to do with this branch.

> Anyway, your actual changes seem okay. If you can justify why you needed
> the extra put() tests. The http stuff is just something that we should
> figure out why we want to disallow a trailing slash passed to abspath. I
> realize it isn't the standard way of using it anyway (even if the
> parameter is a directory, you would pass it without the trailing slash).

Ok.  I think I've dealt with your concerns; I'll ask Robert to merge if he's
happy.

-Andrew.