Merge changes I8978e906,I83d36056,Ib97b137c,I45b16437

* changes:
  ChangeRebuilder: Add FinalUpdatesEvent before sorting
  StatusChangeEvent: Support MERGED
  ChangeRebuilder Factor out event for status changes
  ChangeRebuilder: Add dependencies on PatchSetEvent

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

@@ -715,6 +715,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     rin.message = "comment";
 
     Timestamp ts = new Timestamp(c.getCreatedOn().getTime() + 2000);
+    assertThat(ts).isGreaterThan(c.getCreatedOn());
+    assertThat(ts).isLessThan(db.patchSets().get(psId).getCreatedOn());
     RevisionResource revRsrc = parseCurrentRevisionResource(r.getChangeId());
     postReview.get().apply(revRsrc, rin, ts);
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -624,17 +624,31 @@ public class ChangeBundle {
     List<String> tempDiffs = new ArrayList<>();
     String temp = "temp";
 
+    // ReviewDb allows timestamps before patch set was created, but NoteDb
+    // truncates this to the patch set creation timestamp.
+    Timestamp ta = a.getWrittenOn();
+    Timestamp tb = b.getWrittenOn();
+    PatchSet psa = bundleA.patchSets.get(a.getPatchSetId());
+    PatchSet psb = bundleB.patchSets.get(b.getPatchSetId());
     boolean excludePatchSet = false;
+    boolean excludeWrittenOn = false;
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludePatchSet = a.getPatchSetId() == null;
+      excludeWrittenOn = psa != null && psb != null
+          && ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn());
     } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
       excludePatchSet = b.getPatchSetId() == null;
+      excludeWrittenOn = psa != null && psb != null
+          && tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn());
     }
 
     List<String> exclude = Lists.newArrayList("key");
     if (excludePatchSet) {
       exclude.add("patchset");
     }
+    if (excludeWrittenOn) {
+      exclude.add("writtenOn");
+    }
 
     diffColumnsExcluding(
         tempDiffs, ChangeMessage.class, temp, bundleA, a, bundleB, b, exclude);
@@ -673,7 +687,28 @@ public class ChangeBundle {
       PatchSetApproval a = as.get(k);
       PatchSetApproval b = bs.get(k);
       String desc = describe(k);
-      diffColumns(diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b);
+
+      // ReviewDb allows timestamps before patch set was created, but NoteDb
+      // truncates this to the patch set creation timestamp.
+      Timestamp ta = a.getGranted();
+      Timestamp tb = b.getGranted();
+      PatchSet psa = checkNotNull(bundleA.patchSets.get(a.getPatchSetId()));
+      PatchSet psb = checkNotNull(bundleB.patchSets.get(b.getPatchSetId()));
+      boolean excludeGranted = false;
+      List<String> exclude = new ArrayList<>(1);
+      if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
+        excludeGranted =
+            ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn());
+      } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
+        excludeGranted =
+            tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn());
+      }
+      if (excludeGranted) {
+        exclude.add("granted");
+      }
+
+      diffColumnsExcluding(
+          diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b, exclude);
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeMessageEvent.java

