Community
Participate
Working Groups
Greetings, It seems that when you use the Team -> Apply Patch feature in Eclipse, there is a bug that modifies the line-endings in the file. Example: afile.txt is an ascii file with \n line endings You apply a patch that is also an ascii file with \n line endings The result is that afile.txt now has \r\n line endings on every line in the file, even the lines not modified by the patch (there should only be \n endings). This is unexpected behavior and can lead to much grief later. One problem this causes is that when you later do a compare against CVS, the whole file appears to be one big change, which is not good. The file must be converted back to its original line endings (using something like dos2unix) before comparisons are useful again. I have looked in my settings and the only thing that seemed relevant was the "Convert text files to use platform line ending" option under Team -> CVS -> Files and Folders, and it is unchecked. Am I doing something wrong?
The problem is that the Apply Patch wizard probably uses a Reader and Writer for accessing and writting file contents. Readers will break on any well known line break character but Writers (at least by default) use the platform line ending. Correcting the problem would require that the Apply Patch wizard look at the line endings in the file and preserve them when writing the patched version back out.
There is also a File>Convert Line Delimeters submenu in Eclipse which allows use to convert line delimeters as well.
Just wanted to report that this is still an issue with Eclipse 3.3, and occurs on Linux as well as Windows. In my case, I: 1. Ran Eclipse 3.3 on Linux (with my workspace set to use default line endings) 2. Created a file and used File | Convert Line Delimiters To Windows 3. Created a patch for the file (also using Windows delimiters) 4. Applied the patch using Team | Apply Patch 5. Noted in the preview that all the line endings in the "After Patch" pane have been changed to UNIX line endings (see attached screenshot) I did try changing the workspace line ending setting to Windows in case this setting was being used by the Apply Patch wizard. This is not the case; the platform line endings are always used, which lends weight to Michael's theory in comment #2. If someone can point me in the right direction (I've not done any community development before) then I'd be happy to have a look into this since I feel it's an important bug. As it stands, the apply patch functionality is useless when working with files that have line endings that differ from that of the current platform---a fairly common scenario these days.
Created attachment 78544 [details] Screenshot of the Apply Patch preview, about to change the line endings from Windows to UNIX
Chris, thanks for you offer to help. The code in question is found in the org.eclipse.compare plug-in in the repository :pserver:anonymous@dev.eclipse.org:/cvsroot/eclipse. The class/method that handles patch application is org.eclipse.compare.internal.patch.Patcher#applyAll.
Created attachment 89918 [details] Proposed patch against CVS head I finally got around to looking at this like I said I would, although it took me long enough! Anyway, Patcher.isPreserveLineDelimeters() is used in 4 places (Patcher.applyAll and WorkspacePatcher.applyAll) where the code stores the results of applying a diff into a file. It's also used in PatchFileTypedElement.getContents() when loading the left/right/ancestor files from disk. My patch very simply changes Patcher.isPreserveLineDelimeters() to return true instead of false; this resolves the bug for me. Browsing the code, I don't think that this is likely to cause any ill-effects, but I'm keen to get feedback from folk.
Thanks for the patch Chris, it coulnd't be more straightforward. I've got one concern though. I'm afraid that the code looks and works like this for a reason, is that right Michael? I must admit it would work better with your patch, but sometimes things aren't that easy. I think we should keep backward compatibility even for stuff which doesn't work perfectly. In other words, some people might got used to this "unexpected behaviour". This is how it should be done, but in my opinion we can apply the patch and probably get away with it. An answer to this issue could be a preference called (surprisingly) "Preserve line delimiters". It could be used for applying and copying (left to right and vice versa) in a compare editor. But still, we will need to decide whether the option should be on by default. How does it sound? Michael, what do you think?
Any further thoughts on this? From my perspective, I can't imagine why anyone would expect/desire the apply patch wizard to convert all the line endings to the current platform's line ending style. So in that respect the simplest solution is just to go with my patch. ;-) However, if the consensus is that people are relying on the existing behaviour then I'd be happy to try and implement a compatibility option. That said, I've never done any serious SWT/JFace work so I'll have to see how far I get!
Sorry, I didn't realize this was gated on my response. When applying a patch, I think that the line endings in the target file should be preserved. If that's all that the change in the patch does, then I think it is the right thing to do (i.e. we need to make sure there are no other side effects).
Assigning to myself so I won't forget to double check the patch and release it.
The patch looks fine, however it doesn't cover all the cases. In comment 0, it's mentioned that if there is a file with \n line endings and you apply a patch with \n line endings you will get a patched file with \r\n. Turning the isPreserveLineDelimeters flag to true does fix this issue, the problem is that it does it only when saving a patched file to disk (or displaying the result in UI[1]). IMO having it fixed on a lower level would be much better (and easier to test). For example changing the value returned by isPreserveLineDelimiters method doesn't affect the result kept in FileDiffResult object, which still contains patched lines with invalid delimiters. The other thing which concerns me, is the way isPreserveLineDelimiters changes the patched file. It does either replace all line endings to a standard one for the given platform or leave them as they are. It works fine if a file to patch and a patch file have the same delimiters. If they are different (eg lf for file and crlf for patch) you will end up with a patched file with mixed endings. A desired result would kept the line delimiter from the original file and set it for the whole file (eg lf for the previous example). I will send my proposition of the patch + test cases in a minute. [1] org.eclipse.compare.internal.patch.PatchFileTypedElement.getContents()
Created attachment 109644 [details] Test cases These are different configuration of line endings for a file to patch, patch file and expected result. The assumption is that the line endings should be the same in the original file as in the one with expected result. Neither the patching mechanism nor the line endings in the patch should change it. PS. I'm afraid that the patch won't work when applied since it has all line endings replaced with CRLF by CVS :/ I hope that commiting it from my workspace will preserve the line delimiters.
Created attachment 109645 [details] Patch v01
Created attachment 109646 [details] mylyn/context/zip
(In reply to comment #12) > PS. I'm afraid that the patch won't work when applied since it has all line > endings replaced with CRLF by CVS :/ I hope that committing it from my workspace > will preserve the line delimiters. This is tracked by by bug 107650.
*** Bug 230888 has been marked as a duplicate of this bug. ***
Tests and patch released to HEAD.
I tried to verify the fix on I20080918-0100. However I found a scenario in which the fix doesn't work. OS: Windows XP Steps: 1. Add two lines at the end of some file (lets say a.txt). 2. Create a patch. 3. Replace the a.txt with the latest from HEAD. 4. Change line delimeters in a.txt and the patch to be Unix. 5. Apply patch. Expected result: 6a. File a.txt has Unix line delimeters. Actual result: 6b. File a.txt has Windows line delimeters.
See Pawel's comment above.
Pawel has offered his help here. Assigning to him.
Tomasz, you have to apply the patch provided by Chris in comment 4 for the fix to be complete. The fix you released prevents from mixing line delimiters in case path and file being patched have different ones. But without Chris' patch this work gets ignored. Tests have to be updated in order to catch this behaviour. Right now tests compare patched file in memory, without writing it onto disk. So, they cannot catch errors occurred while writing the result onto disk.
>> Tomasz, you have to apply the patch provided by Chris in comment 4 for the >> fix to be complete. Actually it was in comment 6:)
I guess you're right, only when both patches are applied the patcher works as expected. But still I think it would be great if you could have a look at our tests and update them. I will release Chris' patch as soon as we have tests ready. Let's try to take advantage of test-first programming here.
Created attachment 113741 [details] Chris' patch updated
We run out of time in 3.5 M3, changing target milestone to M4.
Created attachment 120188 [details] Patch for test cases This is a fix to test cases suite that converts output of the patcher to the form that is used when writing onto disk. This is equal to comparing patched file to the expected file byte after byte (as they are stored on disk). Tom, you have to change the property of all the files in patchdata in org.eclipse.compare.tests to binary. Right now I can't even determine line delimiters used in these files because they are changed on the fly wile serving them back to client.
(In reply to comment #26) > Tom, you have to change the property of all the files in patchdata in > org.eclipse.compare.tests to binary. Done. I've changed the properties to Binary, released the updated test from you and Chris' patch at the same time. Thanks Pawel!
