Improvements to post-review error-handling with Perforce

Review Request #1886 — Created Nov. 1, 2010 and submitted




A handful of changes to improve error handling in post-review.

* Die with an error when the provided Perforce changelist does not exist
* Die with an error if diffs are empty
* Die with an error when there are no changes in a Perforce changelist. Fixes issue 832.
* Default to the default changelist when no Perforce changelist is provided. Fixes issue 1191.

This corresponds to the top four commits at Note that 2bd8897b133aa81e2b32 changes the default of running "post-review" in a Perforce client to uploading the default change - the other changes should not introduce any new behavior.
These changes have been in use for a while.
  1. I haven't yet looked at the code, but I think your first bullet point is issue 1808.
    1. I just noticed that I'm missing one patch that fixes the "default to default changelist" change - what's there now breaks path-based reviews. I'll add a new diff when I get home that fixes that.
  1. Looks good. One small nit, below.
  2. rbtools/ (Diff revision 1)
    While the two SHOULD be the same, "\d+ - no such changelist" or just an on "no such changelist" would seem to be more robust and involve less string work.
Review request changed

Change Summary:

Simplify nonexistant changelist detection in response to code review. Also include a missing change that makes defaulting to the "default" Perforce change more robust, and prints the review URL with a trailing slash (to avoid a useless redirect).

I couldn't find on the contributor guide what to do when I've got a "Ship It!" - should I submit a separate pull request on GitHub with a link to this review, or will you pull? Should I squash my commits?


Revision 2 (+18 -4)

Show changes

  1. As the reporter of issue 1191, I'd strongly prefer if post-review with no arguments printed a usage summary instead of creating a review request from the default changeset.
    1. But, for all of the other SCMs, like SVN, post-review with no arguments posts a review.
    2. Good point.  I'm not a fan of that behavior (although it makes sense for SVN where there traditionally isn't a changeset to specify), but okay.
  1. Thanks for this! I've pushed a squashed version as 65eec92.