Handle change subjects containing '\r' in NoteDb

JGit's RevCommit#getShortMessage() might return a subject containing
"\r\r". If we put this in the Subject footer of a NoteDb commit, which
we do in the commit that creates the patch set, then
RevCommit#getFooterLines() will treat "\r\r" as a new paragraph,
resulting in ignoring some footers.

Fix this by sanitizing '\r' to ' ', along with '\n' and '\0', which were
already sanitized in other contexts.

Change-Id: I449eded06ecbf713dd073f2d8c77dca721ba3d23

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -1216,6 +1216,22 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     rebuilderWrapper.rebuild(db, id);
   }
 
+  @Test
+  public void commitWithCrLineEndings() throws Exception {
+    PushOneCommit.Result r =
+        createChange("Subject\r\rBody\r", PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT);
+    Change c = r.getChange().change();
+
+    // This assertion demonstrates an arguable bug in JGit's commit subject
+    // parsing, and shows how this kind of data might have gotten into
+    // ReviewDb. If that bug ever gets fixed upstream, this assert may start
+    // failing. If that happens, this test can be rewritten to directly set the
+    // subject field in ReviewDb.
+    assertThat(c.getSubject()).isEqualTo("Subject\r\rBody");
+
+    checker.rebuildAndCheckChanges(c.getId());
+  }
+
   private void assertChangesReadOnly(RestApiException e) throws Exception {
     Throwable cause = e.getCause();
     assertThat(cause).isInstanceOf(UpdateException.class);

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -434,6 +434,7 @@ public class ChangeBundle {
       excludeCreatedOn =
           !timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
       aSubj = cleanReviewDbSubject(aSubj);
+      bSubj = cleanNoteDbSubject(bSubj);
       excludeCurrentPatchSetId = !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
       excludeSubject = bSubj.startsWith(aSubj) || excludeCurrentPatchSetId;
       excludeOrigSubj = true;
@@ -444,6 +445,7 @@ public class ChangeBundle {
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludeCreatedOn =
           !timestampsDiffer(bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
+      aSubj = cleanNoteDbSubject(aSubj);
       bSubj = cleanReviewDbSubject(bSubj);
       excludeCurrentPatchSetId = !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
       excludeSubject = aSubj.startsWith(bSubj) || excludeCurrentPatchSetId;
@@ -501,7 +503,11 @@ public class ChangeBundle {
     if (rn >= 0) {
       s = s.substring(0, rn);
     }
-    return s;
+    return ChangeNoteUtil.sanitizeFooter(s);
+  }
+
+  private static String cleanNoteDbSubject(String s) {
+    return ChangeNoteUtil.sanitizeFooter(s);
   }
 
   /**

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.ChangeNotes.parseException;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.CharMatcher;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.primitives.Ints;
@@ -619,4 +620,17 @@ public class ChangeNoteUtil {
     name.append('>');
     appendHeaderField(writer, header, name.toString());
   }
+
+  private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0");
+
+  static String sanitizeFooter(String value) {
+    // Remove characters that would confuse JGit's footer parser if they were
+    // included in footer values, for example by splitting the footer block into
+    // multiple paragraphs.
+    //
+    // One painful example: RevCommit#getShorMessage() might return a message
+    // containing "\r\r", which RevCommit#getFooterLines() will treat as an
+    // empty paragraph for the purposes of footer parsing.
+    return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' ');
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -36,6 +36,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMISSION_I
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.sanitizeFooter;
 import static java.util.Comparator.comparing;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
@@ -599,7 +600,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
 
     if (subject != null) {
-      addFooter(msg, FOOTER_SUBJECT, subject);
+      addFooter(msg, FOOTER_SUBJECT, sanitizeFooter(subject));
     }
 
     if (branch != null) {
@@ -779,8 +780,4 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     sb.append('>');
     return sb;
   }
-
-  private static String sanitizeFooter(String value) {
-    return value.replace('\n', ' ').replace('\0', ' ');
-  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java

@@ -247,6 +247,55 @@ public class ChangeBundleTest extends GerritBaseTests {
     assertNoDiffs(b2, b1);
   }
 
+  @Test
+  public void diffChangesSanitizesSubjectsBeforeComparison() throws Exception {
+    Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
+    c1.setCurrentPatchSet(c1.currentPatchSetId(), "Subject\r\rbody", "Original");
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(c2.currentPatchSetId(), "Subject  body", "Original");
+
+    // Both ReviewDb, exact match required
+    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,
+        "subject differs for Change.Id "
+            + c1.getId()
+            + ":"
+            + " {Subject\r\rbody} != {Subject  body}");
+
+    // Both NoteDb, exact match required (although it should be impossible to
+    // create a NoteDb change with '\r' in the subject).
+    b1 =
+        new ChangeBundle(
+            c1, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+    b2 =
+        new ChangeBundle(
+            c2, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
+    assertDiffs(
+        b1,
+        b2,
+        "subject differs for Change.Id "
+            + c1.getId()
+            + ":"
+            + " {Subject\r\rbody} != {Subject  body}");
+
+    // One ReviewDb, one NoteDb, '\r' is normalized to ' '.
+    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 diffChangesConsidersEmptyReviewDbTopicEquivalentToNullInNoteDb() throws Exception {
     Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));