Improvements to post-review error-handling with Perforce




 


  Review Request #1886
  —
  Created Nov. 1, 2010 and submitted

 















 
  
  
 




 
  

   Information

  

 
 

  

   
   
bhollis


  


  

   
   
RBTools


  


  

   
   



  


  

   
   
832, 1192


  


  

   
   



  


 



 
  

   Reviewers

  

 
 

  

   
   
rbtools


  


  

   
   



  


 











 

  
  


A handful of changes to improve error handling in post-review.

* Die with an error when the provided Perforce changelist does not exist
* Die with an error if diffs are empty
* Die with an error when there are no changes in a Perforce changelist. Fixes issue 832.
* Default to the default changelist when no Perforce changelist is provided. Fixes issue 1191.

This corresponds to the top four commits at http://github.com/bhollis/rbtools/commits/errorhandling/. Note that 2bd8897b133aa81e2b32 changes the default of running "post-review" in a Perforce client to uploading the default change - the other changes should not introduce any new behavior.




 




 

  
  


These changes have been in use for a while.




 














 

 














  

 








 














 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I haven't yet looked at the code, but I think your first bullet point is issue 1808.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I just noticed that I'm missing one patch that fixes the "default to default changelist" change - what's there now breaks path-based reviews. I'll add a new diff when I get home that fixes that.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 AJ





   

   







  

 



 

  


   

   

    
 
ajsrb


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good. One small nit, below.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      rbtools/postreview.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    While the two SHOULD be the same, "\d+ - no such changelist" or just an re.search on "no such changelist" would seem to be more robust and involve less string work.

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 BH





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Simplify nonexistant changelist detection in response to code review. Also include a missing change that makes defaulting to the "default" Perforce change more robust, and prints the review URL with a trailing slash (to avoid a useless redirect).

I couldn't find on the contributor guide what to do when I've got a "Ship It!" - should I submit a separate pull request on GitHub with a link to this review, or will you pull? Should I squash my commits?





 



  

   
Diff:

   
    
 Revision 2 (+18 -4)
Show changes
 
    rbtools/postreview.py 
 

   		
  



 










  

 
















 





 

  

   





 JA





   

   





  

 



 

  


   

   

    
 
jameslin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    As the reporter of issue 1191, I'd strongly prefer if post-review with no arguments printed a usage summary instead of creating a review request from the default changeset.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      But, for all of the other SCMs, like SVN, post-review with no arguments posts a review.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Good point.  I'm not a fan of that behavior (although it makes sense for SVN where there traditionally isn't a changeset to specify), but okay.

      
 
      

      3. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks for this! I've pushed a squashed version as 65eec92.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...