• 
  
    
 
    
 
    

    



    
    
    
    
     
    




    
 
    

  
    



    
 

 

  

  

 

    


  
    





    
 
    



  

    
 
 
    
  
    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.