Fix CRLF issues

Review Request #83 — Created June 19, 2007 and submitted

Information

Review Board SVN (deprecated)
trunk
78

Reviewers

This is quite an hack, but at least it will start the ball rolling on this issue.

There are several problems, all related to CRLF. The first problem I had, and that the simpler patch I posted on issue 78 solved, was creating a diff on Linux to a file with CRLF endings.

The next problem, that this patch solves, is posting a patch created on windows (with svn diff or tortoisesvn). There were two problems:
* the parsing of the revision number, caused by the first part of the patch.
* after correcting revision parsing, another problem is caused by patch itself. If patch (in Unix, at least) gets a patch file with CRLF line endings, it assumes it was meant to have LF line endings and throws the CR away. This makes the patch fail because the file being patched has CRLF. 

This second problem is solved by converting the header lines to end with LF.
By the way, I think this is the cause for the issue mentioned by someone on the mailing list using tortoise svn to generate the diff.

The code is a hack, I just started looking at python code this week, and I don't really know what happens with review-board installed on Windows, but it solves my problem.
It works for patches to files containing CRLF, generated both on Windows and Linux, at least when the server is running on Linux
chipx86
  1. 
      
  2. /trunk/reviewboard/diffviewer/parser.py (Diff revision 1)
     
     
     
     
     
     
    I'm a bit confused as to this segment. Why are we special-casing this? If it's necessary, a comment should be added describing why.
    1. Well, it's related to what I wrote in the description about the patch command on unix dropping all CRLF from the diff if all lines have CRLF line endings. This way, removing the CRLF from the header and keeping them in the file contents prevented patch from doing that, due to the inconsistency of line endings, allowing to patch files with CRLF line endings even on Unix.
    2. So let me make sure I've got this.. If you're patching a file that's supposed to have CRLF line endings, patch will strip them, which screws things up for the file, right? However, if the file is always using just \n, then we're good, regardless of the line terminators in the patch, right?
      
      If so, here's what I propose:
      
      1) Keep the line endings in the diff as-is.
      2) When we pull files from the repository, quickly check if the first line ends with CRLF. If so, split the lines and rejoin with \n.
      3) Patch that.
      
      Since the results of this are only used for showing the side-by-side diffs, it doesn't matter what line terminators they use. All that matters is preserving them in the original diff so that they won't be mangled when people download the raw diff. Patch will still say that it's stripping CRLFs, but we won't care, because we know the line terminator in the file it's patching is compatible.
    3. Okay, ended up putting up a simpler, safer patch with unit tests that should fix this problem (/r/83). Going to close this one out.
  3. I reverted the previous CRLF change due to several bugs that it caused. To prevent that here, I'd like to request some thorough unit tests covering diffs with different line terminators and testing any revision info that would be affected. We should test that each can still be parsed and that each can be applied.