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 7a7b7acde4..3d2c3d4027 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 @@ -790,7 +790,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { PushOneCommit.FILE_NAME, "new contents", r.getChangeId()) - .to("refs/heads/master"); + .to("refs/for/master"); r.assertOkStatus(); PatchSet.Id psId = r.getPatchSetId(); @@ -1232,6 +1232,48 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { checker.rebuildAndCheckChanges(c.getId()); } + @Test + public void patchSetsOutOfOrder() throws Exception { + String id = createChange().getChangeId(); + amendChange(id); + PushOneCommit.Result r = amendChange(id); + + ChangeData cd = r.getChange(); + PatchSet.Id psId3 = cd.change().currentPatchSetId(); + assertThat(psId3.get()).isEqualTo(3); + + PatchSet ps1 = db.patchSets().get(new PatchSet.Id(cd.getId(), 1)); + PatchSet ps3 = db.patchSets().get(psId3); + assertThat(ps1.getCreatedOn()).isLessThan(ps3.getCreatedOn()); + + // Simulate an old Gerrit bug by setting the created timestamp of the latest + // patch set ID to the timestamp of PS1. + ps3.setCreatedOn(ps1.getCreatedOn()); + db.patchSets().update(Collections.singleton(ps3)); + + checker.rebuildAndCheckChanges(cd.getId()); + + setNotesMigration(true, true); + cd = changeDataFactory.create(db, project, cd.getId()); + assertThat(cd.change().currentPatchSetId()).isEqualTo(psId3); + + List patchSets = ImmutableList.copyOf(cd.patchSets()); + assertThat(patchSets).hasSize(3); + + PatchSet newPs1 = patchSets.get(0); + assertThat(newPs1.getId()).isEqualTo(ps1.getId()); + assertThat(newPs1.getCreatedOn()).isEqualTo(ps1.getCreatedOn()); + + PatchSet newPs2 = patchSets.get(1); + assertThat(newPs2.getCreatedOn()).isGreaterThan(newPs1.getCreatedOn()); + + PatchSet newPs3 = patchSets.get(2); + assertThat(newPs3.getId()).isEqualTo(ps3.getId()); + // Migrated with a newer timestamp than the original, to preserve ordering. + assertThat(newPs3.getCreatedOn()).isAtLeast(newPs2.getCreatedOn()); + assertThat(newPs3.getCreatedOn()).isGreaterThan(ps1.getCreatedOn()); + } + private void assertChangesReadOnly(RestApiException e) throws Exception { Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(UpdateException.class); 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 1fe0e644ac..72859f0bbc 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 @@ -22,6 +22,7 @@ import static com.google.gerrit.common.TimeUtil.roundToSecond; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB; import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB; +import static java.util.stream.Collectors.toList; import com.google.auto.value.AutoValue; import com.google.common.base.CharMatcher; @@ -50,6 +51,7 @@ import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl; @@ -648,7 +650,26 @@ public class ChangeBundle { List diffs, ChangeBundle bundleA, ChangeBundle bundleB) { Map as = bundleA.patchSets; Map bs = bundleB.patchSets; - for (PatchSet.Id id : diffKeySets(diffs, as, bs)) { + Set ids = diffKeySets(diffs, as, bs); + + // Old versions of Gerrit had a bug that created patch sets during + // rebase or submission with a createdOn timestamp earlier than the patch + // set it was replacing. (In the cases I examined, it was equal to createdOn + // for the change, but we're not counting on this exact behavior.) + // + // ChangeRebuilder ensures patch set events come out in order, but it's hard + // to predict what the resulting timestamps would look like. So, completely + // ignore the createdOn timestamps if both: + // * ReviewDb timestamps are non-monotonic. + // * NoteDb timestamps are monotonic. + boolean excludeCreatedOn = false; + if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) { + excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids); + } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) { + excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids); + } + + for (PatchSet.Id id : ids) { PatchSet a = as.get(id); PatchSet b = bs.get(id); String desc = describe(id); @@ -662,6 +683,9 @@ public class ChangeBundle { } List exclude = Lists.newArrayList(pushCertField); + if (excludeCreatedOn) { + exclude.add("createdOn"); + } if (excludeDesc) { exclude.add("description"); } @@ -678,6 +702,18 @@ public class ChangeBundle { return CharMatcher.is('\n').trimTrailingFrom(ps.getPushCertificate()); } + private static boolean createdOnIsMonotonic( + Map patchSets, Set limitToIds) { + List orderedById = + patchSets + .values() + .stream() + .filter(ps -> limitToIds.contains(ps.getId())) + .sorted(ChangeUtil.PS_ID_ORDER) + .collect(toList()); + return Ordering.natural().onResultOf(PatchSet::getCreatedOn).isOrdered(orderedById); + } + private static void diffPatchSetApprovals( List diffs, ChangeBundle bundleA, ChangeBundle bundleB) { Map as = bundleA.filterPatchSetApprovals(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index 326ceb3f6e..15bcbdbcfa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; @@ -77,11 +76,12 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; +import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.TreeMap; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; @@ -304,8 +304,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { deleteDraftRefs(change, manager.getAllUsersRepo()); Integer minPsNum = getMinPatchSetNum(bundle); - Map patchSetEvents = - Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size()); + TreeMap patchSetEvents = + new TreeMap<>(ReviewDbUtil.intKeyOrdering()); for (PatchSet ps : bundle.getPatchSets()) { PatchSetEvent pse = new PatchSetEvent(change, ps, manager.getChangeRepo().rw); @@ -320,6 +320,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { draftCommentEvents.put(c.author.getId(), e); } } + ensurePatchSetOrder(patchSetEvents); for (PatchSetApproval psa : bundle.getPatchSetApprovals()) { PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId()); @@ -394,6 +395,19 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { return minPsNum; } + private static void ensurePatchSetOrder(TreeMap events) { + if (events.isEmpty()) { + return; + } + Iterator it = events.values().iterator(); + PatchSetEvent curr = it.next(); + while (it.hasNext()) { + PatchSetEvent next = it.next(); + next.addDep(curr); + curr = next; + } + } + private static List getComments( ChangeBundle bundle, String serverId, PatchLineComment.Status status, PatchSet ps) { return bundle 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 151a053079..fec80eadd5 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 @@ -1290,6 +1290,111 @@ public class ChangeBundleTest extends GerritBaseTests { b1, b2, "description differs for PatchSet.Id " + ps1.getId() + ":" + " { abc } != {cba}"); } + @Test + public void diffPatchSetsIgnoresCreatedOnWhenReviewDbIsNonMonotonic() throws Exception { + Change c = TestChanges.newChange(project, accountId); + + Timestamp beforePs1 = TimeUtil.nowTs(); + + PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1)); + goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + goodPs1.setUploader(accountId); + goodPs1.setCreatedOn(TimeUtil.nowTs()); + PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2)); + goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + goodPs2.setUploader(accountId); + goodPs2.setCreatedOn(TimeUtil.nowTs()); + assertThat(goodPs2.getCreatedOn()).isGreaterThan(goodPs1.getCreatedOn()); + + PatchSet badPs2 = clone(goodPs2); + badPs2.setCreatedOn(beforePs1); + assertThat(badPs2.getCreatedOn()).isLessThan(goodPs1.getCreatedOn()); + + // Both ReviewDb, exact match required. + ChangeBundle b1 = + new ChangeBundle( + c, + messages(), + patchSets(goodPs1, goodPs2), + approvals(), + comments(), + reviewers(), + REVIEW_DB); + ChangeBundle b2 = + new ChangeBundle( + c, + messages(), + patchSets(goodPs1, badPs2), + approvals(), + comments(), + reviewers(), + REVIEW_DB); + assertDiffs( + b1, + b2, + "createdOn differs for PatchSet.Id " + + badPs2.getId() + + ":" + + " {2009-09-30 17:00:18.0} != {2009-09-30 17:00:06.0}"); + + // Non-monotonic in ReviewDb but monotonic in NoteDb, timestamps are + // ignored, including for ps1. + PatchSet badPs1 = clone(goodPs1); + badPs1.setCreatedOn(TimeUtil.nowTs()); + b1 = + new ChangeBundle( + c, + messages(), + patchSets(badPs1, badPs2), + approvals(), + comments(), + reviewers(), + REVIEW_DB); + b2 = + new ChangeBundle( + c, + messages(), + patchSets(goodPs1, goodPs2), + approvals(), + comments(), + reviewers(), + NOTE_DB); + assertNoDiffs(b1, b2); + assertNoDiffs(b2, b1); + + // Non-monotonic in NoteDb but monotonic in ReviewDb, timestamps are not + // ignored. + b1 = + new ChangeBundle( + c, + messages(), + patchSets(goodPs1, goodPs2), + approvals(), + comments(), + reviewers(), + REVIEW_DB); + b2 = + new ChangeBundle( + c, + messages(), + patchSets(badPs1, badPs2), + approvals(), + comments(), + reviewers(), + NOTE_DB); + assertDiffs( + b1, + b2, + "createdOn differs for PatchSet.Id " + + badPs1.getId() + + " in NoteDb vs. ReviewDb:" + + " {2009-09-30 17:00:24.0} != {2009-09-30 17:00:12.0}", + "createdOn differs for PatchSet.Id " + + badPs2.getId() + + " in NoteDb vs. ReviewDb:" + + " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:18.0}"); + } + @Test public void diffPatchSetApprovalKeySets() throws Exception { Change c = TestChanges.newChange(project, accountId);