Expose the mergedOn date of a change in ChangeNotes
This is required for indexing the change on the merge date to implement
the merge search operator.
- Currently, this value can be extracted from
ChangeData.getSubmitApproval
- However, it should not be used as it is planned to deprecate SUBM
label from NoteDb, see LabelId.LEGACY_SUBMIT_NAME, see the change
Id7fbddfa.
- Also cache the mergedOn date as a part of ChangeNotesState.
Change-Id: I5b54fda392f1c6ca86d7f3440a227ded2aa40eb0
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index d48cbc4..b816264 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -64,6 +64,7 @@
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 @@
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);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 220e683..c554ca5 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -191,7 +191,8 @@
+ list(state.changeMessages(), changeMessage())
+ P
+ map(state.publishedComments().asMap(), comment())
- + I; // updateCount
+ + I // updateCount
+ + T; // mergedOn
}
private static int str(String s) {
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index fae29f8..6403b95 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -156,6 +156,7 @@
private Change.Id revertOf;
private int updateCount;
private PatchSet.Id cherryPickOf;
+ private Timestamp mergedOn;
ChangeNotesParser(
Change.Id changeId,
@@ -259,7 +260,8 @@
firstNonNull(hasReviewStarted, true),
revertOf,
cherryPickOf,
- updateCount);
+ updateCount,
+ mergedOn);
}
private Map<PatchSet.Id, PatchSet> buildPatchSets() throws ConfigInvalidException {
@@ -322,9 +324,9 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
}
}
+ /**
+ * 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();
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index 27cfb70..af8c8c8 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -136,7 +136,8 @@
boolean reviewStarted,
@Nullable Change.Id revertOf,
@Nullable PatchSet.Id cherryPickOf,
- int updateCount) {
+ int updateCount,
+ @Nullable Timestamp mergedOn) {
requireNonNull(
metaId,
() ->
@@ -184,6 +185,7 @@
.changeMessages(changeMessages)
.publishedComments(publishedComments)
.updateCount(updateCount)
+ .mergedOn(mergedOn)
.build();
}
@@ -329,6 +331,9 @@
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 @@
abstract Builder updateCount(int updateCount);
+ abstract Builder mergedOn(Timestamp mergedOn);
+
abstract ChangeNotesState build();
}
@@ -515,6 +522,10 @@
.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 @@
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();
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 432b48a..6de263e 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -307,7 +307,7 @@
private Integer unresolvedCommentCount;
private Integer totalCommentCount;
private LabelTypes labelTypes;
-
+ private Optional<Timestamp> mergedOn;
private ImmutableList<byte[]> refStates;
private ImmutableList<byte[]> refStatePatterns;
@@ -616,6 +616,29 @@
}
/**
+ * 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 @@
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();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 085d23d..2891d4b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1386,6 +1386,17 @@
}
}
+ @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();
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 321e4da..6c5bc2e 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -837,6 +837,7 @@
"publishedComments",
new TypeLiteral<ImmutableListMultimap<ObjectId, HumanComment>>() {}.getType())
.put("updateCount", int.class)
+ .put("mergedOn", Timestamp.class)
.build());
}
@@ -977,6 +978,19 @@
}
@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)
.hasAutoValueMethods(ImmutableMap.of("changeId", Change.Id.class, "uuid", String.class));
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index cc0b109..bae2f46 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -677,6 +677,74 @@
}
@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();
Ref initial = repo.exactRef(changeMetaRef(c.getId()));
diff --git a/proto/cache.proto b/proto/cache.proto
index aa71b87..31bb86f 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -76,7 +76,7 @@
// 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 @@
// 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
