From 8b98f0c651d8fe23894d08aa1e839004f13eaa6a Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Fri, 27 May 2016 20:40:17 -0400
Subject: [PATCH] ChangeBundle: Handle change subjects extracted with a buggy
 JGit

Change-Id: I92f08bbd26e47cf4162519814fe59181f330606c
---
 .../gerrit/server/notedb/ChangeBundle.java    | 21 ++++++++++++--
 .../server/notedb/ChangeBundleTest.java       | 28 +++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
index 38e2a3f981..f5202f4759 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
@@ -446,7 +446,6 @@ public class ChangeBundle {
     Timestamp aUpdated = a.getLastUpdatedOn();
     Timestamp bUpdated = b.getLastUpdatedOn();
 
-    CharMatcher s = CharMatcher.is(' ');
     boolean excludeSubject = false;
     boolean excludeOrigSubj = false;
     // Subject is not technically a nullable field, but we observed some null
@@ -490,7 +489,7 @@ public class ChangeBundle {
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
-      aSubj = s.trimLeadingFrom(aSubj);
+      aSubj = cleanReviewDbSubject(aSubj);
       excludeSubject = bSubj.startsWith(aSubj);
       excludeOrigSubj = true;
       String aTopic = trimLeadingOrNull(a.getTopic());
@@ -500,7 +499,7 @@ public class ChangeBundle {
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
-      bSubj = s.trimLeadingFrom(bSubj);
+      bSubj = cleanReviewDbSubject(bSubj);
       excludeSubject = aSubj.startsWith(bSubj);
       excludeOrigSubj = true;
       String bTopic = trimLeadingOrNull(b.getTopic());
@@ -542,6 +541,22 @@ public class ChangeBundle {
     return s != null ? CharMatcher.whitespace().trimLeadingFrom(s) : null;
   }
 
+  private static String cleanReviewDbSubject(String s) {
+    s = CharMatcher.is(' ').trimLeadingFrom(s);
+
+    // An old JGit bug failed to extract subjects from commits with "\r\n"
+    // terminators: https://bugs.eclipse.org/bugs/show_bug.cgi?id=400707
+    // Changes created with this bug may have "\r\n" converted to "\r " and the
+    // entire commit in the subject. The version of JGit used to read NoteDb
+    // changes parses these subjects correctly, so we need to clean up old
+    // ReviewDb subjects before comparing.
+    int rn = s.indexOf("\r \r ");
+    if (rn >= 0) {
+      s = s.substring(0, rn);
+    }
+    return s;
+  }
+
   /**
    * Set of fields that must always exactly match between ReviewDb and NoteDb.
    * <p>
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
index 8be7f264b3..978682b0e9 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
@@ -479,6 +479,34 @@ public class ChangeBundleTest {
             + " {\tChange subject} != {Change subject}");
   }
 
+  @Test
+  public void diffChangesHandlesBuggyJGitSubjectExtraction() throws Exception {
+    Change c1 = TestChanges.newChange(project, accountId);
+    String buggySubject = "Subject\r \r Rest of message.";
+    c1.setCurrentPatchSet(c1.currentPatchSetId(), buggySubject, buggySubject);
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(c2.currentPatchSetId(), "Subject", "Subject");
+
+    // Both ReviewDb.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), reviewers(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), reviewers(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "originalSubject differs for Change.Id " + c1.getId() + ":"
+            + " {Subject\r \r Rest of message.} != {Subject}",
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {Subject\r \r Rest of message.} != {Subject}");
+
+    // NoteDb has correct subject without "\r ".
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), reviewers(), REVIEW_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), reviewers(), NOTE_DB);
+    assertNoDiffs(b1, b2);
+    assertNoDiffs(b2, b1);
+  }
+
   @Test
   public void diffChangeMessageKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);