2916: Review Board does not respect line endings
- New
- Review Board
| kano*****@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| 2278, 3198, 3672 |
What version are you running? 1.6.15 What's the URL of the page containing the problem? Any diff viewer page. What steps will reproduce the problem? Patches downloaded from the Review Board seem to always have Unix line endings (LF), no matter what line endings the original patch has. I.e. if you submit a patch that contains DOS line endings (CRLF) to Review board and then download it from the review request, the DOS line endings get lost. You cannot apply such a patch. What is the expected output? What do you see instead? What we need is for Review Board to always preserve original line endings, i.e. an uploaded patch should not change when you download it back. For example, if the source contains mixture of DOS and Unix line endings, it should be preserved as is, otherwise applying the patch will fail. What operating system are you using? What browser? Windows/Linux, Chrome,Firefox, Opera. Please provide any additional information below.
We are seeing exactly the same problem, in the MySQL group of Oracle.
We test the latest version of RB (1.7.6). It exposes the same problem.
Chris/David, Would it work if we make DiffParser class init do a data.splitlines(True) instead of data.splitlines(), which will keep the line-endings intact(instead of stripping it as it does now and the code manually tacking a \n at the end). We would then have to manipulate the patch method in diffutils.py to strip the line-endings in a way that patch can understand. That way, the data stored in filediffdata remains the same as what the user uploaded, and can be downloaded as such. Maybe Im over-simplifying this issue but thought Id check with you guys before I start on this.
Perhaps... Line endings in general are a real problem in diffs, because not only do you have \n and \r\n to deal with, but some things will stupidly give you \r\r\n and other mixtures. If that change passes all unit tests, then I'm okay shipping it and seeing if there's fallout.
Posted a patch for this at http://reviews.reviewboard.org/r/3970/.
How we can apply this patch?
Tested the patch, works well. Thank a lot! Looking forward to seeing it in a release.
Some problem detected with this patch. Unable to add patch with windows line endings (CRLF). Trying to add such patch gives the following error: "Unable to parse diff revision header '(revision NNN) '" NNN - revision number.
Hi, Sorry about that. I will try this on a windows box. I created a patch on linux and ran a "todos" to convert it to a windows styled patch (with CRLF line ending), uploading this worked fine.
Are you running 1.6.15? I can see that the string is stripped (which should remove the newlines) before the regex is applied (failing the regex throws the error you mentioned), but the strip is not in the 1.6.15 branch, only on master (1.7 or above). You might want to try it on a 1.7 reviewboard to see if its still a problem.
We running 1.6.16 Everything works fine with patches that contains unix line endings or unix line endings+windows line endings. But gives the error if the patch contains only windows line endings.
Just verified that its not in 1.6.16 either, You would need to be on the 1.7 tree to have that change. The stripping of the newlines is what makes the difference. With my patch on 1.7 tree, it would work because the strip takes care of removing the newlines before the regex parsing happens, but with 1.6.x, since the regex looks like for a line ending with "revision (XXXX)" it fails because windows styled line endings have a \r\n tacked after the "revision (XXXX)".
I see. That is your patch is only suitable for the 1.7 tree?
Yes. Patches are typically done against the current trunk, we can check with reviewboard developers if this can be backported to 1.6.x branch
It would be nice. Thanks for your answers.
What version of the RB is planned to include this patch? Version 1.7.7.1 does not contain its.
1.7.9 also has this issue.
1.7.14 has this issue as well...
Our current release process is to merge the approved patch into /trunk, and this issue creates extra steps when merging the downloaded patch. I use 'svn patch' to apply the patch file, which works without errors, but all of the newly inserted lines, as well as the surrounding 'context' lines are converted to unix-style line-endings. I then need to use either unix2dos or Eclipse "File -> Convert Line Delimiters To -> Windows" to fix the line-endings before committing the changes. RB should preserve the uploaded patch without modifications. If it needs a normalized version of the patch for internal processing, it should probably create a working copy. If storage space is a concern, you might also consider deleting the working copies when the review is closed, re-creating them as needed from the original assets if the review is re-opened. You might also be able to short-cut the creation of the working copy of the original patch file is already in the correct format. Thank you for your time, -David Farrell
I'm being bitten by this issue as well, and it's a real pain when trying to wrap automation around ReviewBoard (i.e. a Jenkins job that has a git commit hash, and tries to make sure the current diff against master matches the one that was shipped).
Looks like Ricket 3198 is another duplicate?
This issue makes me have to let Jenkins search and replace the CRLFs the whole repo every run just to workaround
As far as I can see this is still an issue despite the previous attempt to fix it. What is the status on this? What does it take for a fix? As this is a problem for us - I work in a Windows oriented company :( - I would be wiling to put some effort into a patch.
My suggestion: https://reviews.reviewboard.org/r/10090/