ChangeBundle: Ignore originalSubject between ReviewDb and NoteDb

The migration path in Change#setCurrentPatchSet means that the
original subject field in ReviewDb may be null, or may be the actual
original subject of PS1, or may be the subject of any intervening
patch set, if that patch set was the first one pushed after adding the
column to ReviewDb. This means we can't reliably compare these after
converting to NoteDb, at least without changing the implementation of
ChangeBundle substantially in order to give it access to the
repository.

Satisfy ourselves with explicitly checking the subject and
originalSubject in ChangeRebuilderIT to ensure it's populated properly
in the common case.

Change-Id: I2160bf6f38b5ea97e0c3ee512a0450854e1af265

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

@@ -459,6 +459,35 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     checker.rebuildAndCheckChanges(id);
   }
 
+  @Test
+  public void noteDbUsesOriginalSubjectFromPatchSetAndIgnoresChangeField()
+      throws Exception {
+    PushOneCommit.Result r = createChange();
+    String orig = r.getChange().change().getSubject();
+    r = pushFactory.create(
+            db, admin.getIdent(), testRepo, orig + " v2",
+            PushOneCommit.FILE_NAME, "new contents", r.getChangeId())
+        .to("refs/heads/master");
+    r.assertOkStatus();
+
+    PatchSet.Id psId = r.getPatchSetId();
+    Change.Id id = psId.getParentKey();
+    Change c = db.changes().get(id);
+
+    c.setCurrentPatchSet(psId, c.getSubject(), "Bogus original subject");
+    db.changes().update(Collections.singleton(c));
+
+    checker.rebuildAndCheckChanges(id);
+
+    notesMigration.setAllEnabled(true);
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    Change nc = notes.getChange();
+    assertThat(nc.getSubject()).isEqualTo(c.getSubject());
+    assertThat(nc.getSubject()).isEqualTo(orig + " v2");
+    assertThat(nc.getOriginalSubject()).isNotEqualTo(c.getOriginalSubject());
+    assertThat(nc.getOriginalSubject()).isEqualTo(orig);
+  }
+
   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

@@ -346,20 +346,34 @@ public class ChangeBundle {
     // subject historically may have been truncated to fit in a SQL varchar
     // column.
     //
-    // Ignore null original subject on the ReviewDb side, as this field is
-    // always set in NoteDb.
+    // Ignore original subject on the ReviewDb side when comparing to NoteDb.
+    // This field may have any number of values:
+    //  - It may be null, if the change has had no new patch sets pushed since
+    //    migrating to schema 103.
+    //  - It may match the first patch set subject, if the change was created
+    //    after migrating to schema 103.
+    //  - It may match the subject of the first patch set that was pushed after
+    //    the migration to schema 103, even though that is neither the subject
+    //    of the first patch set nor the subject of the last patch set. (See
+    //    Change#setCurrentPatchSet as of 43b10f86 for this behavior.) This
+    //    subject of an intermediate patch set is not available to the
+    //    ChangeBundle; we would have to get the subject from the repo, which is
+    //    inconvenient at this point.
+    //
+    // Ignore original subject on the ReviewDb side if it equals the subject of
+    // the current patch set.
     //
     // 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) {
       excludeSubject = b.getSubject().startsWith(a.getSubject());
-      excludeOrigSubj = a.getOriginalSubjectOrNull() == null;
+      excludeOrigSubj = true;
       excludeTopic = "".equals(a.getTopic()) && b.getTopic() == null;
       aUpdated = bundleA.getLatestTimestamp();
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludeSubject = a.getSubject().startsWith(b.getSubject());
-      excludeOrigSubj = b.getOriginalSubjectOrNull() == null;
+      excludeOrigSubj = true;
       excludeTopic = a.getTopic() == null && "".equals(b.getTopic());
       bUpdated = bundleB.getLatestTimestamp();
     }

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

@@ -182,14 +182,14 @@ public class ChangeBundleTest {
   }
 
   @Test
-  public void diffChangesIgnoresNullOriginalSubjectInReviewDb()
+  public void diffChangesIgnoresOriginalSubjectInReviewDb()
       throws Exception {
     Change c1 = TestChanges.newChange(
         new Project.NameKey("project"), new Account.Id(100));
-    c1.setCurrentPatchSet(c1.currentPatchSetId(), "Subject", null);
+    c1.setCurrentPatchSet(c1.currentPatchSetId(), "Subject", "Original A");
     Change c2 = clone(c1);
     c2.setCurrentPatchSet(
-        c2.currentPatchSetId(), c1.getSubject(), "Original subject");
+        c2.currentPatchSetId(), c1.getSubject(), "Original B");
 
     // Both ReviewDb, exact match required.
     ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
@@ -198,7 +198,7 @@ public class ChangeBundleTest {
         comments(), REVIEW_DB);
     assertDiffs(b1, b2,
         "originalSubject differs for Change.Id " + c1.getId() + ":"
-            + " {null} != {Original subject}");
+            + " {Original A} != {Original B}");
 
     // Both NoteDb, exact match required.
     b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
@@ -207,23 +207,15 @@ public class ChangeBundleTest {
         NOTE_DB);
     assertDiffs(b1, b2,
         "originalSubject differs for Change.Id " + c1.getId() + ":"
-            + " {null} != {Original subject}");
+            + " {Original A} != {Original B}");
 
-    // Original subject ignored if ReviewDb has null value.
+    // One ReviewDb, one NoteDb, original subject is ignored.
     b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
         REVIEW_DB);
     b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
         NOTE_DB);
     assertNoDiffs(b1, b2);
-
-    // Exact match still required if NoteDb has null value (not realistic).
-    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
-        NOTE_DB);
-    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
-        REVIEW_DB);
-    assertDiffs(b1, b2,
-        "originalSubject differs for Change.Id " + c1.getId() + ":"
-            + " {null} != {Original subject}");
+    assertNoDiffs(b2, b1);
   }
 
   @Test