[MERGE][0.90][Bug 133965] PathNotChild, port mismatch with "bzr info" for bzr:// smartserver

[Vincent Ladeuil]

Sorry to come this late in the game (still ~1500 emails in my backlog).

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> James Westby wrote:
    >> On (24/08/07 06:50), Robert Collins wrote:
    >>> This is clearly wrong.
    >>> 
    >>> The right way to handle this is to use the default port number if none
    >>> has been explicitly provided.
    >>> 
    >>> E.g. to do a comparison with http, you need to supply 80, for https 443,
    >>> etc.

This has not been the case so far and IMHO should be considered
separately (i.e. let's fix the bug and discuss that afterward).

All other connected transports set the port at connection time or
rely on lower levels to do it when no port is specified. Such
lower levels do not exist for bzr obviously.

AIUI, bzr should never encounter two urls, one with no port
specified and one with the default port explicitly set, except
for some pathological cases like:

  bzr diff http://host/path http://host:80/path/subpath

I.e. internally bzr should never set the port if the user did not
do it himself. This assumption have been verified so far for all
other schemes.

    >> 
    >> Hi,
    >> 
    >> I'm not really sure how to proceed, so I have recorded my understanding
    >> in the hope that it will help someone to see the light.
    >> 
    >> Ok, for explicit ports in the url everything is fine, so we only need to
    >> consider default ports.
    >> 
    >> When the user provides a url they do not put a port in it, and so
    >> _split_url returns None for the port.
    >> 
    >> For most transports self._port is None in the default case, and so this
    >> works out fine.
    >> 
    >> For bzr:// if the port is None when the transport is created then
    >> self._port is set to BZR_DEFAULT_PORT. This then means that the url that
    >> was used to create the transport is no longer a child of it, as the
    >> ports differ, giving rise to the bug.
    >> 

Yes, my error. _port should have been left untouched and set only
when needed: at connection time.

    >> Why does the bzr:// transport differ here in that it explicitly sets
    >> its port to the default?

History. 

When 4155 was accepted as the default port for 'bzr' the simplest
way to handle it was to set it in the constructor if none was
specified.

During the ConnectedTransport refactoring I was bitten by that
approach and tried to push the setting lower in the stack. 

I then defined _split_url as a method instead of a function in
urlutils so that RemoteTCPTransport can override it, but further
refactoring allowed RemoteTCPTransport to set the port in
_build_medium only (closer to connection time).

I leave the method static because there was no attribute accesses
but I wanted to keep the overriding possible.

    >> 
    >> Is the best way to change all transports to explicitly set the default
    >> port? Can the default be dropped from bzr://?

Well, I'd prefer to avoid putting /etc/services into bzr, but
users may be surprised that we don't treat 'port not specified'
and 'default port specified' as strictly equivalent.

But again, I'd consider that a feature request and separate it
from this bug.

    >> 
    >> I'm not going to have time to complete this tonight, so if someone can
    >> come up with a fix I would appreciate it.

John's fix is the right one I think, it's coherent with how the
default port is handled by other schemes.

    >> 
    >> Thanks,
    >> 
    >> James
    >> 


    john> Well, it seems like this would be the simple fix:
    john> === modified file 'bzrlib/transport/remote.py'
    john> --- bzrlib/transport/remote.py  2007-07-30 14:36:04 +0000
    john> +++ bzrlib/transport/remote.py  2007-08-24 14:20:41 +0000
    john> @@ -443,9 +443,10 @@

    john>      def _build_medium(self):
    john>          assert self.base.startswith('bzr://')
    john> -        if self._port is None:
    john> -            self._port = BZR_DEFAULT_PORT
    john> -        return medium.SmartTCPClientMedium(self._host, self._port), None
    john> +        port = self._port
    john> +        if port is None:
    john> +            port = BZR_DEFAULT_PORT
    john> +        return medium.SmartTCPClientMedium(self._host, port), None


    john>  class RemoteSSHTransport(RemoteTransport):

    john> Basically, just create a local variable to hold the real port, and don't change
    john> the self._port value.

I agree that's the right and minimal approach.

    john> Arguably the _split_url and _unsplit_url could be moved
    john> onto the base Transport class rather than the derived
    john> ConnectedTransport. I don't remember exactly why
    john> Vincent wanted the split, but I think there was a
    john> reason.

Host, port and credentials are needed for connected transports only,
but Transport can have a simpler definition for split and unsplit
if needed.

  Vila