[MERGE] Fix the dbus plugin by changing the SmartServer hooks API.

[Robert Collins]

On Thu, 2007-07-05 at 02:47 -0400, Martin Pool wrote:
> Martin Pool has voted +1 (conditional).
> Status is now: Conditionally approved
> Comment:
> > interally
> 
> Internally
> 
> +        # There are three interesting urls:
> +        # The URL the server can be contacted on.
> +        # The URL that a commit done on the same machine as the server 
> will
> +        # have within the servers space.
> +        # The URL that will be given to other hooks in the same process 
> -
> +        # The URL of the backing transport itself.
> 
> That seems to be four URLs?  I guess the last two are the same?  Please 
> make it clearer.

Theres a hyphen and a bad capital on the 'The' of the last line. It
should read:
# The URL that will be given to other hooks in the same process -
# the URL of the backing transport itself.


> The comment is good otherwise, and much needed here - probably would be 
> even better with an example, or even just saying what scheme they 
> typically use: as i understand it, that would be bzr+something, file, 
> and chroot?

added:

# The URL the server can be contacted on. (e.g. bzr://host/)
# The URL that a commit done on the same machine as the server will
# have within the servers space. (e.g. file:///home/user/source)
# The URL that will be given to other hooks in the same process -
# the URL of the backing transport itself. (e.g. chroot+:///)


> +        backing_urls = [self.backing_transport.base]
> +        try:
> +            backing_urls.append(self.backing_transport.external_url())
> 
> If they are to be used in different ways is it really a good idea to 
> just put them into a list.

# The latter two urls are different aliases to the servers url,
# so we group those in a list - as there might be more aliases 
# in the future.

>           Hooks.__init__(self)
>           # Introduced in 0.16:
>           # invoked whenever the server starts serving a directory.
> -        # The api signature is (backing url, public url).
> +        # The api signature is (backing urls, public url).
> 
> If you need to change the hook arguments I would kind of prefer to 
> change to giving it an object, to make future extension cleaner, as we 
> did with push/pull.  But I realize this may be a larger change than you 
> can do today.

Its a good point but yeah I am a bit short on time today :).

> +    def external_url(self):
> 
> I wonder if this needs a more explicit name?  Both to flag that this is 
> a potentially dangerous thing to do in the server or a test case, and 
> also to  just make it more clear?  externally_accessible_url?  not much 
> better maybe.

I couldn't think of anything that wasn't
<===================================================> long that was
appreciably better than 'external_url'. At least 'external_url' does
indicate that its not an internal thing but should be for external
use :).

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070705/c2cebe35/attachment.pgp