[MERGE] Give nicer errors for some write operations on read-only transports.

[James Westby]

On (03/09/07 05:30), Martin Pool wrote:
> Martin Pool has voted resubmit.
> Status is now: Resubmit
> Comment:
> Thanks for doing this, there is at least one open bug report complaining 
> about the ugly message:
> https://bugs.launchpad.net/bzr/+bug/129701

Ah, thanks. You made me realise I completely forgot NEWS in this patch,
so I will add the reference there.

>
> >Perhaps send could use the last log message as the message to start 
> editing,
>> or at least have an option to. Any comments?
>
> I think that would be good.  Or maybe it should even show all the unmerged 
> messages.  Fairly often my last message before I submit something is just 
> 'update docs' and so not very interesting.  If it's useful some of the time 
> it's probably a win as deleting unwanted text is fast compared to searching 
> for the right thing to include.

I agree. However this may be too much for many, many changes, but I say
better to have it and then restrict it later if need be.

The other thing is how much work would have to be done to extract all
log messages without an extra pass at the end.

>
> +def ensure_transport_writable(transport):
> +    """Ensure that the transport is writable.
> +
> +    If it is not (transport.is_readonly() is False) then it raises
> +    errors.ReadonlyTransport, otherwise it does nothing.
> +
> +    :param transport: the transport to test.
> +    :return: None
> +    :raise errors.ReadonlyTransport: if the transport is readonly.
> +    """
> +    if transport.is_readonly():
> +      raise errors.ReadonlyTransport(transport)
> +
> +
>
> I think this should be a Transport method rather than a function.

It was first a Transport method, but I didn't like that too much, as it
required Transport to throw a user error, and to reference a help topic.
The comments in the bug you cite suggest mentioning bind as well, which
also seems odd for a Transport method. I guess these are properties of
the exception, so if Transport can throw a user error it seems ok. I'll
make the change.

>
> We normally use the word 'ensure' to mean 'make it so', and
> 'require' for 'raise an error'.  (That should maybe be in the
> developer guide, if other people agree it's the standard.)

That makes sense to me. Should I add it to the docs in this patch, or
would you prefer it separately so that it can be considered?

> Aside from that it looks good, thanks!

This was triggered by seeing it one too many times from a user on IRC.
Once they see the traceback they have no idea how to react. This goes
for the smart server as well which is quite traceback happy (as
evidenced by the number of bugs in lp about this), but I would not be so
comfortable making the changes for that.

Thanks,

James

-- 
  James Westby   --    GPG Key ID: B577FE13    --     http://jameswestby.net/
  seccure key - (3+)k7|M*edCX/.A:n*N!>|&7U.L#9E)Tu)T0>AM - secp256r1/nistp256