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.

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.

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

Done.

Done.

Fixed.

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.

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?

You're right, I misread.