Summary

Fix CRLF issues

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

Information

Owner

nuno

Repository

Review Board SVN (deprecated)

Branch

trunk

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

It works for patches to files containing CRLF, generated both on Windows and Linux, at least when the server is running on Linux

/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.

nuno June 24, 2007, 12:57 a.m.

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.

chipx86 June 24, 2007, 7:24 a.m.

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.

chipx86 July 2, 2007, 6:09 p.m.

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.

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.

nuno June 24, 2007, 12:57 a.m.

I now think there is no way for this to support every client/server/file line endings combination, exactly because patch tries to "fix" the diff files, so maybe the sane way is to support files with the native file endings.

To support this, the previous CRLF with the fix I posted on issue 78 is working for us, what problems are you having?

chipx86 June 24, 2007, 7:09 a.m.

The breakage we encountered was first the extra newline in the revision info (solved by your rstrip()). We fixed that by rstripping, but then we had issues where patch complained about the line endings not being consistent or something. I wish I had written it down, but I was in a mad rush to get Review Board working again for everybody at work who was using it.

Before any fix goes in for this, I'd love to see unit tests written in order to make sure we've completely got it. We should test with each combination of line terminator in both patch and source files.

I built new fixtures for unit tests last night and will get info up on the wiki today about them, so watch for that. It'll be under the UnitTestFixtures page.