post-review: Option to update the changelist description with the review url.

Review Request #527 — Created Sept. 4, 2008 and discarded


Review Board SVN (deprecated)


Adds '-u' option to post-review. This will get the changelist description and update it with the review url created.
Only perforce is supported.
Posted a review and verified that the changelist was updated with url
  1. A really nice feature but I would not want my users to have to install P4Python by themselfes.
  2. trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
    I don't know whether you want to add a dependency to P4Python although this could also be done using "p4 change -o" and "p4 change -i".
    1. Hmm, that *is* a valid point. Let me look into 'p4 change' commands.
  2. trunk/reviewboard/contrib/tools/post-review (Diff revision 4)
    Since you're only using this regex once, you can use re.match as a shorthand.
    1. I'm using it twice. 2nd time is when we update the desc with the url. And the regex was kinda long. Code might be easier to read with a compile. 
  3. trunk/reviewboard/contrib/tools/post-review (Diff revision 4)
    Two comments here:
    1. It would be really nice if this could update an existing 'Review Url:' field. At VMware we have changeset templates with a bunch of fields in them; being able to add an empty field to the template and let post-review populate it would be awesome.
    2. A more pythonic way of writing this is:
        if 'Review Url:' not in desc:
    1. Made the changes. So if a template exists, script will update it.
  4. This won't work now because execute sets shell=False. It might be helpful to add some plumbing to plumb some stuff through to execute() such that you can dump the file into the pipe for stdin. This would also eliminate the need to write the file out to disk.
    1. I'm not sure I understand this. Would you mind explaining this?
    2. You're using shell syntax to redirect input from temp_name. The execute() function no longer passes shell=True to the underlying subprocess.Popen call, so "<" and temp_name will be passed as *arguments* to p4, which is not what you want. You need to add an (optional) argument to execute which is the contents of stdin, and then you can pass the data that you would have written out to temp_name into that function.
    3. Fixed. Much better
    4. I cannot view the uploaded diff. Reviewboard is giving some error. I tried creating a new review but that failed as well. 
Review request changed