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

[Andrew Bennetts]

Vincent Ladeuil wrote:
> Sorry to come this late in the game (still ~1500 emails in my backlog).

Better late than never!

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

This is true, but it would be nice if our APIs just did the Right Thing here.
We can't predict what random plugins might do, for instance.

I agree this is tangential to fixing the release critical bug though.

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

I disagree (or perhaps I misunderstand what you mean?).  _port should never
change during the lifetime of a Transport — it would be confusing if t.relpath
worked before a transport connected, and stopped working afterwards (or vice
versa).  The _port attribute should be as immutable as the base attribute.

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

Unfortunately as I realised it isn't reasonable to override per transport.  You
could cause nasty bugs with _reuse_for with the right combination of
circumstances if _split_url varied by transport.

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

Well, I think my fix is better for bzr.dev.  I can see the case for John's fix
for 0.90 instead of mine (it's simpler and thus safer), but I'll leave the
release manager to make that judgement.

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

Well, the URL syntax isn't going to vary for us by scheme is it?  Some parts
might not apply to all protocols (e.g. there's no user@ for bzr://, and no host
or port for file://), but that doesn't change the parsing AFAICS.  So I think
split/unsplit should global functions, and it's up the individual transports to
explicitly raise errors on obviously impossible urls (e.g. MemoryTransport could
assert that host is None).

-Andrew.