Bugzilla – Full Text Bug Listing
| Summary: | Three-way compare shows wrong changes | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||
| Component: | Compare | Assignee: | Michael Valenta <Michael.Valenta> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||
| Severity: | major | ||||||||||||||
| Priority: | P3 | CC: | Michael.Valenta, Mike_Wilson, sseidel, Szymon.Brandys, tomasz.zarna | ||||||||||||
| Version: | 3.4 | Flags: | Mike_Wilson:
pmc_approved+
tomasz.zarna: review+ |
||||||||||||
| Target Milestone: | 3.3.2 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Markus Keller
I have verified that this is caused by the new differencer we added in 3.3. The differencer is only responsible for calculating a two way diff. A three way comparison is performing by diffing the ancestor and right and ancestor and left and then combining the results. The new differencer yields slightly different results from the old differencer but the results are still valid. In this particuar case, a two way compare between anc.txt and one.txt with the new differencer yields different results than with the old differencer. It turns out that the new diff is better from a java semantic standpoint but the diff of the anc.txt and two.txt is the same (and sematically worse) for both the old and new differencer which works out well with the old differencer but causes problems with the new differencer. I'm going to investigate how we could modify the diffs that the new differencer produces so that the results are consistent. Created attachment 83590 [details]
Patch that fixes the problem
It turns out this was my fault. When I converted the contributed algorithm to use IRangeComparator, I left out the part that shifted the diff as far down in the file as possible (i.e. when the lines before and after the diff are the same). Adding this shift fixes the problem. I have comitted the fix to head. We should consider contributing the fix to 3.3.2
*** Bug 208864 has been marked as a duplicate of this bug. *** Tomek, can you look over the patch to make sure everything looks OK before we ask for PMC approval? *** Bug 196147 has been marked as a duplicate of this bug. *** I'm sure if there is much I can say about the patch itself without getting into details of the differencing algorithm. To provide some assistance I did the following: * run tests on Windows and Linux (just in case) * played around with the compare editor to make sure everything works fine * I looked for any other bugs related to the problem, this way I could find other cases to check (as Markus did for bug 196147) I found one case that made me a little bit worried. I had a file with outgoing changes, compared it with HEAD. With the compare editor opened I replaced the file with the latest version from HEAD, so both local and remote files became the same. But this was not what the compare editor showed me (ie it claimed that there some outgoing changes). After closing the editor and opening it again everything went back to normal. Please take a look at a following screen shot. I'm not if this is a related issue but thought it's worth mentioning. BTW,You made me realized that it's high time for me to grab Myers' article and get familiar with the algorithm. I hope I will become more helpful next time. Created attachment 83634 [details]
Confusing comparisons
Created attachment 83637 [details] An exception thrown while reproducing 34746 I was trying to reproduce bug 34746 when I was hit by the attached exception. Tomek, thanks for the additional testing. The problem was that the array can be null if one of the inputs is empty but that only occurs for the structure compare since a text compare assumes that each input has at least one line even if there are no characters. I've added the null check to head. I'm glad I could help. After further investigation it seems that the issue I mentioned in comment 6 is not related to the patch you provided. I will check if there is a bug for it and if not will log a new one. Again, as mentioned in comment 6 I treated the differencer as a block-box. I haven't been able to find any other issues so I'll give the patch a "+". Created attachment 83901 [details]
Patch for consideration for inclusion in 3.3.2
The latest patch addresses the issue in comment 8 and is optimized to compress and shift the LCS in place instead of creating another array. I think we should consider this for 3.3.2 since it is a regression and results in confusing false conflicts in the compare editor. Fix released to 3.3.2 Verified in I20080115-0800. |