Hi,

Just letting you know I'm not intentionally ignoring this change :) I'm happy to see this support added. Right now, though, we're in feature freeze for RBTools, and are working heavily on Review Board 1.5. So we'll get to this once some of this lightens up, and get it in for the next RBTools release.

IMHO this implementation issue is use "filename" (like ClearCase) instead itemID. This will cause problems with old review requests after change or move item/file/directory.

rbtools should send to ReviewBoard itemID and ReviewBoard should translate itemID to patch each time.

I'm so glad to see PlasticSCM support in ReviewBoard :) This is fantastic news!

This command assume there is GNU diff in system. Documentation of Review Board doesn't talk about GNU diff. Only GNU patch is required.

There is Python difflib module which allow generate unified_diff natively. Only problem is bug with "\ No new line at end of line" described here: http://bugs.python.org/issue2142

I use this method in ClearCase implementation but if we consider the "diff" command is present I will be happy to change difflib to calling "diff" command.

I'm not sure this informations is enough.
I thinking about this a lot and I see some potential problem with file renaming.

Please consider using itemID of object. You can easily transform path <-> itemID using following queries:

cm query "SELECT DISTINCT itemid FROM revisions WHERE itemid='SolvePath(def.txt)'"

cm query "SELECT DISTINCT itemid FROM revisions WHERE itemid=24"
--solvepath=itemid

This may cause problems with behaviour of unified diff adding "\ No newline at end of file" if new line wasn't present. This malformation can break diff viewer on ReviewBoard side. diff -u will handle this situation and IMHO new line should be added.

Maybe better option is make use of --tracking-branch option (which is probably what this branch_diff means) instead passing all to --revision-range

This part is almost redundant to part changenum_diff. My idea is to generate list of files outside the function and pass to one which generate diff based on this list.

Largely, this looks good. Thanks for doing such a good job in keeping with our existing style :) There are a few things that need correcting in the regard, but they're all pretty small.

I'm hoping to build up a unit test suite for post-review down the road. Is there any publicly-accessible server or something we could use for this purpose?

No space between die and (

We're inconsistent in the post-review code, but summaries should begin on the """ line, and if it's just a one-line summary, the terminating """ should also be on that line.

Same with other function docs.

These can be combined into one if statement.

No need for the parens in the return value. The tuple will be implied by the comma.

No blank line here.

You shouldn't need the +, as strings broken up into multiple lines like this will be automatically concatenated.

file is a reserved word, so you should probably use 'f' or something.

No space between function and (

No space between function and (

No need for the \ when using parens for a multi-line conditional.

Also, for our benefit in understanding the code, can you add small comments to each of the bodies in the conditionals describing what these change types are?

Same thing about file here.

Same about the +

No space between function and (

No need for the \

Remove the blank lines between these.

Looks good. Pushed to git. Thanks!