ChangeJson: Add option to include review footers

We have a user who likes the feature of the cherry-pick submit type
but doesn't want to deal with the hassle of changes possibly going
into the SUBMITTED state. They plan to convert their automation to
push directly to branches, bypassing the submit handler entirely.
However, they still want to have review information in footers. Rather
than having them reimplement this piece of Gerrit logic on the client
side, add an option to include review footer info in change detail.

This is inherently racy, since reviews can happen between the most
recent fetch and the push to the branch with the new commit message,
but such races already exist in the submit codepath, as submit rules
are not evaluated in the same transaction as the merge attempt.

Change-Id: If95c6a1e1625fe5eff3300a3e9220643b92d08a2

Documentation/rest-api-changes.txt

@@ -310,6 +310,12 @@ default. Optional fields are:
 * `CHECK`: include potential problems with the change.
 --
 
+[[commit-footers]]
+--
+* `COMMIT_FOOTERS`: include the full commit message with
+  Gerrit-specific commit footers in the
+  link:#revision-info[RevisionInfo].
+
 .Request
 ----
   GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES&o=DOWNLOAD_COMMANDS HTTP/1.0
@@ -4108,6 +4114,12 @@ entities.
 |`reviewed`     |optional|
 Indicates whether the caller is authenticated and has commented on the
 current revision. Only set if link:#reviewed[REVIEWED] option is requested.
+|`messageWithFooter` |optional|
+If the link:#commit-footers[COMMIT_FOOTERS] option is requested and
+this is the current patch set, contains the full commit message with
+Gerrit-specific commit footers, as if this revision were submitted
+using the link:project-configuration.html#cherry_pick[Cherry Pick]
+submit type.
 |===========================
 
 [[rule-input]]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.OutputFormat;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MetaDataUpdate;
@@ -131,6 +132,10 @@ public abstract class AbstractDaemonTest {
   @Inject
   protected Provider<InternalChangeQuery> queryProvider;
 
+  @Inject
+  @CanonicalWebUrl
+  protected Provider<String> canonicalWebUrl;
+
   @Inject
   @GerritServerConfig
   protected Config cfg;

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -15,6 +15,11 @@
 package com.google.gerrit.acceptance.api.change;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
+import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
+import static com.google.gerrit.server.project.Util.category;
+import static com.google.gerrit.server.project.Util.value;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -22,6 +27,8 @@ import com.google.common.collect.Sets;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.RebaseInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -33,8 +40,12 @@ import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.project.Util;
 
 import org.eclipse.jgit.lib.Constants;
 import org.junit.Test;
@@ -376,4 +387,60 @@ public class ChangeIT extends AbstractDaemonTest {
         .get(EnumSet.of(ListChangesOption.CHECK))
         .problems).isEmpty();
   }