@@ -31,11 +31,6 @@ class ChangeMessageEvent extends Event {
   private static final Pattern TOPIC_REMOVED_REGEXP =
       Pattern.compile("^Topic (.+) removed$");
 
-  private static final Pattern STATUS_ABANDONED_REGEXP =
-      Pattern.compile("^Abandoned(\n.*)*$");
-  private static final Pattern STATUS_RESTORED_REGEXP =
-      Pattern.compile("^Restored(\n.*)*$");
-
   private final ChangeMessage message;
   private final Change noteDbChange;
 
@@ -57,7 +52,6 @@ class ChangeMessageEvent extends Event {
     checkUpdate(update);
     update.setChangeMessage(message.getMessage());
     setTopic(update);
-    setStatus(update);
   }
 
   private void setTopic(ChangeUpdate update) {
@@ -86,21 +80,4 @@ class ChangeMessageEvent extends Event {
       noteDbChange.setTopic(null);
     }
   }
-
-  private void setStatus(ChangeUpdate update) {
-    String msg = message.getMessage();
-    if (msg == null) {
-      return;
-    }
-    if (STATUS_ABANDONED_REGEXP.matcher(msg).matches()) {
-      update.setStatus(Change.Status.ABANDONED);
-      noteDbChange.setStatus(Change.Status.ABANDONED);
-      return;
-    }
-
-    if (STATUS_RESTORED_REGEXP.matcher(msg).matches()) {
-      update.setStatus(Change.Status.NEW);
-      noteDbChange.setStatus(Change.Status.NEW);
-    }
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java

@@ -28,6 +28,7 @@ import com.google.common.base.Splitter;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
@@ -91,6 +92,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 
@@ -308,8 +310,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     deleteDraftRefs(change, manager.getAllUsersRepo());
 
     Integer minPsNum = getMinPatchSetNum(bundle);
-    Set<PatchSet.Id> psIds =
+    Map<PatchSet.Id, PatchSetEvent> patchSetEvents =
-        Sets.newHashSetWithExpectedSize(bundle.getPatchSets().size());
+        Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size());
 
     for (PatchSet ps : bundle.getPatchSets()) {
       if (ps.getId().get() > currPsId.get()) {
@@ -318,14 +320,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
             ps.getId(), currPsId);
         continue;
       }
-      psIds.add(ps.getId());
+      PatchSetEvent pse =
-      events.add(new PatchSetEvent(
+          new PatchSetEvent(change, ps, manager.getChangeRepo().rw);
-          change, ps, manager.getChangeRepo().rw));
+      patchSetEvents.put(ps.getId(), pse);
+      events.add(pse);
       for (PatchLineComment c : getPatchLineComments(bundle, ps)) {
         PatchLineCommentEvent e =
             new PatchLineCommentEvent(c, change, ps, patchListCache);
         if (c.getStatus() == Status.PUBLISHED) {
-          events.add(e);
+          events.add(e.addDep(pse));
         } else {
           draftCommentEvents.put(c.getAuthor(), e);
         }
@@ -333,8 +336,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
 
     for (PatchSetApproval psa : bundle.getPatchSetApprovals()) {
-      if (psIds.contains(psa.getPatchSetId())) {
+      PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId());
-        events.add(new ApprovalEvent(psa, change.getCreatedOn()));
+      if (pse != null) {
+        events.add(new ApprovalEvent(psa, change.getCreatedOn()).addDep(pse));
       }
     }
 
@@ -345,10 +349,16 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
 
     Change noteDbChange = new Change(null, null, null, null, null);
     for (ChangeMessage msg : bundle.getChangeMessages()) {
-      if (msg.getPatchSetId() == null || psIds.contains(msg.getPatchSetId())) {
+      List<Event> msgEvents = parseChangeMessage(msg, change, noteDbChange);
-        events.add(
+      if (msg.getPatchSetId() != null) {
-            new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
+        PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
+        if (pse != null) {
+          for (Event e : msgEvents) {
+            e.addDep(pse);
+          }
+        }
       }
+      events.addAll(msgEvents);
     }
 
     sortAndFillEvents(change, noteDbChange, events, minPsNum);
@@ -377,6 +387,18 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
   }
 
+  private List<Event> parseChangeMessage(ChangeMessage msg, Change change,
+      Change noteDbChange) {
+    List<Event> events = new ArrayList<>(2);
+    events.add(new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()));
+    Optional<StatusChangeEvent> sce =
+        StatusChangeEvent.parseFromMessage(msg, change, noteDbChange);
+    if (sce.isPresent()) {
+      events.add(sce.get());
+    }
+    return events;
+  }
+
   private static Integer getMinPatchSetNum(ChangeBundle bundle) {
     Integer minPsNum = null;
     for (PatchSet ps : bundle.getPatchSets()) {
@@ -398,8 +420,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
 
   private void sortAndFillEvents(Change change, Change noteDbChange,
       List<Event> events, Integer minPsNum) {
-    new EventSorter(events).sort();
     events.add(new FinalUpdatesEvent(change, noteDbChange));
+    new EventSorter(events).sort();
 
     // Ensure the first event in the list creates the change, setting the author
     // and any required footers.

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java

@@ -66,8 +66,9 @@ abstract class Event implements Comparable<Event> {
         who, update.getNullableAccountId());
   }
 
-  void addDep(Event e) {
+  Event addDep(Event e) {
     deps.add(e);
+    return this;
   }
 
   /**
@@ -78,10 +79,6 @@ abstract class Event implements Comparable<Event> {
 
   abstract void apply(ChangeUpdate update) throws OrmException, IOException;
 
-  protected boolean isPatchSet() {
-    return false;
-  }
-
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(this)
@@ -94,6 +91,7 @@ abstract class Event implements Comparable<Event> {
   @Override
   public int compareTo(Event other) {
     return ComparisonChain.start()
+        .compareFalseFirst(this.isFinalUpdates(), other.isFinalUpdates())
         .compare(this.when, other.when)
         .compareTrueFirst(isPatchSet(), isPatchSet())
         .compareTrueFirst(this.predatesChange, other.predatesChange)
@@ -102,4 +100,12 @@ abstract class Event implements Comparable<Event> {
             ReviewDbUtil.intKeyOrdering().nullsLast())
         .result();
   }
+
+  private boolean isPatchSet() {
+    return this instanceof PatchSetEvent;
+  }
+
+  private boolean isFinalUpdates() {
+    return this instanceof FinalUpdatesEvent;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java

@@ -46,7 +46,8 @@ class FinalUpdatesEvent extends Event {
       // TODO(dborowitz): Stamp approximate approvals at this time.
       update.fixStatus(change.getStatus());
     }
-    if (change.getSubmissionId() != null) {
+    if (change.getSubmissionId() != null
+        && noteDbChange.getSubmissionId() == null) {
       update.setSubmissionId(change.getSubmissionId());
     }
     if (!update.isEmpty()) {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/PatchSetEvent.java

@@ -66,11 +66,6 @@ class PatchSetEvent extends Event {
     }
   }
 
-  @Override
-  protected boolean isPatchSet() {
-    return true;
-  }
-
   private void setRevision(ChangeUpdate update, PatchSet ps)
       throws IOException {
     String rev = ps.getRevision().get();

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java

@@ -0,0 +1,90 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb.rebuild;
+
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gwtorm.server.OrmException;
+
+import java.sql.Timestamp;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+class StatusChangeEvent extends Event {
+  private static final ImmutableMap<Change.Status, Pattern> PATTERNS =
+      ImmutableMap.of(
+          Change.Status.ABANDONED, Pattern.compile("^Abandoned(\n.*)*$"),
+          Change.Status.MERGED, Pattern.compile(
+              "^Change has been successfully"
+              + " (merged|cherry-picked|rebased|pushed).*$"),
+          Change.Status.NEW, Pattern.compile("^Restored(\n.*)*$"));
+
+  static Optional<StatusChangeEvent> parseFromMessage(ChangeMessage message,
+      Change change, Change noteDbChange) {
+    String msg = message.getMessage();
+    if (msg == null) {
+      return Optional.absent();
+    }
+    for (Map.Entry<Change.Status, Pattern> e : PATTERNS.entrySet()) {
+      if (e.getValue().matcher(msg).matches()) {
+        return Optional.of(new StatusChangeEvent(
+            message, change, noteDbChange, e.getKey()));
+      }
+    }
+    return Optional.absent();
+  }
+
+  private final Change change;
+  private final Change noteDbChange;
+  private final Change.Status status;
+
+  private StatusChangeEvent(ChangeMessage message, Change change,
+      Change noteDbChange, Change.Status status) {
+    this(message.getPatchSetId(), message.getAuthor(),
+        message.getWrittenOn(), change, noteDbChange, message.getTag(),
+        status);
+  }
+
+  private StatusChangeEvent(PatchSet.Id psId, Account.Id author,
+      Timestamp when, Change change, Change noteDbChange,
+      String tag, Change.Status status) {
+    super(psId, author, when, change.getCreatedOn(), tag);
+    this.change = change;
+    this.noteDbChange = noteDbChange;
+    this.status = status;
+  }
+
+  @Override
+  boolean uniquePerUpdate() {
+    return true;
+  }
+
+  @SuppressWarnings("deprecation")
+  @Override
+  void apply(ChangeUpdate update) throws OrmException {
+    checkUpdate(update);
+    update.fixStatus(status);
+    noteDbChange.setStatus(status);
+    if (status == Change.Status.MERGED) {
+      update.setSubmissionId(change.getSubmissionId());
+      noteDbChange.setSubmissionId(change.getSubmissionId());
+    }
+  }
+}

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

@@ -1260,7 +1260,9 @@ public class ChangeBundleTest {
   }
 
   private static List<PatchSet> latest(Change c) {
-    return ImmutableList.of(new PatchSet(c.currentPatchSetId()));
+    PatchSet ps = new PatchSet(c.currentPatchSetId());
+    ps.setCreatedOn(c.getLastUpdatedOn());
+    return ImmutableList.of(ps);
   }
 
   private static List<PatchSetApproval> approvals(PatchSetApproval... ents) {