Change Summary:
Updated diff, that uses newer 'cm log' commands for changeset diffs. This should be a lot faster.
Diff: |
Revision 2 (+350 -1) |
---|
Review Request #1396 — Created Feb. 8, 2010 and submitted
Information | |
---|---|
dick | |
RBTools | |
Reviewers | |
reviewboard | |
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.
Updated diff, that uses newer 'cm log' commands for changeset diffs. This should be a lot faster.
Diff: |
Revision 2 (+350 -1) |
---|
The same diff, but uploaded again to try to make it viewable.
Diff: |
Revision 3 (+350 -1) |
---|
rbtools/postreview.py (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.
I'm so glad to see PlasticSCM support in ReviewBoard :) This is fantastic news!
rbtools/postreview.py (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: 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.
rbtools/postreview.py (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" --solvepath=itemid
rbtools/postreview.py (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.
rbtools/postreview.py (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
rbtools/postreview.py (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.
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?
rbtools/postreview.py (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.
rbtools/postreview.py (Diff revision 3) |
---|
No need for the parens in the return value. The tuple will be implied by the comma.
rbtools/postreview.py (Diff revision 3) |
---|
You shouldn't need the +, as strings broken up into multiple lines like this will be automatically concatenated.
rbtools/postreview.py (Diff revision 3) |
---|
file is a reserved word, so you should probably use 'f' or something.
rbtools/postreview.py (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?