post-review: Support older p4d, pending changelists

 






 

 


  Review Request #1031
  —
  Created Sept. 9, 2009 and submitted

 


















 

  

   Information

  





 
 

  mw_triad
 









 
 

  RBTools
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  rbtools
 









 
 

  
 






 








 





  


 
 

  
This fixes post-review to work on older p4d (which I am stuck with at $DAYJOB); without this change post-review can only be used on submitted changelists. This also adds support for the default changelist (for all p4/p4d versions).

See also http://reviews.review-board.org/r/804/ (replacing the diffs there failed, hence the new request).

 








  


 
 

  

 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      rbtools/postreview.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank line between these.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      rbtools/postreview.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Please add a doc string explaining what this does.

Also, this should probably be get_submitted_changenum. Is that really what this is, though? It doesn't actually return a submitted version of a changenum. It's more that it determines whether it's pending or not and returns None if it's not...

This seems like it would break existing Perforce setups where you have a pending changeset that isn't yet submitted. It would take the perfectly valid changenum and return None, which is not what we want.

The *pending* check here and elsewhere in the code seems to really change the existing behavior, and we need to make sure it doesn't.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      ...except that (IIRC) RB flat out breaks if you try to feed it a pending cl number. Maybe that's because the server has the same problem with older p4d as post-review? But how to deal with it, then? I suppose this function could do the same version check, and only return None for default if p4d is newer. That would be suboptimal in some weird case where the RB server is different from the one seen by post-review, but I don't know if that's even possible, and anyway the alternative is to change Review Board itself.

As for the name... hmm... what about "sanitize_changenum"? As you say it isn't "get the submitted changenum", the actual purpose of the function is to return a changenum that is safe to be fed to Review Board.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I want to make sure we have the same definition of pending here.

You p4 change, describe the changeset, add the files, but don't commit. You then post this change number for review. Right?

This works for us and has for a long time. These are registered on the server, so we can pull the change description down. Does that not work on the 2002 version?

As for the function, sanitize_changenum would be fine.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      "pending": yes, we're on the same page. That *doesn't* work (and AFAIK, never has) for us. My guess is because p4d < 2002.2 doesn't give the same information via 'p4 describe <pending-cl>' that 2002.2 and later do, i.e. the same reason I had to change post-review to go through extra hoops to get the list of files.

It's possible to work around that (and I do just that, in fact, in this patch), so I suppose Review Board could be similarly fixed. But I'm inclined to just make the same version check in sanitize_changenum. Would that be acceptable?
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Yep, that sounds fine. I just wanted to make sure :)
      
 
      

      5. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      rbtools/postreview.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    It would be nice to make a function for returning the server version and then calling it here. It should return the version in a tuple, or some such, for easy comparison.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Good idea, will do that in the next version of the patch.
      
 
      

      2. 


 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   





 MW





   

   

  

 



 















 



 

  

   





 MW





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Store p4d_version when we do get_repository_info (saves some operations), and don't do extra work when we have the file list from 'p4 describe'.





 



  

   
Diff:

   
    
 Revision 3 (+71 -12)
Show changes
 
    rbtools/postreview.py 
 

   		
  



 










  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Committed with minor changes. Thanks!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...