Merge "ChangeBundle: Ignore originalSubject between ReviewDb and NoteDb"

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
+    // Ignore original subject on the ReviewDb side when comparing to NoteDb.
-    // always set in 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);
-
+    assertNoDiffs(b2, b1);
-    // 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}");
   }
 
   @Test