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

[John Arbash Meinel]

Andrew Bennetts wrote:
> 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.

Your comment was that changing the post location caused difficulties in
handling the "connection", but it didn't seem that difficult. But I
wasn't the one arm-deep in it, so if you feel it was not worth the
effort, that is ok.

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

Actually, I mean that with this patch to the client, the *old* server
code worked just fine.


...

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

In my head, a medium doesn't necessarily have a base...

Anyway, nothing was really bad with your patch. And it makes things a
lot better. I really want it to land in 1.1.

John
=:->