The post-review support for Plastic SCM

Review Request #1396 — Created Feb. 8, 2010 and submitted




The post-review side of the Plastic SCM support.  Supports changeset IDs and branch specifications as the diff argument.
Tested against the latest version of Plastic SCM.
  1. 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.
  2. rbtools/ (Diff revision 3)
    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.
  1. I'm so glad to see PlasticSCM support in ReviewBoard :) This is fantastic news!
  2. rbtools/ (Diff revision 3)
    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:
    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.
    1. We basically require GNU diff for other things, so it's okay. It's the most compliant diff tool out there. Though, at some point, we'll likely have our own for this purpose.
      Perforce requires GNU diff, and post-review will check for it, so I have no problem requiring this right now.
    2. If You say so :) I will make good use of GNUDiff in the next version of ClearCase implementation. This will dramatically simplify implementation.
      Thank You for answer :)
  3. rbtools/ (Diff revision 3)
    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"
    1. I see revision id is the best choice of information about particular version :)
    2. There are two problems with this approach:
      1) It's an extra couple of calls to the server, and the code is already slow enough!
      2) The user ability to use 'cm query' is not enabled by default, so is something that would cause reviewboard to break mysteriously until it was set.
      Having said that, I'm not set against the idea (especially if it makes diffs more robust) - so feel free to implement the change once the patch gets accepted :)
  4. rbtools/ (Diff revision 3)
    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.
  2. rbtools/ (Diff revision 3)
    Maybe better option is make use of --tracking-branch option (which is probably what this branch_diff means) instead passing all to --revision-range
  2. rbtools/ (Diff revision 3)
    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.
  1. 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?
  2. rbtools/ (Diff revision 3)
    No space between die and (
  3. rbtools/ (Diff revision 3)
    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.
  4. rbtools/ (Diff revision 3)
    These can be combined into one if statement.
  5. rbtools/ (Diff revision 3)
    No need for the parens in the return value. The tuple will be implied by the comma.
  6. rbtools/ (Diff revision 3)
    No blank line here.
  7. rbtools/ (Diff revision 3)
    You shouldn't need the +, as strings broken up into multiple lines like this will be automatically concatenated.
  8. rbtools/ (Diff revision 3)
    file is a reserved word, so you should probably use 'f' or something.
  9. rbtools/ (Diff revision 3)
    No space between function and (
  10. rbtools/ (Diff revision 3)
    No space between function and (
  11. rbtools/ (Diff revision 3)
    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?
  12. rbtools/ (Diff revision 3)
    Same thing about file here.
  13. rbtools/ (Diff revision 3)
    Same about the +
  14. rbtools/ (Diff revision 3)
    No space between function and (
  15. rbtools/ (Diff revision 3)
    No need for the \
  16. rbtools/ (Diff revision 3)
    Remove the blank lines between these.
Review request changed

Change Summary:

New diff, incorporating all requested changes


Revision 4 (+344 -1)

Show changes

  1. Looks good. Pushed to git. Thanks!