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

[Martin Pool]

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.

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?

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

          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.

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



For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1183603227.15211.113.camel%40localhost.localdomain%3E