Fix infinite loop in IntraLineLoader

If edits due to rebase are present, IntraLineLoader#combineLineEdits
will enter an infinite loop. As we query the intraline diff cache with a
timeout check and use a fall-back behavior in case of a timeout, none of
our threads got completely stuck due to that bug. On the one hand, this
meant that the Gerrit server could still serve requests. On the other
hand, it concealed that bug and resulted in high CPU load, which could
very negatively impact the whole Gerrit server.

The fix of the issue is quite simple: We increase the index counter
appropriately. As this issue shows, the implementation in
IntraLineLoader#combineLineEdits is quite fragile. There are better ways
to achieve the same computation. Adjusting that will be left for the
future.

None of our tests caught that issue as the result of the fall-back
mechanism was the same as the result of the real computation for all
tests which were affected. For that reason, we add a test whose results
differ between the regular and the fall-back mechanism. In addition, we
add some tests which cover intraline edits directly. Up to now, we
didn't have any tests for them. We could and should certainly add more
in the future.

Ideally, we would also use a timeout for all tests within
RevisionDiffIT. Normally, this would be simply achieved by using the
Timeout rule of JUnit. However, adding that rule breaks all tests within
that class as our implementation of ConfigSuite doesn't seem to nicely
interact with arbitrary JUnit rules.

As the result of timed out intraline computations are cached, we need to
invalidate the intraline diff cache. The high CPU load also caused a lot
of PatchList cache computations to time out, for which also the
fall-back result was cached. Hence, we invalidate that cache as well.

Bug: Issue 8742
Change-Id: I0abf392d398320964a021099d72a4f876a73b6a6

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java

@@ -23,6 +23,7 @@ import static java.util.stream.Collectors.joining;
 import static java.util.stream.Collectors.toMap;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
@@ -82,6 +83,11 @@ public class RevisionDiffIT extends AbstractDaemonTest {
 
   @Before
   public void setUp() throws Exception {
+    // Reduce flakiness of tests. (If tests aren't fast enough, we would use a fall-back
+    // computation, which might yield different results.)
+    baseConfig.setString("cache", "diff", "timeout", "1 minute");
+    baseConfig.setString("cache", "diff_intraline", "timeout", "1 minute");
+
     intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false);
 
     ObjectId headCommit = testRepo.getRepository().resolve("HEAD");
@@ -1253,6 +1259,66 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1);
   }
 
+  @Test
+  public void intralineEditsInNonRebaseHunksAreIdentified() throws Exception {
+    assume().that(intraline).isTrue();
+
+    Function<String, String> contentModification =
+        fileContent -> fileContent.replace("Line 1\n", "Line one\n");
+    addModifiedPatchSet(changeId, FILE_NAME, contentModification);
+
+    DiffInfo diffInfo =
+        getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
+    assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
+    assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
+    assertThat(diffInfo)
+        .content()
+        .element(0)
+        .intralineEditsOfA()
+        .containsExactly(ImmutableList.of(5, 1));
+    assertThat(diffInfo)
+        .content()
+        .element(0)
+        .intralineEditsOfB()
+        .containsExactly(ImmutableList.of(5, 3));
+    assertThat(diffInfo).content().element(0).isNotDueToRebase();
+    assertThat(diffInfo).content().element(1).commonLines().hasSize(99);
+  }
+
+  @Test
+  public void intralineEditsInRebaseHunksAreIdentified() throws Exception {
+    assume().that(intraline).isTrue();
+
+    String newFileContent = FILE_CONTENT.replace("Line 1\n", "Line one\n");
+    ObjectId commit2 = addCommit(commit1, FILE_NAME, newFileContent);
+
+    rebaseChangeOn(changeId, commit2);
+    Function<String, String> contentModification =
+        fileContent -> fileContent.replace("Line 50\n", "Line fifty\n");
+    addModifiedPatchSet(changeId, FILE_NAME, contentModification);
+
+    DiffInfo diffInfo =
+        getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get();
+    assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1");
+    assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one");
+    assertThat(diffInfo)
+        .content()
+        .element(0)
+        .intralineEditsOfA()
+        .containsExactly(ImmutableList.of(5, 1));
+    assertThat(diffInfo)
+        .content()
+        .element(0)
+        .intralineEditsOfB()
+        .containsExactly(ImmutableList.of(5, 3));
+    assertThat(diffInfo).content().element(0).isDueToRebase();
+    assertThat(diffInfo).content().element(1).commonLines().hasSize(48);
+    assertThat(diffInfo).content().element(2).linesOfA().containsExactly("Line 50");
+    assertThat(diffInfo).content().element(2).linesOfB().containsExactly("Line fifty");
+    assertThat(diffInfo).content().element(2).isNotDueToRebase();
+    assertThat(diffInfo).content().element(3).commonLines().hasSize(50);
+  }
+
   @Test
   public void closeNonRebaseHunksAreCombinedForIntralineOptimizations() throws Exception {
     assume().that(intraline).isTrue();
@@ -1365,6 +1431,47 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1);
   }
 
