Review Board does not respect line endings

Review Request #3970 — Created March 15, 2013 and discarded

Information

Review Board

Reviewers

Make Reviewboard respect user-uploaded line-ending
    
Reviewboard strips the line-endings from user uploaded patches and manually adds \n at places to make the "patch" command work properly. This causes issues when users try to download the patch and its not using the same line-ending as what they uploaded.
    
Fixed by retaining the user uploaded line ending when the patch is uploaded. This allows the user to download their diffs as-is but stripped off \r\n characters in other places where we compare the diff headers and other diff line changes.
    
Summary of changes:
* reviewboard/diffviewer/parser.py
  + Used splitlines(True) instead of splitlines which retains the line endings, whether they are windows or *nix styled
  + Removed the manual addition to \n in many places because it was stripped originally. Now that we retain it, we dont need to add any line endings
  + In some places where we do string comparisons (==) or check for endswith, did an rstrip('\r\n') so that the line-endings are not involved in comparison operations
    
* reviewboard/scmtools/git.py
  + Removed the manual addition to \n in many places because it was stripped originally. Now that we retain it, we dont need to add any line endings
    
* reviewboard/scmtools/svn.py
* reviewboard/scmtools/mtn.py
  + In some places where we do string comparisons (==), did an rstrip('\r\n') so that the line-endings are not involved in comparison operations
    
* reviewboard/scmtools/tests.py
  + Fixed CVS Tests where the lineending was failing with asserts. RStripped them to make it work.
Ran unit tests. Installed Git,SVN, Hg, CVS binaries to test all their cases. Did not test bzr or perforce as I dont have binaries for those. 

Manual: Created a patch in Linux, uploaded them , created another revision of it. Made sure that the patch uploads fine, can be viewed properly as well as Downloaded properly. Inter diffs worked fine as well. Verified with `file` command to ensure that they are "ASCII text", which is how it appears for unix line-endings.
raja@amethyst:/tmp$ file /tmp/t1.patch 
/tmp/t1.patch: unified diff output, ASCII text

Did a `todos` on the unix patch(to make it a dos-styled patch), uploaded it as a review request, created another revision of it. Verified that patch uploads fine, viewed properly and downloaded with dos line endings. Inter diffs worked as well. Verified with `file` command to ensure that they are "ASCII Text with CRLF line terminators".
raja@amethyst:/tmp$ file !$
file /tmp/t22.patch
/tmp/t22.patch: unified diff output, ASCII text, with CRLF line terminators
Description From Last Updated

Col: 12 E127 continuation line over-indented for visual indent

reviewbotreviewbot

this is another sample to attach comment

SW swimpool
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/diffviewer/parser.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/diffviewer/parser.py (Diff revision 1)
     
     
    Col: 12
     E127 continuation line over-indented for visual indent
    
    1. This is in-line with the existing code and the code around it.
  3. 
      
SW
  1. 
      
  2. reviewboard/diffviewer/parser.py (Diff revision 1)
     
     
    this is another sample to attach comment
    1. Hi, Please don't use real review requests for testing purposes. It confuses everyone and makes it hard to review.
  3. 
      
BC
  1. What version of ReviewBoard does this apply again? I tried patching 1.7.9 and several hunks failed.

    1. I should add that I was trying to apply it in order to test it and hopefully get it committed soon.

    2. Going by the release notes, this patch was done on 1.7.6 (1.7.7 released on April 2013, and this patch was posted before that and after the 1.7.6 interval).

    3. hmm, it doesn't apply against 1.7.6 either. Could you update the patch against either 2.0 RC2 or 1.7.24 please?

    4. I will update the patch against 1.7.24 in the next couple of days.

  2. 
      
BC
  1. I updated the patch against reviewboard master (all except the svn part, since it's changed substantially) and applied it to a 2.0 RC2 installation. The diff viewer fails with a decode error on vcxproj files, apparently because they're utf-8 and contain non-ascii bytes.

  2. 
      
david
Review request changed

Status: Discarded

Loading...