From 56d316ff7c7c34b9b8fd54c9bc01b7cf32056a9b Mon Sep 17 00:00:00 2001 From: Martin Waitz Date: Fri, 18 Mar 2016 09:06:17 +0100 Subject: [PATCH] IntraLineLoader: remove special-casing of whitespace The heuristic for presentation of identation spaces was broken. It is quite complicated code and produces incorrect edits. So remove it in order to make it easier to refactor the code. The goal of presenting whitespace changes in one visual block is still good, but we should try to find a better heuristic. Bug: Issue 3423 Bug-tracked-down-by: Patrick Steinhardt Change-Id: Ifd18c33579c47342092a139a44eb1855ebb124ef --- .../gerrit/server/patch/IntraLineLoader.java | 106 ++++-------------- .../server/patch/IntraLineLoaderTest.java | 5 +- 2 files changed, 22 insertions(+), 89 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index b6b7fb7cec..de70478d92 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -186,80 +186,34 @@ class IntraLineLoader implements Callable { // The leading part of an edit and its trailing part in the same // text might be identical. Slide down that edit and use the tail - // rather than the leading bit. If however the edit is only on a - // whitespace block try to shift it to the left margin, assuming - // that it is an indentation change. + // rather than the leading bit. // - boolean aShift = true; - if (ab < ae && isOnlyWhitespace(a, ab, ae)) { - int lf = findLF(wordEdits, j, a, ab); - if (lf < ab && a.charAt(lf) == '\n') { - int nb = lf + 1; - int p = 0; - while (p < ae - ab) { - if (cmp.equals(a, ab + p, a, ab + p)) { - p++; - } else { - break; - } - } - if (p == ae - ab) { - ab = nb; - ae = nb + p; - aShift = false; - } - } + while (0 < ab && ab < ae && a.charAt(ab - 1) != '\n' + && cmp.equals(a, ab - 1, a, ae - 1)) { + ab--; + ae--; } - if (aShift) { - while (0 < ab && ab < ae && a.charAt(ab - 1) != '\n' - && cmp.equals(a, ab - 1, a, ae - 1)) { - ab--; - ae--; - } - if (!a.isLineStart(ab) || !a.contains(ab, ae, '\n')) { - while (ab < ae && ae < a.size() && cmp.equals(a, ab, a, ae)) { - ab++; - ae++; - if (a.charAt(ae - 1) == '\n') { - break; - } + if (!a.isLineStart(ab) || !a.contains(ab, ae, '\n')) { + while (ab < ae && ae < a.size() && cmp.equals(a, ab, a, ae)) { + ab++; + ae++; + if (a.charAt(ae - 1) == '\n') { + break; } } } - boolean bShift = true; - if (bb < be && isOnlyWhitespace(b, bb, be)) { - int lf = findLF(wordEdits, j, b, bb); - if (lf < bb && b.charAt(lf) == '\n') { - int nb = lf + 1; - int p = 0; - while (p < be - bb) { - if (cmp.equals(b, bb + p, b, bb + p)) { - p++; - } else { - break; - } - } - if (p == be - bb) { - bb = nb; - be = nb + p; - bShift = false; - } - } + while (0 < bb && bb < be && b.charAt(bb - 1) != '\n' + && cmp.equals(b, bb - 1, b, be - 1)) { + bb--; + be--; } - if (bShift) { - while (0 < bb && bb < be && b.charAt(bb - 1) != '\n' - && cmp.equals(b, bb - 1, b, be - 1)) { - bb--; - be--; - } - if (!b.isLineStart(bb) || !b.contains(bb, be, '\n')) { - while (bb < be && be < b.size() && cmp.equals(b, bb, b, be)) { - bb++; - be++; - if (b.charAt(be - 1) == '\n') { - break; - } + if (!b.isLineStart(bb) || !b.contains(bb, be, '\n')) { + while (bb < be && be < b.size() && cmp.equals(b, bb, b, be)) { + bb++; + be++; + if (b.charAt(be - 1) == '\n') { + break; } } } @@ -342,22 +296,4 @@ class IntraLineLoader implements Callable { } return true; } - - private static int findLF(List edits, int j, CharText t, int b) { - int lf = b; - int limit = 0 < j ? edits.get(j - 1).getEndB() : 0; - while (limit < lf && t.charAt(lf) != '\n') { - lf--; - } - return lf; - } - - private static boolean isOnlyWhitespace(CharText t, final int b, final int e) { - for (int c = b; c < e; c++) { - if (!Character.isWhitespace(t.charAt(c))) { - return false; - } - } - return b < e; - } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java index 617c17c289..7bddadeff7 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java @@ -128,10 +128,7 @@ public class IntraLineLoaderTest { ); } - //TODO: expected failure, issue #3423 - // The current code wrongly marks the first space of the second line, - // instead of the one inserted after the '*'. - @Test(expected = AssertionError.class) + @Test public void insertedWhitespaceIsRecognizedInMultipleLines() throws Exception { // |0 5 10 | 5 20 5 30 String a = " int *foobar\n int *foobar\n";