"serve --allow-writes" allows more than you might think

so, only changes in option name required to fix this bug?

Yes.

Isn't this option also used to enable writes when bzr serve is used over ssh ?

Could be. I stuck a branch up with the potential changes for this bug, I am not all all famililiar with the bzr code at all yet, but there appear to be some necessary changes for this around ssh. In contrib/bzr_ssh_path_limiter but I am not quite sure what any of it does yet.

Jelmer: yes, --allow-writes is used over bzr+ssh.

I presume that --allow-writes will also have to be supported for a time, therefore should --allow-anonymous-write-access be implemented and presented in the help, but --allow-writes also be accepted too but not documented? Or does the documentation (bzr help + docs) need to convey this change?

As discussed on https://code.edge.launchpad.net/~jml/bzr/allow-writes-change-84659/+merge/8222, renaming this option is not acceptable, since it breaks backwards compatibility. Many other solutions to the root problem are discussed on that page.

Martin suggested a fix on IRC. If serve is called without --inet and with --allow-writes, issue a warning. This is easy enough, just call note() in cmd_serve or something that cmd_serve calls.

Here is a patch that displays a warning (using 'note') when --allow-writes is used without --inet.

2009/7/20 Danny van Heumen <email address hidden>:
> Here is a patch that displays a warning (using 'note') when --allow-
> writes is used without --inet.
>
> ** Attachment added: "Patch that shows a warning (note) as suggested by Martin."
>   http://launchpadlibrarian.net/29255904/bzr.dev-bug-84659.patch

Thanks, that looks good Danny, with a few tweaks:

 * The string should be wrapped as it makes the line too long.
 * The indenting should be 4 spaces
 * You should use the trace.warning function not note
 * Ideally there would also be a test for it, along the lines of those
already in blackbox.test_serve

Could you please see about pushing a branch containing your fix to
Launchpad and then propose it for merging into trunk, then we can do a
proper review:

When this is merged it should be mentioned in NEWS as closing this bug.

--
Martin <http://launchpad.net/~mbp/>

I'm not going to be working on this any time soon. I'd really appreciate it if someone else could land Danny's patch though.

I've applied Danny's patch and made the changes suggested by Martin and pushed it to lp:~doxxx/bzr/84659-allow-writes.

Nevermind, I see his changes have been accepted.

Coming back to this, I have reason /not/ to emit a warning.

We now support --http to serve http from 'bzr serve'. Its arguably sane for people to run this up behind apache with apache doing the auth, and emitting a warning there would simply be annoying.

I think the help docs on the option are really the sensible total feedback we should give on this. Dangerous options exist, but its reasonable to expect users to find out about options via the docs, where we can warn without emitting warnings in usual circumstances.

Since Robert's fix is merged, should the status be set to 'Fix Committed' or something? ('In Progress' is not really the current state ...)

>Since Robert's fix is merged, should the status be set to 'Fix Committed' or something? ('In Progress' is not really the current state ...)

That's correct. I fixed it.