post-review: support perforce pending changelists

Review Request #804 — Created April 3, 2009 and discarded

Information

Review Board SVN (deprecated)

Reviewers

This adds support for pending changelists, including the default changelist, to post-review.
This patch is being used internally in my $DAYJOB group; works great from all indications
JO
  1. 
      
  2. 
      
chipx86
  1. 
      
  2. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
     
    Indentation looks wrong.
  3. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
     
    Blank line after blocks.
  4. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
    Is there a chance we'll get a \r\n? Wondering if splitlines() is a better idea.
    1. Not unless 'd += "...\n"' is magically adding a "\r" as well?
      
      Note that this is splitting the string built by the previous line. (I'd omit this entirely, except I'm a Python newbie and didn't know how to build it as an array directly :-).)
  5. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
    This blank line should stay.
  6. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
     
    Can we just roll this up into the previous cl_is_pending check?
    1. Not if you want to omit it entirely ;-).
  7. /trunk/rbtools/scripts/post-review (Diff revision 1)
     
     
     
    Not wild about this. It's one thing to call a new method, and another to change expectations for return values.
    
    I don't feel diff is the right place to really transform changenum, so maybe another function is in order. Perhaps a function that returns whether or not a changenum is valid for upload to a server. Then new_review_request could call this before determining whether to upload it, like:
    
       if changenum and tool.is_uploadable_changenum(changenum):
    
    Not sure about that name, but something along those lines.
    1. Makes sense (even if it's duplicated code). At the time, getting it to work was higher on my priority list than doing it elegantly.
  8. 
      
MW
EH
  1. I'm not so hot about this change, though I'm having trouble describing exactly why.
    
    Some of the terminology and the statements in issue #1020 are what confuse me.  post-review handles pending changelists just fine.  It just doesn't handle the *default* changelist.  When I first started using reviewboard I thought it was a little odd that I had to run "p4 change" in order to post a review, but I got used to it and it ends up being convenient to keep the changelist description in sync with the reviewboard description.
    
    The only thing I can see about this change is that it clobbers the change number for pending changelists (for non-default changes).  I think that would cause problems when you try to repost a review without specifying -r.
    
    1. That's... interesting. It most assuredly doesn't work over here; 'p4 describe' doesn't give a list of open files for pending changelists (besides not working with 'default'). And the server (we are running 1.0beta2) rejects review requests where the changenum is set and is a pending changelist (possibly for the same reason?).
      
      That said, our p4d is ancient; maybe the behavior of 'describe' changed.
      
      The change to generating the file list is (or should be) harmless (and adds support for non-numbered pending changelists, i.e. "default"). If I change stripping the changenum to do so only with p4d < 2003.x, would that be satisfactory?
      
      As for stripping the change number causing problems, it most certainly does not, except so far as the request description isn't pulled from the pending changelist. Which as mentioned, doesn't work for us anyway, and also doesn't apply to "default" changelists. It is not necessary to specify -r; RB handles perforce requests with no change number just fine (I can say that with confidence since none of the many-dozen requests on our server against the perforce repo are problematic in any way, issue #1021 notwithstanding).
  2. 
      
MW
MW
Review request changed
Change Summary:
According to the p4d release notes, the behavior of 'p4 describe' changed in 2002.2 (perforce issue #35391); update the strip-change-number test to reflect this.