[MERGE] Support for Putty SSH implementation

[Dmitry Vasiliev]

Andrew Bennetts wrote:
> Dmitry Vasiliev wrote:
>> +    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 :)

I think it would be even better to add manager.register_default_vendor() method.

>> +    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?

Actually, I didn't change the behaviour in this case. I think the vendor
caching  can speed up ssh vendor lookup for bzr as an utility but it can be
slightly bad idea for bzrlib as a library. The one possibly solution can be to
add clear_cache() method to the manager interface.

> 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.)

Good idea, I'll change it.

>> +    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)!

Ok, I'll add some comments to 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.

Agreed, I'll add clear_cache() method and rename 'ssh_vendor' into '_ssh_vendor'.

>> +    * 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).

No problem for me to add some note under the INTERNALS. :-)

> 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).

It seems the errors.py doesn't strictly follow this rule. ;-)
I'll wrap all long lines in the error.py.

> 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.

Ok, I'll fix it.

[SKIP]
>> +class SSHVendorManager(object):
>> +    """Manager for manage SSH vendors."""
> 
> Hmm, it's tempting to making this a bzrlib.registry.Registry, until you realise
> that the interface doesn't quite fit; instead of "get(key)" this essentially
> just has "get()", which chooses the best registered vendor for the current
> environment (I mean environment in the broader sense, not just the "os.environ"
> sense).
> 
> It may be worth adding a brief comment highlighting this fundamental difference,
> to stop people (like me) from chasing foolish consistency by trying reuse
> Registry here :)

Ok

> [...]
>> +    def _get_vendor_by_environment(self, environment=None):
[SKIP]
> 
> A comment in English explaining the intent would be good.  Even though it's not
> hard to reverse-engineer the intent from the current implementation, code
> evolves, and intent that seemed clear will get obscured unless it's stated
> explicitly.
> 
> So, add a comment saying "If the user has set the BZR_SSH environment variable,
> use that to choose the vendor." 

Agreed

>> +    def _get_vendor_by_version_string(self, version):
[SKIP]
> A docstring mentioning that "version" is expected to be the output of executing
> a command like 'ssh -V' would be good.
> 
>> +    def get_vendor(self, environment=None):
[SKIP]
> The docstring for get_vendor ought to mention that it can raise
> SSHVendorNotFound or UnknownSSH.

I'll add docstrings.

-- 
Dmitry Vasiliev (dima at hlabs.spb.ru)
     http://hlabs.spb.ru