diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 8893ecb940..780d7a84d1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -17,8 +17,11 @@ package com.google.gerrit.acceptance.git; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.cloneProject; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; @@ -41,12 +44,18 @@ import com.jcraft.jsch.JSchException; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.transport.PushResult; +import org.joda.time.DateTime; +import org.joda.time.DateTimeUtils; +import org.joda.time.DateTimeUtils.MillisProvider; +import org.junit.AfterClass; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; import java.io.IOException; import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; public abstract class AbstractPushForReview extends AbstractDaemonTest { @ConfigSuite.Config @@ -63,6 +72,24 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { private String sshUrl; + @BeforeClass + public static void setTimeForTesting() { + final long clockStepMs = MILLISECONDS.convert(1, SECONDS); + final AtomicLong clockMs = new AtomicLong( + new DateTime(2009, 9, 30, 17, 0, 0).getMillis()); + DateTimeUtils.setCurrentMillisProvider(new MillisProvider() { + @Override + public long getMillis() { + return clockMs.getAndAdd(clockStepMs); + } + }); + } + + @AfterClass + public static void restoreTime() { + DateTimeUtils.setCurrentMillisSystem(); + } + @Before public void setUp() throws Exception { sshUrl = sshSession.getUrl(); @@ -194,6 +221,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(cr.all).hasSize(1); assertThat(cr.all.get(0).name).isEqualTo("Administrator"); assertThat(cr.all.get(0).value.intValue()).is(1); + assertThat(Iterables.getLast(ci.messages).message).isEqualTo( + "Uploaded patch set 1: Code-Review+1."); PushOneCommit push = pushFactory.create(db, admin.getIdent(), PushOneCommit.SUBJECT, @@ -202,9 +231,20 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { ci = get(r.getChangeId()); cr = ci.labels.get("Code-Review"); + assertThat(Iterables.getLast(ci.messages).message).isEqualTo( + "Uploaded patch set 2: Code-Review+2."); + assertThat(cr.all).hasSize(1); assertThat(cr.all.get(0).name).isEqualTo("Administrator"); assertThat(cr.all.get(0).value.intValue()).is(2); + + push = + pushFactory.create(db, admin.getIdent(), PushOneCommit.SUBJECT, + "c.txt", "moreContent", r.getChangeId()); + r = push.to(git, "refs/for/master/%l=Code-Review+2"); + ci = get(r.getChangeId()); + assertThat(Iterables.getLast(ci.messages).message).isEqualTo( + "Uploaded patch set 3."); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK index f36c4479f0..73183d76ba 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/BUCK @@ -19,5 +19,8 @@ acceptance_tests( java_library( name = 'push_for_review', srcs = ['AbstractPushForReview.java'], - deps = ['//gerrit-acceptance-tests:lib'], + deps = [ + '//gerrit-acceptance-tests:lib', + '//lib/joda:joda-time', + ], ) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 16b316fb96..1184a94536 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -68,6 +68,7 @@ import com.google.gerrit.reviewdb.client.Branch; 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.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; @@ -1716,7 +1717,9 @@ public class ReceiveCommits { ChangeMessage msg = new ChangeMessage(new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)), me, ps.getCreatedOn(), ps.getId()); - msg.setMessage("Uploaded patch set " + ps.getPatchSetId() + "."); + StringBuilder msgs = renderMessageWithApprovals(ps.getPatchSetId(), + approvals, Collections.emptyMap()); + msg.setMessage(msgs.toString() + "."); ins .setReviewers(recipients.getReviewers()) @@ -1842,6 +1845,27 @@ public class ReceiveCommits { } } + private StringBuilder renderMessageWithApprovals(int patchSetId, + Map n, Map c) { + StringBuilder msgs = new StringBuilder("Uploaded patch set " + patchSetId); + if (!n.isEmpty()) { + boolean first = true; + for (Map.Entry e : n.entrySet()) { + if (c.containsKey(e.getKey()) + && c.get(e.getKey()).getValue() == e.getValue()) { + continue; + } + if (first) { + msgs.append(":"); + first = false; + } + msgs.append(" ") + .append(LabelVote.create(e.getKey(), e.getValue()).format()); + } + } + return msgs; + } + private class ReplaceRequest { final Change.Id ontoChange; final RevCommit newCommit; @@ -2075,29 +2099,52 @@ public class ReceiveCommits { return Futures.makeChecked(future, INSERT_EXCEPTION); } - private ChangeMessage newChangeMessage(ReviewDb db, ChangeKind changeKind) + private ChangeMessage newChangeMessage(ReviewDb db, ChangeKind changeKind, + Map approvals) throws OrmException { msg = new ChangeMessage(new ChangeMessage.Key(change.getId(), ChangeUtil .messageUUID(db)), currentUser.getAccountId(), newPatchSet.getCreatedOn(), newPatchSet.getId()); - String message = "Uploaded patch set " + newPatchSet.getPatchSetId(); + StringBuilder msgs = renderMessageWithApprovals( + newPatchSet.getPatchSetId(), approvals, scanLabels(db, approvals)); switch (changeKind) { case TRIVIAL_REBASE: case NO_CHANGE: - message += ": Patch Set " + priorPatchSet.get() + " was rebased"; + msgs.append(": Patch Set " + priorPatchSet.get() + " was rebased"); break; case NO_CODE_CHANGE: - message += ": Commit message was updated"; + msgs.append(": Commit message was updated"); break; case REWORK: default: break; } - msg.setMessage(message + "."); + msg.setMessage(msgs.toString() + "."); return msg; } + private Map scanLabels(ReviewDb db, + Map approvals) + throws OrmException { + Map current = new HashMap<>(); + // We optimize here and only retrieve current when approvals provided + if (!approvals.isEmpty()) { + for (PatchSetApproval a : approvalsUtil.byPatchSetUser( + db, changeCtl, priorPatchSet, currentUser.getAccountId())) { + if (a.isSubmit()) { + continue; + } + + LabelType lt = labelTypes.byLabel(a.getLabelId()); + if (lt != null) { + current.put(lt.getName(), a); + } + } + } + return current; + } + PatchSet.Id upsertEdit() { if (cmd.getResult() == NOT_ATTEMPTED) { cmd.execute(rp); @@ -2158,7 +2205,8 @@ public class ReceiveCommits { changeKind = changeKindCache.getChangeKind( projectControl.getProjectState(), repo, priorCommit, newCommit); - cmUtil.addChangeMessage(db, update, newChangeMessage(db, changeKind)); + cmUtil.addChangeMessage(db, update, newChangeMessage(db, changeKind, + approvals)); if (mergedIntoRef == null) { // Change should be new, so it can go through review again.