[MERGE] Fix bug 146715: bzr+ssh:// and sftp:// transports should not assume port-not-specified means port 22

Andrew Bennetts andrew at canonical.com
Mon Oct 8 07:25:04 BST 2007


Martin Pool wrote:
> Martin Pool has voted comment.
> Status is now: Semi-approved
> Comment:
> +class SSHPortTestMixin(object):
> 
> (comment) It bothers me that we have both mixin and scenario
> tests, though for your particular case using this is simpler.

Yeah.  I think perhaps we can make the scenario infrastructure a bit nicer for
simple cases like this, but it doesn't seem like a big priority.

> It seems strange to me that ssh and sftp need to be treated differently 
> in this regard.  It's true that they are the places where the 
> unspecified port is most likely to be different to the protocol standard 
> port.  But why not in every case just keep unspecified as None and let 
> the transport handle this?

That's a good question.  I was originally planning to fix it that way.

It's partly because I don't think any of the other protocols have this concern
at all; SSH is special because there's a particular client that's used almost
universally, *and* it's fairly common to configure aliases in that client that
specify particular combinations of host, port, authentication, tunnelling magic,
and so on.  As far as I know none of the other protocols we support have
anything like this as a concern.  “http://example.com/foo” *always* means port
80.

The other reason is that I didn't want to examine all the callsites of
_split_url and _unsplit_url to figure out how best to deal with the extra
possible state of port, especially given that it wouldn't be fixing any actual
bug that I know of.  It might be nice, but doing that while preserving the
behaviour of e.g.
“get_transport('http://example.com/')._reuse_for('http://example.com:80/')” (or
deciding that it would be ok not to preserve that), and making sure that we
preserved compatibility for 3rd-party subclasses of ConnectedTransport (or
deciding that it would be ok not to) seemed like a lot of work for minimal
benefit.

So, I'm pretty sure my change is correct in so far as it goes: if we want to be
able to delegate the definition of the default port to OpenSSH, then in bzrlib
we need to remove the 'default_port=22' parameters from the protocol
registrations.  Otherwise we'll erroneously assume that “bzr+ssh://host/” and
“bzr+ssh://host:22/” are equivalent (and thus risk problems like a _reuse_for
call returning an inappropriate transport).  And as the tests I add demonstrate,
this is enough to fix bug 146715.

I do think it might nice if we also tracked whether an port was explicitly
specified for all transports, but at the moment that seems to be extra work for
no benefit.  Perhaps we ought to file a bug about it?  (although without a
concrete use-case to motivate it I suspect it would languish indefinitely)

-Andrew.




More information about the bazaar mailing list