Looking over the plan you've laid out in your comments, no issues come to mind.

This diff is including your changes for 'rb diff', which I assume means you've based your branch for 'rb patch' off of your other branch. To fix this, you should run post-review with '--parent=<name_of_rb_diff_branch>' when you post your next diff.

It appears that the 'rb diff' changes were shown in the latest diff. Make sure to use '--parent=rbdiff' when updating the request so the changes are hidden.

Imports should be order like this:

Standard Library imports (e.g. os, sys, etc.)
===BLANK LINE===
Third Party imports
===BLANK LINE===
Local Imports (e.g. 'rbtools...')

Also, imports should be order alphabetically within the three categories.

This is imported twice.

This should be removed when not debugging.

docstrings should contain a one line summary, followed by a blank line, and then further description.

You'll want to get rid of these options. The user should be passing the review request id  and diff revision as arguments, not options.

Right now the command would be called like this:
rb patch -r <request_id> [--diff-revision=<diff_revision>]

We want to call the command like this:
rb patch <request_id> [<diff_revision>]

You shouldn't need these options for patch.

This option isn't required. Although, maybe the command should give the option of creating a change list (A different, but similar option).

This option isn't needed.

Clear out the options which should be removed, as you remove them.

This won't be needed for patch.

This red block means there is trailing whitespace. You should view the diff before publishing and check for these.

You never initialize self.tool, so this will fail.

Yeah, I've been trying to decide how I want to handle this.

ridorigcwd isn't an option. It looks like you've prepended 'self.options.rid' to the line.

Also, this variable (or origcwd) are never referenced again.

The comment explaining why this is set should probably be restored.

These might be better off factored out, and have server_url passed into the method.

You don't need this pass here.

Same thing as my previous comment on docstrings.

You should consolidate your calls to things like get_review_requests, and git_item. Each of these calls causes a HTTP request to the server, which is just returning the same data in this case.

\ isn't needed to continue on the next line when you are inside an open parentheses. 

This return statement is a little cluttered. Could you try and break this out and make it a little more readable.

'with' isn't introduced until python 2.5, and we need to support 2.4, so you'll have to change this.

Also, rbtools.utils.filesystem has functions for creating temporary files etc. You should use those instead of just writing in the working directory.

You'll want to use rbtools.utils.process.execute here instead of executing the command yourself.

for examples, take a look at the ./rbtools/clients/*.py files. 

You'll need the tool since you'll be passing the patch to it. The actual code for applying your patch to the tree should live inside the clients.

The base class will handle the default of calling out to 'patch', but sub classes can override this and use things such as 'git apply'

The patch should be removed, but you'll want to use the temp files stuff I mentioned previously.

Trailing whitespace.

A more concise way to write this is:

p_num = p or self._get_p_number(patch_file)

Leftover debug output?

This should check the return value, no?

If statements in python don't need parentheses. Also, a good way to check for falsiness is "if not p:"

I think a much better way to do this would be to make execute() return the process return code, and check if it's non-zero.

I don't think any of these options are necessary

Some trailing whitespace.

I'm not sure if get_root should be in the base class (yet), but this should definitely be moved out into a helper function.

No parentheses needed, and can check using "not"

Leftover debug stuff

We should definitely delete it, especially in the success case.

Do I have all the ingredients/args to derive the pX number here?

Will fix in next submit

will fix excess space

Should I make this a helper function?!

I made a helper in base class to get cookie, is this okay?

When do you think you'll be ready to take this out of WIP so that we can do a deep review?

Alphabetical:

import re
import sys


 (I had think about it for a sec to make sure, haha)

Should this and other occurrences of this start on the 1st line:

"""Returns...

?

This is looking really good - only two nits this time. One more pass, and I think this has my ship-it.

r before s.

This might look better:

diffs = self.root_resource \
            .get_review_requests() \
            .get_item(request_id) \
            .get_diffs()

I think I'm good with this. Thanks John!

Ship It!