This change shouldn't be checked in as-is; I haven't actually tested to make sure I didn't break something. I'm mostly floating this for suggestions.

post-review calls p4 a number of times. Currently each invocation has a separate "p4" string. This isn't a very good practice as it makes it difficult to point this at a specific p4 binary instead of just whatever is "p4" in your path or to add global p4 options that should be used on all commands.

This is also true of all the other scm tool commands as well and should be fixed, but I don't have environments for testing all of those so I'm not currently trying to change them.

There are a number of different ways this could be done, this was just the most straightforward. It might make sense for each command string to belong to the relevant SCMClient object instead. If _run_p4 mashalled output function is going to be used for all p4 stuff, the definition could go there... but I don't know if it makes sense to add a _run_blah function for all the other scm tools which need this change too. 

Theoretically this could also be configurable via a command line option and/or config file like all the other parameters people seem to be adding. That would actually be fairly useful for us (since we can slap our tool locations in the wrapper script we use instead of hardcoding them). But the script options are starting to sprawl as it is.

This change vaguely touches on the issue that at some point someone should probably make a decision about whether post-review should use normal p4 commands and parse the output, or use p4 -G and the marshalled python output, and make everything work the same way. I have my opinions on that (parsing text is lame but p4 -G is lamer) but I won't get into that.

Okay, that was way too much text for what is mostly a trivial change. ;)

None yet.