Merge "Expose the mergedOn date of a change in ChangeNotes"

java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -64,6 +64,7 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -460,6 +461,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return state.updateCount();
   }
 
+  /** @return {@link Optional} value of time when the change was merged. */
+  public Optional<Timestamp> getMergedOn() {
+    return Optional.ofNullable(state.mergedOn());
+  }
+
   public ImmutableListMultimap<ObjectId, HumanComment> getDraftComments(Account.Id author) {
     return getDraftComments(author, null);
   }

java/com/google/gerrit/server/notedb/ChangeNotesCache.java

@@ -191,7 +191,8 @@ public class ChangeNotesCache {
           + list(state.changeMessages(), changeMessage())
           + P
           + map(state.publishedComments().asMap(), comment())
-          + I; // updateCount
+          + I // updateCount
+          + T; // mergedOn
     }
 
     private static int str(String s) {

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -156,6 +156,7 @@ class ChangeNotesParser {
   private Change.Id revertOf;
   private int updateCount;
   private PatchSet.Id cherryPickOf;
+  private Timestamp mergedOn;
 
   ChangeNotesParser(
       Change.Id changeId,
@@ -259,7 +260,8 @@ class ChangeNotesParser {
         firstNonNull(hasReviewStarted, true),
         revertOf,
         cherryPickOf,
-        updateCount);
+        updateCount,
+        mergedOn);
   }
 
   private Map<PatchSet.Id, PatchSet> buildPatchSets() throws ConfigInvalidException {
@@ -322,9 +324,9 @@ class ChangeNotesParser {
 
   private void parse(ChangeNotesCommit commit) throws ConfigInvalidException {
     updateCount++;
-    Timestamp ts = new Timestamp(commit.getCommitterIdent().getWhen().getTime());
+    Timestamp commitTimestamp = getCommitTimestamp(commit);
 
-    createdOn = ts;
+    createdOn = commitTimestamp;
     parseTag(commit);
 
     if (branch == null) {
@@ -364,21 +366,19 @@ class ChangeNotesParser {
       originalSubject = currSubject;
     }
 
-    parseChangeMessage(psId, accountId, realAccountId, commit, ts);
+    parseChangeMessage(psId, accountId, realAccountId, commit, commitTimestamp);
     if (topic == null) {
       topic = parseTopic(commit);
     }
 
     parseHashtags(commit);
     parseAttentionSetUpdates(commit);
-    parseAssigneeUpdates(ts, commit);
+    parseAssigneeUpdates(commitTimestamp, commit);
 
-    if (submissionId == null) {
-      submissionId = parseSubmissionId(commit);
-    }
+    parseSubmission(commit, commitTimestamp);
 
-    if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
-      lastUpdatedOn = ts;
+    if (lastUpdatedOn == null || commitTimestamp.after(lastUpdatedOn)) {
+      lastUpdatedOn = commitTimestamp;
     }
 
     if (deletedPatchSets.contains(psId)) {
@@ -392,16 +392,10 @@ class ChangeNotesParser {
 
     ObjectId currRev = parseRevision(commit);
     if (currRev != null) {
-      parsePatchSet(psId, currRev, accountId, ts);
+      parsePatchSet(psId, currRev, accountId, commitTimestamp);
     }
     parseCurrentPatchSet(psId, commit);
 
-    if (submitRecords.isEmpty()) {
-      // Only parse the most recent set of submit records; any older ones are
-      // still there, but not currently used.
-      parseSubmitRecords(commit.getFooterLineValues(FOOTER_SUBMITTED_WITH));
-    }
-
     if (status == null) {
       status = parseStatus(commit);
     }
@@ -409,15 +403,15 @@ class ChangeNotesParser {
     // Parse approvals after status to treat approvals in the same commit as
     // "Status: merged" as non-post-submit.
     for (String line : commit.getFooterLineValues(FOOTER_LABEL)) {
-      parseApproval(psId, accountId, realAccountId, ts, line);
+      parseApproval(psId, accountId, realAccountId, commitTimestamp, line);
     }
 
     for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
       for (String line : commit.getFooterLineValues(state.getFooterKey())) {
-        parseReviewer(ts, state, line);
+        parseReviewer(commitTimestamp, state, line);
       }
       for (String line : commit.getFooterLineValues(state.getByEmailFooterKey())) {
-        parseReviewerByEmail(ts, state, line);
+        parseReviewerByEmail(commitTimestamp, state, line);
       }
       // Don't update timestamp when a reviewer was added, matching RevewDb
       // behavior.
@@ -439,6 +433,24 @@ class ChangeNotesParser {
     parseWorkInProgress(commit);
   }
 
+  private void parseSubmission(ChangeNotesCommit commit, Timestamp commitTimestamp)
+      throws ConfigInvalidException {
+    // Only parse the most recent sumbit commit (there should be exactly one).
+    if (submissionId == null) {
+      submissionId = parseSubmissionId(commit);
+    }
+
+    if (submissionId != null && mergedOn == null) {
+      mergedOn = commitTimestamp;
+    }
+
+    if (submitRecords.isEmpty()) {
+      // Only parse the most recent set of submit records; any older ones are
+      // still there, but not currently used.
+      parseSubmitRecords(commit.getFooterLineValues(FOOTER_SUBMITTED_WITH));
+    }
+  }
+
   private String parseSubmissionId(ChangeNotesCommit commit) throws ConfigInvalidException {
     return parseOneFooter(commit, FOOTER_SUBMISSION_ID);
   }
@@ -1016,6 +1028,23 @@ class ChangeNotesParser {
     }
   }
 
+  /**
+   * Returns the {@link Timestamp} when the commit was applied.
+   *
+   * <p>The author's date only notes when the commit was originally made. Thus, use the commiter's
+   * date as it accounts for the rebase, cherry-pick, commit --amend and other commands that rewrite
+   * the history of the branch.
+   *
+   * <p>Don't use {@link org.eclipse.jgit.revwalk.RevCommit#getCommitTime} directly because it
+   * returns int and would overflow.
+   *
+   * @param commit the commit to return commit time.
+   * @return the timestamp when the commit was applied.
+   */
+  private Timestamp getCommitTimestamp(ChangeNotesCommit commit) {
+    return new Timestamp(commit.getCommitterIdent().getWhen().getTime());
+  }
+
   private void pruneReviewers() {
     Iterator<Table.Cell<Account.Id, ReviewerStateInternal, Timestamp>> rit =
         reviewers.cellSet().iterator();

java/com/google/gerrit/server/notedb/ChangeNotesState.java

@@ -136,7 +136,8 @@ public abstract class ChangeNotesState {
       boolean reviewStarted,
       @Nullable Change.Id revertOf,
       @Nullable PatchSet.Id cherryPickOf,
-      int updateCount) {
+      int updateCount,
+      @Nullable Timestamp mergedOn) {
     requireNonNull(
         metaId,
         () ->
@@ -184,6 +185,7 @@ public abstract class ChangeNotesState {
         .changeMessages(changeMessages)
         .publishedComments(publishedComments)
         .updateCount(updateCount)
+        .mergedOn(mergedOn)
         .build();
   }
 
@@ -329,6 +331,9 @@ public abstract class ChangeNotesState {
 
   abstract int updateCount();
 
+  @Nullable
+  abstract Timestamp mergedOn();
+
   Change newChange(Project.NameKey project) {
     ChangeColumns c = requireNonNull(columns(), "columns are required");
     Change change =
@@ -445,6 +450,8 @@ public abstract class ChangeNotesState {
 
     abstract Builder updateCount(int updateCount);
 
+    abstract Builder mergedOn(Timestamp mergedOn);
+
     abstract ChangeNotesState build();
   }
 
@@ -515,6 +522,10 @@ public abstract class ChangeNotesState {
           .forEach(m -> b.addChangeMessage(toByteString(m, ChangeMessageProtoConverter.INSTANCE)));
       object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c)));
       b.setUpdateCount(object.updateCount());
+      if (object.mergedOn() != null) {
+        b.setMergedOnMillis(object.mergedOn().getTime());
+        b.setHasMergedOn(true);
+      }
 
       return Protos.toByteArray(b.build());
     }
@@ -655,7 +666,8 @@ public abstract class ChangeNotesState {
                   proto.getPublishedCommentList().stream()
                       .map(r -> GSON.fromJson(r, HumanComment.class))
                       .collect(toImmutableListMultimap(HumanComment::getCommitId, c -> c)))
-              .updateCount(proto.getUpdateCount());
+              .updateCount(proto.getUpdateCount())
+              .mergedOn(proto.getHasMergedOn() ? new Timestamp(proto.getMergedOnMillis()) : null);
       return b.build();
     }
 

java/com/google/gerrit/server/query/change/ChangeData.java

@@ -307,7 +307,7 @@ public class ChangeData {
   private Integer unresolvedCommentCount;
   private Integer totalCommentCount;
   private LabelTypes labelTypes;
-
+  private Optional<Timestamp> mergedOn;
   private ImmutableList<byte[]> refStates;
   private ImmutableList<byte[]> refStatePatterns;
 
@@ -615,6 +615,29 @@ public class ChangeData {
     return attentionSet;
   }
 
+  /**
+   * Returns the {@link Optional} value of time when the change was merged.
+   *
+   * <p>The value can be set from index field, see {@link ChangeData#setMergedOn} or loaded from the
+   * database (available in {@link ChangeNotes})
+   *
+   * @return {@link Optional} value of time when the change was merged.
+   * @throws StorageException if {@code lazyLoad} is off, {@link ChangeNotes} can not be loaded
+   *     because we do not expect to call the database.
+   */
+  public Optional<Timestamp> getMergedOn() throws StorageException {
+    if (mergedOn == null) {
+      // The value was not loaded yet, try to get from the database.
+      mergedOn = notes().getMergedOn();
+    }
+    return mergedOn;
+  }
+
+  /** Sets the value e.g. when loading from index. */
+  public void setMergedOn(@Nullable Timestamp mergedOn) {
+    this.mergedOn = Optional.ofNullable(mergedOn);
+  }
+
   /**
    * Sets the specified attention set. If two or more entries refer to the same user, throws an
    * {@link IllegalStateException}.
@@ -670,7 +693,9 @@ public class ChangeData {
     return allApprovals;
   }
 
-  /** @return The submit ('SUBM') approval label */
+  /* @return legacy submit ('SUBM') approval label */
+  // TODO(mariasavtchouk): Deprecate legacy submit label,
+  // see com.google.gerrit.entities.LabelId.LEGACY_SUBMIT_NAME
   public Optional<PatchSetApproval> getSubmitApproval() {
     return currentApprovals().stream().filter(PatchSetApproval::isLegacySubmit).findFirst();
   }

javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java

@@ -1386,6 +1386,17 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void submitSetsMergedOn() throws Throwable {
+    PushOneCommit.Result r = createChange();
+    assertThat(r.getChange().getMergedOn()).isEmpty();
+    submit(r.getChangeId());
+    assertThat(r.getChange().getMergedOn()).isPresent();
+    ChangeInfo change = gApi.changes().id(r.getChangeId()).get();
+    assertThat(r.getChange().getMergedOn().get()).isEqualTo(change.updated);
+    assertThat(r.getChange().getMergedOn().get()).isEqualTo(change.submitted);
+  }
+
   @Override
   protected void updateProjectInput(ProjectInput in) {
     in.submitType = getSubmitType();

javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java

@@ -837,6 +837,7 @@ public class ChangeNotesStateTest {
                     "publishedComments",
                     new TypeLiteral<ImmutableListMultimap<ObjectId, HumanComment>>() {}.getType())
                 .put("updateCount", int.class)
+                .put("mergedOn", Timestamp.class)
                 .build());
   }
 
@@ -976,6 +977,19 @@ public class ChangeNotesStateTest {
                 "type", String.class));
   }
 
+  @Test
+  public void serializeMergedOn() throws Exception {
+    assertRoundTrip(
+        newBuilder().mergedOn(new Timestamp(234567L)).build(),
+        ChangeNotesStateProto.newBuilder()
+            .setMetaId(SHA_BYTES)
+            .setChangeId(ID.get())
+            .setMergedOnMillis(234567L)
+            .setHasMergedOn(true)
+            .setColumns(colsProto.toBuilder())
+            .build());
+  }
+
   @Test
   public void changeMessageFields() throws Exception {
     assertThatSerializedClass(ChangeMessage.Key.class)

javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -676,6 +676,74 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getChange().getSubmissionId()).isEqualTo(submissionId.toString());
   }
 
+  @Test
+  public void mergedOnEmptyIfNotSubmitted() throws Exception {
+    Change c = newChange();
+
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    // Make sure unrelevent update does not set mergedOn.
+    update.setTopic("topic");
+    update.commit();
+    assertThat(newNotes(c).getMergedOn()).isEmpty();
+  }
+
+  @Test
+  public void mergedOnSetWhenSubmitted() throws Exception {
+    Change c = newChange();
+
+    SubmissionId submissionId = new SubmissionId(c);
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setSubjectForCommit("Update patch set 1");
+    update.merge(
+        submissionId,
+        ImmutableList.of(
+            submitRecord("OK", null, submitLabel("Code-Review", "OK", otherUser.getAccountId()))));
+    update.commit();
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getMergedOn()).isPresent();
+    Timestamp mergedOn = notes.getMergedOn().get();
+    assertThat(mergedOn).isEqualTo(notes.getChange().getLastUpdatedOn());
+
+    // Next update does not change mergedOn date.
+    update = newUpdate(c, changeOwner);
+    update.putApproval("Code-Review", (short) 1);
+    update.commit();
+    notes = newNotes(c);
+    assertThat(notes.getMergedOn().get()).isEqualTo(mergedOn);
+    assertThat(notes.getMergedOn().get()).isLessThan(notes.getChange().getLastUpdatedOn());
+  }
+
+  @Test
+  public void latestMergedOn() throws Exception {
+    Change c = newChange();
+    SubmissionId submissionId = new SubmissionId(c);
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setSubjectForCommit("Update patch set 1");
+    update.merge(
+        submissionId,
+        ImmutableList.of(
+            submitRecord("OK", null, submitLabel("Code-Review", "OK", otherUser.getAccountId()))));
+    update.commit();
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getMergedOn()).isPresent();
+    Timestamp mergedOn = notes.getMergedOn().get();
+    assertThat(mergedOn).isEqualTo(notes.getChange().getLastUpdatedOn());
+
+    incrementPatchSet(c);
+    update = newUpdate(c, changeOwner);
+    update.setSubjectForCommit("Update patch set 2");
+    update.merge(
+        submissionId,
+        ImmutableList.of(
+            submitRecord(
+                "OK", null, submitLabel("Code-Review", "OK", changeOwner.getAccountId()))));
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(notes.getMergedOn().get()).isGreaterThan(mergedOn);
+    assertThat(notes.getMergedOn().get()).isEqualTo(notes.getChange().getLastUpdatedOn());
+  }
+
   @Test
   public void emptyChangeUpdate() throws Exception {
     Change c = newChange();

proto/cache.proto

@@ -76,7 +76,7 @@ message ChangeNotesKeyProto {
 // Instead, we just take the tedious yet simple approach of having a "has_foo"
 // field for each nullable field "foo", indicating whether or not foo is null.
 //
-// Next ID: 25
+// Next ID: 27
 message ChangeNotesStateProto {
   // Effectively required, even though the corresponding ChangeNotesState field
   // is optional, since the field is only absent when NoteDb is disabled, in
@@ -223,6 +223,10 @@ message ChangeNotesStateProto {
 
   // Includes all attention set updates.
   repeated AttentionSetUpdateProto all_attention_set_update = 24;
+
+  // Epoch millis.
+  int64 merged_on_millis = 25;
+  bool has_merged_on = 26;
 }
 
 // Serialized form of com.google.gerrit.server.query.change.ConflictKey