Merge changes I3d933b87,I1a38cee8

* changes:
  ChangeBundle: Ignore lastUpdatedOn if bundle has no entities
  ChangeBundle: Allow created on timestamp to match first patch set

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

@@ -487,6 +487,25 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(nc.getOriginalSubject()).isEqualTo(orig);
   }
 
+  @Test
+  public void deleteDraftPS1WithNoOtherEntities() throws Exception {
+    PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
+    PushOneCommit.Result r = push.to("refs/drafts/master");
+    push = pushFactory.create(db, admin.getIdent(), testRepo,
+        PushOneCommit.SUBJECT, "b.txt", "4711", r.getChangeId());
+    r = push.to("refs/drafts/master");
+    PatchSet.Id psId = r.getPatchSetId();
+    Change.Id id = psId.getParentKey();
+
+    gApi.changes().id(r.getChangeId()).revision(1).delete();
+
+    checker.rebuildAndCheckChanges(id);
+
+    notesMigration.setAllEnabled(true);
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    assertThat(notes.getPatchSets().keySet()).containsExactly(psId);
+  }
+
   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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
@@ -28,6 +29,7 @@ import com.google.common.collect.ComparisonChain;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.LinkedListMultimap;
 import com.google.common.collect.Lists;
@@ -150,8 +152,8 @@ public class ChangeBundle {
   }
 
 
-  private static Map<PatchSet.Id, PatchSet> patchSetMap(Iterable<PatchSet> in) {
-    Map<PatchSet.Id, PatchSet> out = new TreeMap<>(
+  private static TreeMap<PatchSet.Id, PatchSet> patchSetMap(Iterable<PatchSet> in) {
+    TreeMap<PatchSet.Id, PatchSet> out = new TreeMap<>(
         new Comparator<PatchSet.Id>() {
           @Override
           public int compare(PatchSet.Id a, PatchSet.Id b) {
@@ -244,7 +246,7 @@ public class ChangeBundle {
 
   private final Change change;
   private final ImmutableList<ChangeMessage> changeMessages;
-  private final ImmutableMap<PatchSet.Id, PatchSet> patchSets;
+  private final ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private final ImmutableMap<PatchSetApproval.Key, PatchSetApproval>
       patchSetApprovals;
   private final ImmutableMap<PatchLineComment.Key, PatchLineComment>
@@ -260,7 +262,7 @@ public class ChangeBundle {
       Source source) {
     this.change = checkNotNull(change);
     this.changeMessages = changeMessageList(changeMessages);
-    this.patchSets = ImmutableMap.copyOf(patchSetMap(patchSets));
+    this.patchSets = ImmutableSortedMap.copyOfSorted(patchSetMap(patchSets));
     this.patchSetApprovals =
         ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));
     this.patchLineComments =
@@ -316,9 +318,16 @@ public class ChangeBundle {
     return ImmutableList.copyOf(diffs);
   }
 
+  private Timestamp getFirstPatchSetTime() {
+    if (patchSets.isEmpty()) {
+      return change.getCreatedOn();
+    }
+    return patchSets.firstEntry().getValue().getCreatedOn();
+  }
+
   private Timestamp getLatestTimestamp() {
-    Ordering<Timestamp> o = Ordering.natural();
-    Timestamp ts = change.getLastUpdatedOn();
+    Ordering<Timestamp> o = Ordering.natural().nullsFirst();
+    Timestamp ts = null;
     for (ChangeMessage cm : getChangeMessages()) {
       ts = o.max(ts, cm.getWrittenOn());
     }
@@ -334,7 +343,7 @@ public class ChangeBundle {
         ts = o.max(ts, plc.getWrittenOn());
       }
     }
-    return ts;
+    return firstNonNull(ts, change.getLastUpdatedOn());
   }
 
   private static void diffChanges(List<String> diffs, ChangeBundle bundleA,
@@ -343,13 +352,16 @@ public class ChangeBundle {
     Change b = bundleB.change;
     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();
 
-
+    // Allow created timestamp in NoteDb to be either the created timestamp of
+    // the change, or the timestamp of the first remaining patch set.
+    //
     // Ignore subject if the NoteDb subject starts with the ReviewDb subject.
     // The NoteDb subject is read directly from the commit, whereas the ReviewDb
     // subject historically may have been truncated to fit in a SQL varchar
@@ -376,11 +388,15 @@ public class ChangeBundle {
     //
     // 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());
       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());
       excludeOrigSubj = true;
       excludeTopic = a.getTopic() == null && "".equals(b.getTopic());
@@ -390,6 +406,9 @@ public class ChangeBundle {
     String updatedField = "lastUpdatedOn";
     List<String> exclude =
         Lists.newArrayList(updatedField, "noteDbState", "rowVersion");
+    if (excludeCreatedOn) {
+      exclude.add("createdOn");
+    }
     if (excludeSubject) {
       exclude.add("subject");
     }
@@ -402,9 +421,9 @@ public class ChangeBundle {
     diffColumnsExcluding(diffs, Change.class, desc, bundleA, a, bundleB, b,
         exclude);
 
-    // Allow timestamps to either be exactly equal (within slop), or the NoteDb
-    // timestamp to be equal to the latest entity timestamp in the whole
-    // ReviewDb bundle (within slop).
+    // Allow last updated timestamps to either be exactly equal (within slop),
+    // or the NoteDb timestamp to be equal to the latest entity timestamp in the
+    // whole ReviewDb bundle (within slop).
     if (timestampsDiffer(bundleA, a.getLastUpdatedOn(),
           bundleB, b.getLastUpdatedOn())) {
       diffTimestamps(diffs, desc, bundleA, aUpdated, bundleB, bUpdated,

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

@@ -293,6 +293,41 @@ public class ChangeBundleTest {
     assertNoDiffs(b1, b2);
   }
 
+  @Test
+  public void diffChangesIgnoresChangeTimestampIfAnyOtherEntitiesExist() {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    PatchSetApproval a = new PatchSetApproval(
+        new PatchSetApproval.Key(
+            c1.currentPatchSetId(), accountId, new LabelId("Code-Review")),
+        (short) 1,
+        TimeUtil.nowTs());
+    c1.setLastUpdatedOn(a.getGranted());
+
+    Change c2 = clone(c1);
+    c2.setLastUpdatedOn(TimeUtil.nowTs());
+
+    // ReviewDb has later lastUpdatedOn timestamp than NoteDb, allowed since
+    // NoteDb matches the latest timestamp of a non-Change entity.
+    ChangeBundle b1 = new ChangeBundle(c2, messages(), patchSets(),
+        approvals(a), comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c1, messages(), patchSets(),
+        approvals(a), comments(), NOTE_DB);
+    assertThat(b1.getChange().getLastUpdatedOn())
+        .isGreaterThan(b2.getChange().getLastUpdatedOn());
+    assertNoDiffs(b1, b2);
+
+    // Timestamps must actually match if Change is the only entity.
+    b1 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
+        REVIEW_DB);
+    b2 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
+        NOTE_DB);
+    assertDiffs(b1, b2,
+        "effective last updated time differs for Change.Id " + c1.getId()
+            + " in NoteDb vs. ReviewDb:"
+            + " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:12.0}");
+  }
+
   @Test
   public void diffChangesAllowsReviewDbSubjectToBePrefixOfNoteDbSubject()
       throws Exception {