+
+  @Test
+  public void commitFooters() throws Exception {
+    LabelType verified = category("Verified",
+        value(1, "Failed"), value(0, "No score"), value(-1, "Passes"));
+    LabelType custom1 = category("Custom1",
+        value(1, "Positive"), value(0, "No score"), value(-1, "Negative"));
+    LabelType custom2 = category("Custom2",
+        value(1, "Positive"), value(0, "No score"), value(-1, "Negative"));
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    cfg.getLabelSections().put(verified.getName(), verified);
+    cfg.getLabelSections().put(custom1.getName(), verified);
+    cfg.getLabelSections().put(custom2.getName(), verified);
+    String heads = "refs/heads/*";
+    AccountGroup.UUID anon =
+        SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
+    Util.allow(cfg, Permission.forLabel("Verified"), -1, 1, anon, heads);
+    Util.allow(cfg, Permission.forLabel("Custom1"), -1, 1, anon, heads);
+    Util.allow(cfg, Permission.forLabel("Custom2"), -1, 1, anon, heads);
+    saveProjectConfig(project, cfg);
+
+    PushOneCommit.Result r1 = createChange();
+    r1.assertOkStatus();
+    PushOneCommit.Result r2 = pushFactory.create(
+          db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new content",
+          r1.getChangeId())
+        .to("refs/for/master");
+    r2.assertOkStatus();
+
+    ReviewInput in = new ReviewInput();
+    in.label("Code-Review", 1);
+    in.label("Verified", 1);
+    in.label("Custom1", -1);
+    in.label("Custom2", 1);
+    gApi.changes().id(r2.getChangeId()).current().review(in);
+
+    EnumSet<ListChangesOption> options = EnumSet.of(
+        ListChangesOption.ALL_REVISIONS, ListChangesOption.COMMIT_FOOTERS);
+    ChangeInfo actual = gApi.changes().id(r2.getChangeId()).get(options);
+    assertThat(actual.revisions).hasSize(2);
+
+    // No footers except on latest patch set.
+    assertThat(actual.revisions.get(r1.getCommit().getName()).commitWithFooters)
+        .isNull();
+
+    String expected = SUBJECT + "\n"
+        + "\n"
+        + "Change-Id: " + r2.getChangeId() + "\n"
+        + "Reviewed-on: "
+            + canonicalWebUrl.get() + r2.getChange().getId() + "\n"
+        + "Reviewed-by: Administrator <admin@example.com>\n"
+        + "Custom2: Administrator <admin@example.com>\n"
+        + "Tested-by: Administrator <admin@example.com>\n";
+    assertThat(actual.revisions.get(r2.getCommit().getName()).commitWithFooters)
+        .isEqualTo(expected);
+  }
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java

