From 3fb83747e17ea1e90a5946fe9dbf9ebcb0c054bb Mon Sep 17 00:00:00 2001 From: Aleksey Pivovarov Date: Mon, 17 Oct 2016 16:51:36 +0300 Subject: [PATCH] merge: use non-greedy approach for "magic" conflict resolve * it produces lots of confusing false-positive results * move "greedy" approach under registry key --- .../diff/comparison/ComparisonMergeUtil.java | 8 ++- .../diff/comparison/MergeResolveUtil.kt | 62 ++++++++++++++++++- .../src/com/intellij/diff/util/DiffUtil.java | 32 ++++++++++ .../diff/comparison/MergeResolveUtilTest.kt | 41 ++++++++---- .../util/resources/misc/registry.properties | 2 + 5 files changed, 129 insertions(+), 16 deletions(-) diff --git a/platform/diff-impl/src/com/intellij/diff/comparison/ComparisonMergeUtil.java b/platform/diff-impl/src/com/intellij/diff/comparison/ComparisonMergeUtil.java index 1b1b926d6f9d..7c8340f04f45 100644 --- a/platform/diff-impl/src/com/intellij/diff/comparison/ComparisonMergeUtil.java +++ b/platform/diff-impl/src/com/intellij/diff/comparison/ComparisonMergeUtil.java @@ -20,6 +20,7 @@ import com.intellij.diff.util.MergeRange; import com.intellij.diff.util.Range; import com.intellij.diff.util.Side; import com.intellij.openapi.progress.ProgressIndicator; +import com.intellij.openapi.util.registry.Registry; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -160,6 +161,11 @@ public class ComparisonMergeUtil { public static CharSequence tryResolveConflict(@NotNull CharSequence leftText, @NotNull CharSequence baseText, @NotNull CharSequence rightText) { - return MergeResolveUtil.tryResolveConflict(leftText, baseText, rightText); + if (Registry.is("diff.merge.resolve.conflict.action.use.greedy.approach")) { + return MergeResolveUtil.tryGreedyResolve(leftText, baseText, rightText); + } + else { + return MergeResolveUtil.tryResolve(leftText, baseText, rightText); + } } } \ No newline at end of file diff --git a/platform/diff-impl/src/com/intellij/diff/comparison/MergeResolveUtil.kt b/platform/diff-impl/src/com/intellij/diff/comparison/MergeResolveUtil.kt index 4096c79fc003..df886328b8e6 100644 --- a/platform/diff-impl/src/com/intellij/diff/comparison/MergeResolveUtil.kt +++ b/platform/diff-impl/src/com/intellij/diff/comparison/MergeResolveUtil.kt @@ -16,13 +16,29 @@ package com.intellij.diff.comparison import com.intellij.diff.fragments.DiffFragment +import com.intellij.diff.util.DiffUtil import com.intellij.diff.util.Side import com.intellij.diff.util.Side.LEFT import com.intellij.diff.util.Side.RIGHT +import com.intellij.diff.util.TextDiffType +import com.intellij.diff.util.ThreeSide import com.intellij.openapi.progress.DumbProgressIndicator import com.intellij.util.text.MergingCharSequence object MergeResolveUtil { + @JvmStatic + fun tryResolve(leftText: CharSequence, baseText: CharSequence, rightText: CharSequence): CharSequence? { + try { + val resolved = tryResolve(leftText, baseText, rightText, ComparisonPolicy.DEFAULT) + if (resolved != null) return resolved + + return tryResolve(leftText, baseText, rightText, ComparisonPolicy.IGNORE_WHITESPACES) + } + catch (e: DiffTooBigException) { + return null + } + } + /* * Here we assume, that resolve results are explicitly verified by user and can be safely undone. * Thus we trade higher chances of incorrect resolve for higher chances of correct resolve. @@ -36,12 +52,12 @@ object MergeResolveUtil { * modifications can be considered as "insertion + deletion" and resolved accordingly. */ @JvmStatic - fun tryResolveConflict(leftText: CharSequence, baseText: CharSequence, rightText: CharSequence): CharSequence? { + fun tryGreedyResolve(leftText: CharSequence, baseText: CharSequence, rightText: CharSequence): CharSequence? { try { - val resolved = Helper(leftText, baseText, rightText).execute(ComparisonPolicy.DEFAULT) + val resolved = tryGreedyResolve(leftText, baseText, rightText, ComparisonPolicy.DEFAULT) if (resolved != null) return resolved - return Helper(leftText, baseText, rightText).execute(ComparisonPolicy.IGNORE_WHITESPACES) + return tryGreedyResolve(leftText, baseText, rightText, ComparisonPolicy.IGNORE_WHITESPACES) } catch (e: DiffTooBigException) { return null @@ -49,6 +65,46 @@ object MergeResolveUtil { } } +private fun tryResolve(leftText: CharSequence, baseText: CharSequence, rightText: CharSequence, + policy: ComparisonPolicy): CharSequence? { + val texts = listOf(leftText, baseText, rightText) + + val changes = ByWord.compare(leftText, baseText, rightText, policy, DumbProgressIndicator.INSTANCE) + + val newContent = StringBuilder() + + var last = 0 + for (fragment in changes) { + val type = DiffUtil.getWordMergeType(fragment, texts, policy) + if (type.diffType == TextDiffType.CONFLICT) return null; + + val baseStart = fragment.getStartOffset(ThreeSide.BASE) + val baseEnd = fragment.getEndOffset(ThreeSide.BASE) + + newContent.append(baseText, last, baseStart) + + if (type.isChange(Side.LEFT)) { + val leftStart = fragment.getStartOffset(ThreeSide.LEFT) + val leftEnd = fragment.getEndOffset(ThreeSide.LEFT) + newContent.append(leftText, leftStart, leftEnd) + } + else { + val rightStart = fragment.getStartOffset(ThreeSide.RIGHT) + val rightEnd = fragment.getEndOffset(ThreeSide.RIGHT) + newContent.append(rightText, rightStart, rightEnd) + } + last = baseEnd + } + + newContent.append(baseText, last, baseText.length) + return newContent.toString() +} + +private fun tryGreedyResolve(leftText: CharSequence, baseText: CharSequence, rightText: CharSequence, + policy: ComparisonPolicy): CharSequence? { + return Helper(leftText, baseText, rightText).execute(policy) +} + private class Helper(val leftText: CharSequence, val baseText: CharSequence, val rightText: CharSequence) { val newContent = StringBuilder() diff --git a/platform/diff-impl/src/com/intellij/diff/util/DiffUtil.java b/platform/diff-impl/src/com/intellij/diff/util/DiffUtil.java index fc8d62f14922..3ade1e10cfd3 100644 --- a/platform/diff-impl/src/com/intellij/diff/util/DiffUtil.java +++ b/platform/diff-impl/src/com/intellij/diff/util/DiffUtil.java @@ -23,6 +23,7 @@ import com.intellij.diff.SuppressiveDiffTool; import com.intellij.diff.comparison.ByWord; import com.intellij.diff.comparison.ComparisonManager; import com.intellij.diff.comparison.ComparisonPolicy; +import com.intellij.diff.comparison.ComparisonUtil; import com.intellij.diff.contents.DiffContent; import com.intellij.diff.contents.DocumentContent; import com.intellij.diff.contents.EmptyContent; @@ -30,6 +31,7 @@ import com.intellij.diff.contents.FileContent; import com.intellij.diff.fragments.DiffFragment; import com.intellij.diff.fragments.LineFragment; import com.intellij.diff.fragments.MergeLineFragment; +import com.intellij.diff.fragments.MergeWordFragment; import com.intellij.diff.impl.DiffSettingsHolder; import com.intellij.diff.impl.DiffSettingsHolder.DiffSettings; import com.intellij.diff.requests.ContentDiffRequest; @@ -1069,6 +1071,36 @@ public class DiffUtil { return fragment.getStartLine(side) == fragment.getEndLine(side); } + @NotNull + public static MergeConflictType getWordMergeType(@NotNull MergeWordFragment fragment, + @NotNull List texts, + @NotNull ComparisonPolicy policy) { + return getMergeType((side) -> isWordMergeIntervalEmpty(fragment, side), + (side1, side2) -> compareWordMergeContents(fragment, texts, policy, side1, side2)); + } + + private static boolean compareWordMergeContents(@NotNull MergeWordFragment fragment, + @NotNull List texts, + @NotNull ComparisonPolicy policy, + @NotNull ThreeSide side1, + @NotNull ThreeSide side2) { + int start1 = fragment.getStartOffset(side1); + int end1 = fragment.getEndOffset(side1); + int start2 = fragment.getStartOffset(side2); + int end2 = fragment.getEndOffset(side2); + + CharSequence document1 = side1.select(texts); + CharSequence document2 = side2.select(texts); + + CharSequence content1 = document1.subSequence(start1, end1); + CharSequence content2 = document2.subSequence(start2, end2); + return ComparisonUtil.isEquals(content1, content2, policy); + } + + private static boolean isWordMergeIntervalEmpty(@NotNull MergeWordFragment fragment, @NotNull ThreeSide side) { + return fragment.getStartOffset(side) == fragment.getEndOffset(side); + } + // // Writable // diff --git a/platform/diff-impl/tests/com/intellij/diff/comparison/MergeResolveUtilTest.kt b/platform/diff-impl/tests/com/intellij/diff/comparison/MergeResolveUtilTest.kt index 1283f7ed8f46..c5c299bd593a 100644 --- a/platform/diff-impl/tests/com/intellij/diff/comparison/MergeResolveUtilTest.kt +++ b/platform/diff-impl/tests/com/intellij/diff/comparison/MergeResolveUtilTest.kt @@ -118,35 +118,35 @@ class MergeResolveUtilTest : DiffTestCase() { } fun testNonFailureConflicts() { - test( + testGreedy( "x X x", "x x", "x X Y x", "x Y x" ) - test( + testGreedy( "x X x", "x x", "x Y X x", "x Y x" ) - test( + testGreedy( "x X Y x", "x X x", "x Y x", "x x" ) - test( + testGreedy( "x X Y Z x", "x X x", "x Z x", "x x" ) - test( + testGreedy( "x A B C D E F G H K x", "x C F K x", "x A D H x", @@ -157,21 +157,21 @@ class MergeResolveUtilTest : DiffTestCase() { fun testConfusingConflicts() { // these cases might be a failure as well - test( + testGreedy( "x X x", "x x", "x Z x", "xZ x" ) - test( + testGreedy( "x X X x", "x X Y X x", "x x", "x Y x" ) - test( + testGreedy( "x X x", "x x", "x Y x", @@ -179,7 +179,7 @@ class MergeResolveUtilTest : DiffTestCase() { ) - test( + testGreedy( "x X X x", "x Y x", "x X Y x", @@ -187,8 +187,25 @@ class MergeResolveUtilTest : DiffTestCase() { ) } - private fun test(base: String, left: String, right: String, expected: String?) { - val actual = MergeResolveUtil.tryResolveConflict(left, base, right) - assertEquals(expected, actual?.toString()) + private fun testGreedy(base: String, left: String, right: String, expected: String?) { + test(base, left, right, expected, true); + } + + private fun test(base: String, left: String, right: String, expected: String?, isGreedy: Boolean = false) { + val simpleResult = MergeResolveUtil.tryResolve(left, base, right) + val magicResult = MergeResolveUtil.tryGreedyResolve(left, base, right); + + if (expected == null) { + assertNull(simpleResult) + assertNull(magicResult) + } + else if (isGreedy) { + assertNull(simpleResult) + assertEquals(expected, magicResult) + } + else { + assertEquals(expected, simpleResult) + assertEquals(expected, magicResult) + } } } diff --git a/platform/util/resources/misc/registry.properties b/platform/util/resources/misc/registry.properties index a21ae20afa6b..82cf604b0d4b 100644 --- a/platform/util/resources/misc/registry.properties +++ b/platform/util/resources/misc/registry.properties @@ -491,6 +491,8 @@ diff.divider.repainting.disable.blitting=true diff.divider.repainting.disable.blitting.description=Fix painting glitch on scrolling in diff - disable BLIT_SCROLL_MODE to force repainting with RepaintManager diff.merge.resolve.conflict.action.visible=true diff.merge.resolve.conflict.action.visible.description=Allows to resolve some conflict in merge in one click (with a high probability of wrong result) +diff.merge.resolve.conflict.action.use.greedy.approach=false +diff.merge.resolve.conflict.action.use.greedy.approach.description=Use greedy heuristic in attempt to resolve conflict. This leads to higher amounts of false-positive and true-positive results. diff.enable.psi.highlighting=true diff.enable.psi.highlighting.description=Enable advanced highlighting and code navigation in VCS content in diff viewers. diff.pass.rich.editor.context=false