+  @Test
+  public void closeNonRebaseHunksNextToRebaseHunksAreCombinedForIntralineOptimizations()
+      throws Exception {
+    assume().that(intraline).isTrue();
+
+    String fileContent = FILE_CONTENT.replace("Line 5\n", "{\n").replace("Line 7\n", "{\n");
+    ObjectId commit2 = addCommit(commit1, FILE_NAME, fileContent);
+    rebaseChangeOn(changeId, commit2);
+    String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+
+    String newFileContent = fileContent.replace("Line 8\n", "Line eight!\n");
+    ObjectId commit3 = addCommit(commit1, FILE_NAME, newFileContent);
+    rebaseChangeOn(changeId, commit3);
+
+    addModifiedPatchSet(
+        changeId,
+        FILE_NAME,
+        content -> content.replace("Line 4\n", "Line four\n").replace("Line 6\n", "Line six\n"));
+
+    DiffInfo diffInfo =
+        getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get();
+    assertThat(diffInfo).content().element(0).commonLines().hasSize(3);
+    assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 4", "{", "Line 6");
+    assertThat(diffInfo)
+        .content()
+        .element(1)
+        .linesOfB()
+        .containsExactly("Line four", "{", "Line six");
+    assertThat(diffInfo).content().element(1).isNotDueToRebase();
+    assertThat(diffInfo).content().element(2).commonLines().hasSize(1);
+    assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 8");
+    assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line eight!");
+    assertThat(diffInfo).content().element(3).isDueToRebase();
+    assertThat(diffInfo).content().element(4).commonLines().hasSize(92);
+
+    Map<String, FileInfo> changedFiles =
+        gApi.changes().id(changeId).current().files(previousPatchSetId);
+    assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(2);
+    assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(2);
+  }
+
   private void assertDiffForNewFile(
       PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception {
     DiffInfo diff =

gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java

@@ -21,7 +21,7 @@ import org.eclipse.jgit.lib.ObjectId;
 
 @AutoValue
 public abstract class IntraLineDiffKey implements Serializable {
-  public static final long serialVersionUID = 11L;
+  public static final long serialVersionUID = 12L;
 
   public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) {
     return new AutoValue_IntraLineDiffKey(aId, bId, whitespace);

gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java

@@ -271,6 +271,7 @@ class IntraLineLoader implements Callable<IntraLineDiff> {
       if (editsDueToRebase.contains(c) || editsDueToRebase.contains(n)) {
         // Don't combine any edits which were identified as being introduced by a rebase as we would
         // lose that information because of the combination.
+        j++;
         continue;
       }
 

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java

@@ -32,7 +32,7 @@ import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.ObjectId;
 
 public class PatchListKey implements Serializable {
-  public static final long serialVersionUID = 30L;
+  public static final long serialVersionUID = 31L;
 
   public static final ImmutableBiMap<Whitespace, Character> WHITESPACE_TYPES =
       ImmutableBiMap.of(

gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/ContentEntrySubject.java

@@ -17,6 +17,7 @@ package com.google.gerrit.extensions.common;
 import static com.google.common.truth.Truth.assertAbout;
 
 import com.google.common.truth.FailureStrategy;
+import com.google.common.truth.IterableSubject;
 import com.google.common.truth.StringSubject;
 import com.google.common.truth.Subject;
 import com.google.common.truth.SubjectFactory;
@@ -78,4 +79,16 @@ public class ContentEntrySubject extends Subject<ContentEntrySubject, ContentEnt
     ContentEntry contentEntry = actual();
     return ListSubject.assertThat(contentEntry.b, Truth::assertThat).named("lines of 'b'");
   }
+
+  public IterableSubject intralineEditsOfA() {
+    isNotNull();
+    ContentEntry contentEntry = actual();
+    return Truth.assertThat(contentEntry.editA).named("intraline edits of 'a'");
+  }
+
+  public IterableSubject intralineEditsOfB() {
+    isNotNull();
+    ContentEntry contentEntry = actual();
+    return Truth.assertThat(contentEntry.editB).named("intraline edits of 'b'");
+  }
 }