ReceiveCommits: Include approvals from magic branch in change message

I52a6799395 extended magic branch options with approvals, but missed
to include the approvals in the change message.

Render the approvals for new change and for new patch set.

* Uploaded patch set 1: Code-Review-1 Verified+1.
* Uploaded patch set 2: Code-Review+1 Verified-1: Commit message was
  updated.

Label votes are only included in the change message, when they differ
from the current votes from the same user. This is the same logic as
implemented in PostReview.

Unit test is extended to verify that the approvals included in change
message. To prevent the test to be flaky and make chronological sorting
order stable (createdOn attribute of message) clock step is bumped to 1
second in AbstractPushForReview.

Reported-By: Khai Do <zaro0508@gmail.com>
Change-Id: I60a5f1447f084dfd3967ee335652df4a75354a71

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

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',
+  ],
 )

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.<String, PatchSetApproval>emptyMap());
+      msg.setMessage(msgs.toString() + ".");
 
       ins
         .setReviewers(recipients.getReviewers())
@@ -1842,6 +1845,27 @@ public class ReceiveCommits {
     }
   }
 
+  private StringBuilder renderMessageWithApprovals(int patchSetId,
+      Map<String, Short> n, Map<String, PatchSetApproval> c) {
+    StringBuilder msgs = new StringBuilder("Uploaded patch set " + patchSetId);
+    if (!n.isEmpty()) {
+      boolean first = true;
+      for (Map.Entry<String, Short> 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<String, Short> 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<String, PatchSetApproval> scanLabels(ReviewDb db,
+        Map<String, Short> approvals)
+        throws OrmException {
+      Map<String, PatchSetApproval> 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.