Review Board does not handle files with ^M characters at end of line.

 






 

 


  Review Request #826
  —
  Created April 20, 2009 and submitted

 


















 

  

   Information

  





 
 

  raghu
 









 
 

  Review Board SVN (deprecated)
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This is a patch for http://code.google.com/p/reviewboard/issues/detail?id=1071 that I had reported. The idea is to make post-review and diffviewer
intelligent about lines ending with \r\r\n pattern. All we have to do is to convert those to a simple newline.

 








  


 
 

  
Tested this at my work, looks fine so far.

 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Before I get into actually reviewing the change, I'm trying to understand the need for the change.

First, I don't think post-review should deal with anything involving newlines. It just shouldn't care. All logic should be in Review Board for this. We don't guarantee that people even will use post-review for uploading, as they may use the web UI or some third party app.

Second, where does \r\r\n even come from? How is that a valid newline? Out of all the people that have used Review Board, I have never seen anyone hit an issue with a diff with \r\r\n. If this is some weirdness with a file originally using \r newlines later having \r\n newlines added on top of that, then that's something that needs to be fixed in the file and not Review Board, imho.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      There are two parts to this. The file obviously is bad. But that should not cause Review Board to crash, IMO.

The trailing ^M is a common problem when people ftp a file from windows to Unix or sometimes even when there is a network file share. Almost all editors will handle them gracefully and vi will even show you that there is a ^M at the end. In my case I found few hundred files like this in the perforce repo and this has been there for over 5 years and nobody even noticed. That shows how other tools & editors handle it without a fuss and I strongly feel Review Board too should handle it gracefully.

Now about the change in post-review. I agree post-review should not worry about the newline translation. But with existing code, it does some not-so-obvious translation. First, the _write_file function used to invoke subprocess.popen with universal_newlines=True which will convert a \r\r\n sequence to \r\n\r\n. So I get a file with blank line between every lines. My change is to fix this so that I get the file as it exists in the perforce repository. I would believe this problem does does not exist with other SCMs. I'll try to create unified diff manually and see whether that works.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Okay, it seemed bizarre to me at first, but it makes more sense now. You're right, we should handle it without breaking, and the change seems reasonable enough.
      
 
      

      3. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/rbtools/scripts/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Should be in the list alphabetically.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Fixed. Sorry about these glitches, this is my first python program. :)
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/rbtools/scripts/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Should wrap to 80 chars. Probably want to put translate_newlines on the next line, aligned with diff_cmd.
    
  
    
 
    

 
    
  



 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/rbtools/scripts/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Blank lines before this, and end with a period.
    
  
    
 
    

 
    
  



 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/rbtools/scripts/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Trailing wihtespace.
    
  
    
 
    

 
    
  



 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/rbtools/scripts/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Just to be sure, you're certain that we no longer hit this permission error?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Right, the problem was that perforce saved the file as read only and when we did an unlink later it gave a permission denied error on Windows. I do not get that after this change.
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    





    
    Seems redundant. We're now doing two convert_line_endings on the same data, which could slow things down on large files.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I am confused, this is the original file from SCM to show on the left side of the review. The other convert_line_endings call is on the file after patching - the right side of the diff. Shouldn't we convert both?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      You're right, I misread.
      
 
      

      3. 


 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   





 RA





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
I have fixed all of the review comments except one. Sorry about being so late on this, I wanted to get this in early but was on a long vacation and didn't get time to complete this.





 



  

   
Diff:

   
    
 Revision 3 (+41 -35)
Show changes
 
    /trunk/rbtools/scripts/post-review 
    /trunk/reviewboard/diffviewer/diffutils.py 
 

   		
  



 










  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks! Committed as r1988.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 MB





   

   





  

 



 

  


   

   

    
 
mbee


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/diffviewer/diffutils.py
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    You should replace the second data.replace by a temp.replace

This is a classical example of quick copy/paste ;)
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...