diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index 1deabe7d75..6b03b99319 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/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); 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 0b183158ab..c8f75ce4f0 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 @@ -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(); } 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 decda283b3..8b6b590c8b 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 @@ -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