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:
Col: 12 E127 continuation line over-indented for visual indent
Review Board does not respect line endings
Review Request #3970 — Created March 15, 2013 and discarded
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
What version of ReviewBoard does this apply again? I tried patching 1.7.9 and several hunks failed.
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.