NoteDb: Handle leading whitespace in change subjects

JGit's FooterLine parser trims leading spaces (but not all whitespace)
from any values it reads. This means that even though we represent the
leading spaces in subjects faithfully when storing, we can't read it
back out. Handle this minor variation.

Change-Id: I9eb358eecdbf05a0528dfcda9650d714847b0f75

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

@@ -555,6 +555,25 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
         .containsExactly(change.currentPatchSetId());
   }
 
+  @Test
+  public void leadingSpacesInSubject() throws Exception {
+    String subj = "   " + PushOneCommit.SUBJECT;
+    PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo,
+        subj, PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT);
+    PushOneCommit.Result r = push.to("refs/for/master");
+    r.assertOkStatus();
+    Change change = r.getChange().change();
+    assertThat(change.getSubject()).isEqualTo(subj);
+    Change.Id id = r.getPatchSetId().getParentKey();
+
+    checker.rebuildAndCheckChanges(id);
+
+    notesMigration.setAllEnabled(true);
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    assertThat(notes.getChange().getSubject()).isNotEqualTo(subj);
+    assertThat(notes.getChange().getSubject()).isEqualTo(PushOneCommit.SUBJECT);
+  }
+
   private void setInvalidNoteDbState(Change.Id id) throws Exception {
     ReviewDb db = unwrapDb();
     Change c = db.changes().get(id);

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

@@ -413,12 +413,16 @@ public class ChangeBundle {
     String desc = a.getId().equals(b.getId()) ? describe(a.getId()) : "Changes";
 
     boolean excludeCreatedOn = false;
-    boolean excludeSubject = false;
-    boolean excludeOrigSubj = false;
     boolean excludeTopic = false;
     Timestamp aUpdated = a.getLastUpdatedOn();
     Timestamp bUpdated = b.getLastUpdatedOn();
 
+    CharMatcher s = CharMatcher.is(' ');
+    boolean excludeSubject = false;
+    boolean excludeOrigSubj = false;
+    String aSubj = a.getSubject();
+    String bSubj = b.getSubject();
+
     // Allow created timestamp in NoteDb to be either the created timestamp of
     // the change, or the timestamp of the first remaining patch set.
     //
@@ -444,34 +448,39 @@ public class ChangeBundle {
     // Ignore original subject on the ReviewDb side if it equals the subject of
     // the current patch set.
     //
+    // For all of the above subject comparisons, first trim any leading spaces
+    // from the NoteDb strings. (We actually do represent the leading spaces
+    // faithfully during conversion, but JGit's FooterLine parser trims them
+    // when reading.)
+    //
     // Ignore empty topic on the ReviewDb side if it is null on the NoteDb side.
     //
     // Use max timestamp of all ReviewDb entities when comparing with NoteDb.
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
-      excludeSubject = b.getSubject().startsWith(a.getSubject());
+      aSubj = s.trimLeadingFrom(aSubj);
+      excludeSubject = bSubj.startsWith(aSubj);
       excludeOrigSubj = true;
       excludeTopic = "".equals(a.getTopic()) && b.getTopic() == null;
       aUpdated = bundleA.getLatestTimestamp();
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
-      excludeSubject = a.getSubject().startsWith(b.getSubject());
+      bSubj = s.trimLeadingFrom(bSubj);
+      excludeSubject = aSubj.startsWith(bSubj);
       excludeOrigSubj = true;
       excludeTopic = a.getTopic() == null && "".equals(b.getTopic());
       bUpdated = bundleB.getLatestTimestamp();
     }
 
+    String subjectField = "subject";
     String updatedField = "lastUpdatedOn";
-    List<String> exclude =
+    List<String> exclude = Lists.newArrayList(
-        Lists.newArrayList(updatedField, "noteDbState", "rowVersion");
+        subjectField, updatedField, "noteDbState", "rowVersion");
     if (excludeCreatedOn) {
       exclude.add("createdOn");
     }
-    if (excludeSubject) {
-      exclude.add("subject");
-    }
     if (excludeOrigSubj) {
       exclude.add("originalSubject");
     }
@@ -489,6 +498,9 @@ public class ChangeBundle {
       diffTimestamps(diffs, desc, bundleA, aUpdated, bundleB, bUpdated,
           "effective last updated time");
     }
+    if (!excludeSubject) {
+      diffValues(diffs, desc, aSubj, bSubj, subjectField);
+    }
   }
 
   /**

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

@@ -365,6 +365,64 @@ public class ChangeBundleTest {
             + " {Change subject} != {Change sub}");
   }
 
+  @Test
+  public void diffChangesTrimsLeadingSpacesFromReviewDbComparingToNoteDb()
+      throws Exception {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(c1.currentPatchSetId(),
+        "   " + c1.getSubject(), c1.getOriginalSubject());
+
+    // Both ReviewDb, exact match required.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {Change subject} != {   Change subject}");
+
+    // ReviewDb is missing leading spaces, allowed.
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), NOTE_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    assertNoDiffs(b1, b2);
+    assertNoDiffs(b2, b1);
+  }
+
+  @Test
+  public void diffChangesDoesntTrimLeadingNonSpaceWhitespaceFromSubject()
+      throws Exception {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(c1.currentPatchSetId(),
+        "\t" + c1.getSubject(), c1.getOriginalSubject());
+
+    // Both ReviewDb.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {Change subject} != {\tChange subject}");
+
+    // One NoteDb.
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), NOTE_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {Change subject} != {\tChange subject}");
+    assertDiffs(b2, b1,
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {\tChange subject} != {Change subject}");
+  }
+
   @Test
   public void diffChangeMessageKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);

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

@@ -47,6 +47,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
+import com.google.gerrit.testutil.TestChanges;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 
@@ -752,6 +753,34 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(ts10).isGreaterThan(ts9);
   }
 
+  @Test
+  public void subjectLeadingWhitespaceChangeNotes() throws Exception {
+    Change c = TestChanges.newChange(project, changeOwner.getAccountId());
+    String trimmedSubj = c.getSubject();
+    c.setCurrentPatchSet(c.currentPatchSetId(), "  " + trimmedSubj,
+        c.getOriginalSubject());
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setChangeId(c.getKey().get());
+    update.setBranch(c.getDest().get());
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getChange().getSubject()).isEqualTo(trimmedSubj);
+
+    String tabSubj = "\t\t" + trimmedSubj;
+
+    c = TestChanges.newChange(project, changeOwner.getAccountId());
+    c.setCurrentPatchSet(c.currentPatchSetId(), tabSubj,
+        c.getOriginalSubject());
+    update = newUpdate(c, changeOwner);
+    update.setChangeId(c.getKey().get());
+    update.setBranch(c.getDest().get());
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(notes.getChange().getSubject()).isEqualTo(tabSubj);
+  }
+
   @Test
   public void commitChangeNotesUnique() throws Exception {
     // PatchSetId -> RevId must be a one to one mapping

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

@@ -289,6 +289,43 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         update.getResult());
   }
 
+  @Test
+  public void leadingWhitespace() throws Exception {
+    Change c = TestChanges.newChange(project, changeOwner.getAccountId());
+    c.setCurrentPatchSet(c.currentPatchSetId(), "  " + c.getSubject(),
+        c.getOriginalSubject());
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setChangeId(c.getKey().get());
+    update.setBranch(c.getDest().get());
+    update.commit();
+
+    assertBodyEquals("Update patch set 1\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Change-id: " + c.getKey().get() + "\n"
+        + "Subject:   Change subject\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: " + update.getCommit().name() + "\n",
+        update.getResult());
+
+    c = TestChanges.newChange(project, changeOwner.getAccountId());
+    c.setCurrentPatchSet(c.currentPatchSetId(), "\t\t" + c.getSubject(),
+        c.getOriginalSubject());
+    update = newUpdate(c, changeOwner);
+    update.setChangeId(c.getKey().get());
+    update.setBranch(c.getDest().get());
+    update.commit();
+
+    assertBodyEquals("Update patch set 1\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Change-id: " + c.getKey().get() + "\n"
+        + "Subject: \t\tChange subject\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: " + update.getCommit().name() + "\n",
+        update.getResult());
+  }
+
   private RevCommit parseCommit(ObjectId id) throws Exception {
     if (id instanceof RevCommit) {
       return (RevCommit) id;