@@ -58,7 +58,10 @@ public enum ListChangesOption {
   CHECK(15),
 
   /** Include allowed change actions client could perform. */
-  CHANGE_ACTIONS(16);
+  CHANGE_ACTIONS(16),
+
+  /** Include a copy of commit messages including review footers. */
+  COMMIT_FOOTERS(17);
 
   private final int value;
 

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RevisionInfo.java

@@ -29,4 +29,5 @@ public class RevisionInfo {
   public CommitInfo commit;
   public Map<String, FileInfo> files;
   public Map<String, ActionInfo> actions;
+  public String commitWithFooters;
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -19,6 +19,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.ALL_FILES;
 import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS;
 import static com.google.gerrit.extensions.client.ListChangesOption.CHANGE_ACTIONS;
 import static com.google.gerrit.extensions.client.ListChangesOption.CHECK;
+import static com.google.gerrit.extensions.client.ListChangesOption.COMMIT_FOOTERS;
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_ACTIONS;
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_COMMIT;
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_FILES;
@@ -92,9 +93,11 @@ import com.google.gerrit.server.WebLinks;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.LabelNormalizer;
+import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -133,6 +136,8 @@ public class ChangeJson {
   private final Provider<CurrentUser> userProvider;
   private final AnonymousUser anonymous;
   private final GitRepositoryManager repoManager;
+  private final ProjectCache projectCache;
+  private final MergeUtil.Factory mergeUtilFactory;
   private final IdentifiedUser.GenericFactory userFactory;
   private final ChangeData.Factory changeDataFactory;
   private final FileInfoJson fileInfoJson;
@@ -157,6 +162,8 @@ public class ChangeJson {
       Provider<CurrentUser> user,
       AnonymousUser au,
       GitRepositoryManager repoManager,
+      ProjectCache projectCache,
+      MergeUtil.Factory mergeUtilFactory,
       IdentifiedUser.GenericFactory uf,
       ChangeData.Factory cdf,
       FileInfoJson fileInfoJson,
@@ -173,9 +180,11 @@ public class ChangeJson {
     this.labelNormalizer = ln;
     this.userProvider = user;
     this.anonymous = au;
-    this.userFactory = uf;
     this.changeDataFactory = cdf;
     this.repoManager = repoManager;
+    this.userFactory = uf;
+    this.projectCache = projectCache;
+    this.mergeUtilFactory = mergeUtilFactory;
     this.fileInfoJson = fileInfoJson;
     this.accountLoaderFactory = ailf;
     this.downloadSchemes = downloadSchemes;
@@ -882,14 +891,22 @@ public class ChangeJson {
 
     boolean setCommit = has(ALL_COMMITS)
         || (out.isCurrent && has(CURRENT_COMMIT));
-    if (setCommit) {
+    boolean addFooters = out.isCurrent && has(COMMIT_FOOTERS);
+    if (setCommit || addFooters) {
       Project.NameKey project = c.getProject();
       try (Repository repo = repoManager.openRepository(project);
           RevWalk rw = new RevWalk(repo)) {
         String rev = in.getRevision().get();
         RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
         rw.parseBody(commit);
-        out.commit = toCommit(ctl, rw, commit, has(WEB_LINKS));
+        if (setCommit) {
+          out.commit = toCommit(ctl, rw, commit, has(WEB_LINKS));
+        }
+        if (addFooters) {
+          out.commitWithFooters = mergeUtilFactory
+              .create(projectCache.get(project))
+              .createCherryPickCommitMessage(commit, ctl, in.getId());
+        }
       }
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -25,13 +25,16 @@ import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.LabelId;
+import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Provider;
@@ -195,7 +198,9 @@ public class MergeUtil {
     }
   }
 
-  public String createCherryPickCommitMessage(final CodeReviewCommit n) {
+  public String createCherryPickCommitMessage(RevCommit n, ChangeControl ctl,
+      PatchSet.Id psId) {
+    Change c = ctl.getChange();
     final List<FooterLine> footers = n.getFooterLines();
     final StringBuilder msgbuf = new StringBuilder();
     msgbuf.append(n.getFullMessage());
@@ -215,16 +220,16 @@ public class MergeUtil {
       msgbuf.append('\n');
     }
 
-    if (!contains(footers, FooterConstants.CHANGE_ID, n.change().getKey().get())) {
+    if (!contains(footers, FooterConstants.CHANGE_ID, c.getKey().get())) {
       msgbuf.append(FooterConstants.CHANGE_ID.getName());
       msgbuf.append(": ");
-      msgbuf.append(n.change().getKey().get());
+      msgbuf.append(c.getKey().get());
       msgbuf.append('\n');
     }
 
     final String siteUrl = urlProvider.get();
     if (siteUrl != null) {
-      final String url = siteUrl + n.getPatchsetId().getParentKey().get();
+      final String url = siteUrl + c.getId().get();
       if (!contains(footers, FooterConstants.REVIEWED_ON, url)) {
         msgbuf.append(FooterConstants.REVIEWED_ON.getName());
         msgbuf.append(": ");
@@ -235,7 +240,7 @@ public class MergeUtil {
 
     PatchSetApproval submitAudit = null;
 
-    for (final PatchSetApproval a : safeGetApprovals(n)) {
+    for (final PatchSetApproval a : safeGetApprovals(ctl, psId)) {
       if (a.getValue() <= 0) {
         // Negative votes aren't counted.
         continue;
@@ -301,6 +306,10 @@ public class MergeUtil {
     return msgbuf.toString();
   }
 
+  public String createCherryPickCommitMessage(final CodeReviewCommit n) {
+    return createCherryPickCommitMessage(n, n.getControl(), n.getPatchsetId());
+  }
+
   private static boolean isCodeReview(LabelId id) {
     return "Code-Review".equalsIgnoreCase(id.get());
   }
@@ -309,11 +318,12 @@ public class MergeUtil {
     return "Verified".equalsIgnoreCase(id.get());
   }
 
-  private Iterable<PatchSetApproval> safeGetApprovals(CodeReviewCommit n) {
+  private Iterable<PatchSetApproval> safeGetApprovals(
+      ChangeControl ctl, PatchSet.Id psId) {
     try {
-      return approvalsUtil.byPatchSet(db.get(), n.getControl(), n.getPatchsetId());
+      return approvalsUtil.byPatchSet(db.get(), ctl, psId);
     } catch (OrmException e) {
-      log.error("Can't read approval records for " + n.getPatchsetId(), e);
+      log.error("Can't read approval records for " + psId, e);
       return Collections.emptyList();
     }
   }