[MERGE] Calculate remote path relative to the shared medium in _SmartClient

[Andrew Bennetts]

John Arbash Meinel wrote:
> John Arbash Meinel has voted comment.
> Status is now: Semi-approved
> Comment:
> As near as I can tell, the only strictly needed part is this:
> -        return unescape(urlparse(transport.base)[2]).encode('utf8')
> +        if self._shared_connection.base.startswith('bzr+http://'):
> +            medium_base = self._shared_connection.base
> +        else:
> +            medium_base = urlutils.join(self._shared_connection.base, '/')
> +
> +        return urlutils.relative_url(medium_base, 
> transport.base).encode('utf8')
>
> I don't really understand why you need that "startswith()" check.

The short answer is because it doesn't work without it ;)

Without this, an attempt to do “bzr log bzr+http://localhost/repo/branch” will
attempt to request “BzrDir.open repo/branch/” at
http://localhost/repo/branch/, rather than “BzrDir.open .”.

I see now that the automated tests don't fail without it, though.  Hmm.  I'll
see if I can do something about that, possibly by replacing the startswith check
with a call to the _shared_connection so that a test dummy can override it.

I'm not totally happy with the abstractions here.  The sharing of path
management duties between the _SmartClient, the medium, and the
_SharedConnection seems overly complicated.  _SharedConnection didn't exist when
the smart medium and smart client layers first existed, but now that it does I
feel like we have too many layers here.

> The problem with leaving it POSTing to the original URL is when you give it 
> a filename at the end of a branch.
>
> Specifically: bzr log http://bazaar-vcs.org/bzr/bzr.dev/bzr
>
> Should give you the log of just the changes to the 'bzr' file. But POSTing 
> to http://bazaar-vcs.org/bzr/bzr.dev/bzr/.bzr/smart seems a little wrong 
> (considering you are posting to a URL which is underneath a file.)

It might seem a bit weird, but it's not invalid and also not a problem in
practice, so I don't see any reason to go out of our way to avoid it.

> I wouldn't think handling the connection would be a strict problem, since 
> it should already be handled in the case that an HTTPTransport is cloned 
> from itself.

I'm not sure what you mean here.

> In my testing, to get bzr+http:// to work I only needed the client to be 
> updated (I happen to have both of your patches, but I'm guessing this is 
> the important one for the client).

Right.  On the client side only this patch matters.

> === modified file 'bzrlib/transport/remote.py'
> --- bzrlib/transport/remote.py  2007-11-19 13:44:25 +0000
> +++ bzrlib/transport/remote.py  2007-12-13 06:29:54 +0000
> @@ -102,7 +102,8 @@
>                      trace.mutter('hpss: Built a new medium: %s',
>                                   medium.__class__.__name__)
>              self._shared_connection = transport._SharedConnection(medium,
> - credentials)
> + credentials,
> + self.base)
>
>          if _client is None:
>              self._client = client._SmartClient(self.get_shared_medium())
> @@ -496,7 +497,7 @@
>          """After connecting, HTTP Transport only deals in relative 
> URLs."""
>          # Adjust the relpath based on which URL this smart transport is
>          # connected to.
> -        http_base = urlutils.normalize_url(self._http_transport.base)
> +        http_base = urlutils.normalize_url(self.get_smart_medium().base)
>          url = urlutils.join(self.base[len('bzr+'):], relpath)
>          url = urlutils.normalize_url(url)
>          return urlutils.relative_url(http_base, url)
>
> ^- I also don't quite understand why you prefer "get_smart_medium().base". 
> Especially since you had to switch _SharedConnection to start having a 
> .base attribute.

Putting a .base on _SharedConnection is necessary anyway, for the
_SmartClient.remote_path_for_transport change (which is used by
RemoteBzrDir._path_for_remote_call).

I prefer get_smart_medium() to _http_transport here because it's the more
appropriate abstraction, I think: the medium is what smart request is issued on,
and so that's the object that the path needs to be adjusted for.

-Andrew.