[MERGE] Support for Putty SSH implementation

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Dmitry Vasiliev wrote:
>> The attached patch adds support for Putty's plink SSH implementation. 
>> Also slightly refactored SSH negotiation code and added some tests.
> 
> I like the look of this.  Some comments, mostly about making the intent of some
> code clearer:
> 
> bzrlib/tests/test_ssh_transport.py
> ----------------------------------
> 
> [...]
>> +
>> +    def test_default_vendor(self):
>> +        manager = TestSSHVendorManager()
>> +        self.assertRaises(SSHVendorNotFound, manager.get_vendor, {})
>> +        manager.register_vendor("default", "VENDOR")
>> +        self.assertEqual(manager.get_vendor({}), "VENDOR")
> 
> An explicit comment or docstring in the test stating that "default" as a vendor
> name is treated specially would be good.  It's not hard to guess, but making the
> intent explicit is good, in case someone guesses incorrectly :)
> 
>> +    def test_cached_vendor(self):
>> +        manager = TestSSHVendorManager()
>> +        self.assertRaises(SSHVendorNotFound, manager.get_vendor, {})
>> +        manager.register_vendor("vendor", "VENDOR")
>> +        self.assertRaises(SSHVendorNotFound, manager.get_vendor, {})
>> +        self.assertEqual(manager.get_vendor({"BZR_SSH": "vendor"}), "VENDOR")
>> +        self.assertEqual(manager.get_vendor({}), "VENDOR")
> 
> The intented behaviour here isn't immediately obvious to me.  Why should
> manager.get_vendor({"BZR_SSH": "vendor"}) change the result of a later
> manager.get_vendor({}) call?
> 
> Basically, I'd like to see a comment (or docstring) in this test explain the
> intent.  Something like "Once SSHVendorManager.get_vendor successfully returns
> a vendor, later invocations will return the same vendor, regardless of changes
> to the environment.", but that doesn't sound quite right to me.  How you would
> explain this behaviour, and the reason for it?
> 
> I'd also be tempted to use something other than "VENDOR" here, e.g. "vendor =
> object()", so that you can add "self.assertIdentical(vendor,
> manager.get_vendor(...))", i.e. demonstrate that it's the exact same object.
> (You could technically do it with the string object, but string identity is
> mildly confusing due to interning, so using an object would be better.)
> 
>> +    def test_vendor_getting_methods_precedence(self):
>> +        manager = TestSSHVendorManager()
>> +        self.assertRaises(SSHVendorNotFound, manager.get_vendor, {})
>> +
>> +        manager.register_vendor("default", "DEFAULT")
>> +        self.assertEqual(manager.get_vendor({}), "DEFAULT")
>> +
>> +        manager.ssh_vendor = None
>> +        manager.set_ssh_version_string("OpenSSH")
>> +        self.assertIsInstance(manager.get_vendor({}), OpenSSHSubprocessVendor)
>> +
>> +        manager.ssh_vendor = None
>> +        manager.register_vendor("vendor", "VENDOR")
>> +        self.assertEqual(manager.get_vendor({"BZR_SSH": "vendor"}), "VENDOR")
>> +
>> +        self.assertEqual(manager.get_vendor({}), "VENDOR")
> 
> This test could use some comments as well.  What's the "precendence" you're
> testing?  Peeking at the implementation, I guess this is testing the order that
> get_vendor attempts to choose a vendor, i.e. by environment variable, by
> inspection, by default, and finally by giving up.  Say so explicitly in the test
> (including naming the stages in the test)!
> 
> Also, the purpose of the "manager.ssh_vendor = None" lines probably deserves a
> comment explaining that this is clearing the cache.  Perhaps it would be clearer
> to create a fresh TestSSHVendorManager each time, although you'd have to repeat
> some registrations, so perhaps not.
> 
> I'm mildly surprised that "ssh_vendor" isn't "_ssh_vendor", it doesn't look like
> something that most code ought to be mucking with.
> 
> NEWS
> ----
> 
>>    IMPROVEMENTS:
>>  
>> +    * Added support for Putty's SSH implementation. (Dmitry Vasiliev)
>> +
> 
> You're being overly modest.  You've done a nice cleanup of SSH vendor
> registration too! :)   (although mentioning that probably belongs under the
> INTERNALS heading).
> 
> 
> bzrlib/errors.py
> ----------------
> 
> [...]
>> +class SSHVendorNotFound(BzrError):
>> +
>> +    _fmt = "Don't know how to handle SSH connections. Please set BZR_SSH environment variable."
> 
> Lines should have no more than 79 characters (PEP 8).
> 
> 
> bzrlib/transport/ssh.py
> ------------------------
> 
> [...]
>>                             SocketConnectionError,
>>                             TransportError,
>>                             UnknownSSH,
>> +                           SSHVendorNotFound,
>>                             )
> 
> I don't think this convention is documented anywhere (I don't see it in HACKING
> or PEP 8), but the other names being imported from bzrlib.errors there are in
> alphabetical order; I think it would be good to keep that ordering.
> 

IIRC, PEP8 says that imports should be in sorted order. I think we just
took it such that if

import bar
import foo

is in sorted order, then

from x import (
  bar,
  foo,
  )

should also be in sorted order.

John
=:->



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFonlbJdeBCYSNAAMRAkJsAKCimE5p+pzYvxAbQ9EDnNxDqJ/7IACfZT1C
NoGXd15eYYWKtAgsf6ufmT8=
=JYI7
-----END PGP SIGNATURE-----