[MERGE] Calculate remote path relative to the shared medium in _SmartClient
John Arbash Meinel
john at arbash-meinel.com
Fri Jan 4 14:35:51 GMT 2008
Andrew Bennetts wrote:
> John Arbash Meinel wrote:
>> John Arbash Meinel has voted comment.
>> Status is now: Semi-approved
>> As near as I can tell, the only strictly needed part is this:
>> - return unescape(urlparse(transport.base)).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,
>> 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
> 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.
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.
More information about the bazaar