[MERGE][0.91][Bug 133965] PathNotChild, port mismatch with "bzr info" for bzr:// smartserver

[Vincent Ladeuil]

>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> Vincent Ladeuil wrote:
    Andrew> [...]
    Andrew> -def register_transport_proto(prefix, help=None, info=None):
    Andrew> -    transport_list_registry.register_transport(prefix, help, info)
    Andrew> +def register_transport_proto(prefix, help=None, info=None, default_port=None):
    Andrew> +    transport_list_registry.register_transport(prefix, help, default_port)
    >> 
    >> 
    >> Using transport_list_registry is not clean:
    >> - what if ConnectedTransport need to be used without it ?

    Andrew> I'm not sure what your concern here is.
    Andrew> ConnectedTransport is defined in the same module that
    Andrew> initialises transport_list_registry, so
    Andrew> transport_list_registry is always available to
    Andrew> ConnectedTransport.  So I don't see a problem here.

It feels weird to make a transport knows about its own registry,
I would prefer a solution where the registry provides the default
port to the transport class at construction time or if each
ConnectedTransport class declares its own default_port as a class
variable (I think only http may cause a problem here as the same
class is used for http and https, but splitting them may not be
hard).

It feels also weird to have a parameter that only some transports
will use while others have no need for it.

I thought you tried the class attribute approach in a previous
version why did you change your mind ?

    >> - at least use _get_protocol_handlers which is the API to
    >> access it.

    Andrew> I don't understand why this is any better than
    Andrew> accessing transport_list_registry directly,

Coherence.

    Andrew> and there's no docstring or other comments to explain
    Andrew> what the intent of this indirection is.

I agree with you here :)

<snip/>

    >> And here.
    >> 
    >> I realized you didn't write some of these uses, but there are some
    >> inconsistencies here.

    Andrew> The inconsistency as far as I can tell is that
    Andrew> _get_protocol_handlers exists at all ;)

I'm fine with it if you get rid of them :)

    Andrew> I'd love to know what _get_protocol_handlers is meant
    Andrew> to achieve.  _{get,set,clear}_protocol_handlers seems
    Andrew> to be unused, except for their tests.

I guess you pointed the right reason: tests :)

    Andrew> So I'd rather not make the change you suggest; it
    Andrew> makes things more confusing for no benefit that I can
    Andrew> see.  In fact, I think we should remove the
    Andrew> _*_protocol_handler functions entirely, as they don't
    Andrew> seem to be of any use.

I agree with that.

    Andrew> Can you explain why you think we should use those
    Andrew> functions rather than directly accessing
    Andrew> transport_list_registry?

Be cause I presumed they were introduced for a good reason and
not used consistently for a bad one. But I've no problem with
doing the opposite :)

In summary, I agree with you about _[sg]et_protocol_handlers,
will feel better with default_port being a class or instance
attribute.

      Vincent