Rebase-Always strategy: always generate footers.

Repurpose Cherry-Pick's strategy message generation for wider use.
This completes work on Rebase-Always strategy.

Bug: issue 4452
Change-Id: Id282999ae0c12a57edd35207220a640a27fc3659

Documentation/project-configuration.txt

@@ -103,7 +103,8 @@ submit type unless
 link:config-gerrit.html#change.submitWholeTopic[`change.submitWholeTopic`]
 is enabled and depending changes share the same topic. So generally
 submitters must remember to submit changes in the right order when using this
-submit type.
+submit type. If all you want is extra information in the commit message,
+consider using the Rebase Always submit strategy.
 
 [[rebase_if_necessary]]
 * Rebase If Necessary
@@ -120,8 +121,12 @@ the same file has also been changed on the other side of the merge.
 [[rebase_always]]
 * Rebase Always
 +
-Basically, the same as Rebase If Necessary, but it creates a new patchset even if
+Basically, the same as Rebase If Necessary, but it creates a new patchset even
-fast forward is possible. In this regard, it's similar to Cherry Pick, but with
+if fast forward is possible AND like Cherry Pick it ensures footers such as
+Change-Id, Reviewed-On, and others are present in resulting commit that is
+merged.
+
+Thus, Rebase Always can be considered similar to Cherry Pick, but with
 the important distinction that Rebase Always does not ignore dependencies.
 
 [[content_merge]]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java

@@ -18,9 +18,12 @@ import static com.google.common.truth.Truth.assertThat;
 
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ChangeInfo;
 
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Test;
 
@@ -50,4 +53,44 @@ public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
     assertRefUpdatedEvents(oldHead, head);
     assertChangeMergedEvents(change.getChangeId(), head.name());
   }
+
+  @Test
+  @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
+  public void alwaysAddFooters() throws Exception {
+    PushOneCommit.Result change1 = createChange();
+    PushOneCommit.Result change2 = createChange();
+
+    assertThat(
+        getCurrentCommit(change1).getFooterLines(FooterConstants.REVIEWED_BY))
+            .isEmpty();
+    assertThat(
+        getCurrentCommit(change2).getFooterLines(FooterConstants.REVIEWED_BY))
+            .isEmpty();
+
+    // change1 is a fast-forward, but should be rebased in cherry pick style
+    // anyway, making change2 not a fast-forward, requiring a rebase.
+    approve(change1.getChangeId());
+    submit(change2.getChangeId());
+    // ... but both changes should get reviewed-by footers.
+    assertLatestRevisionHasFooters(change1);
+    assertLatestRevisionHasFooters(change2);
+  }
+
+  private void assertLatestRevisionHasFooters(PushOneCommit.Result change)
+      throws Exception {
+    RevCommit c = getCurrentCommit(change);
+    assertThat(c.getFooterLines(FooterConstants.CHANGE_ID)).isNotEmpty();
+    assertThat(c.getFooterLines(FooterConstants.REVIEWED_BY)).isNotEmpty();
+    assertThat(c.getFooterLines(FooterConstants.REVIEWED_ON)).isNotEmpty();
+  }
+
+  private RevCommit getCurrentCommit(PushOneCommit.Result change)
+      throws Exception {
+    testRepo.git().fetch().setRemote("origin").call();
+    ChangeInfo info = get(change.getChangeId());
+    RevCommit c = testRepo.getRevWalk()
+        .parseCommit(ObjectId.fromString(info.currentRevision));
+    testRepo.getRevWalk().parseBody(c);
+    return c;
+  }
 }

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

@@ -1112,7 +1112,7 @@ public class ChangeJson {
         if (addFooters) {
           out.commitWithFooters = mergeUtilFactory
               .create(projectCache.get(project))
-              .createCherryPickCommitMessage(commit, ctl, in.getId());
+              .createDetailedCommitMessage(commit, ctl, in.getId());
         }
       }
     }

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

@@ -68,6 +68,7 @@ public class RebaseChangeOp extends BatchUpdate.Op {
   private CommitValidators.Policy validate;
   private boolean forceContentMerge;
   private boolean copyApprovals = true;
+  private boolean detailedCommitMessage = false;
   private boolean postMessage = true;
 
   private RevCommit rebasedCommit;
@@ -118,6 +119,12 @@ public class RebaseChangeOp extends BatchUpdate.Op {
     return this;
   }
 
+  public RebaseChangeOp setDetailedCommitMessage(
+      boolean detailedCommitMessage) {
+    this.detailedCommitMessage = detailedCommitMessage;
+    return this;
+  }
+
   public RebaseChangeOp setPostMessage(boolean postMessage) {
     this.postMessage = postMessage;
     return this;
@@ -134,6 +141,15 @@ public class RebaseChangeOp extends BatchUpdate.Op {
     RevWalk rw = ctx.getRevWalk();
     RevCommit original = rw.parseCommit(ObjectId.fromString(oldRev.get()));
     rw.parseBody(original);
+
+    String newCommitMessage;
+    if (detailedCommitMessage) {
+      newCommitMessage = newMergeUtil().createDetailedCommitMessage(original,
+          ctl, originalPatchSet.getId());
+    } else {
+      newCommitMessage = original.getFullMessage();
+    }
+
     RevCommit baseCommit;
     if (baseCommitish != null) {
        baseCommit = rw.parseCommit(ctx.getRepository().resolve(baseCommitish));
@@ -143,7 +159,7 @@ public class RebaseChangeOp extends BatchUpdate.Op {
            ctx.getRepository(), ctx.getRevWalk()));
     }
 
-    rebasedCommit = rebaseCommit(ctx, original, baseCommit);
+    rebasedCommit = rebaseCommit(ctx, original, baseCommit, newCommitMessage);
 
     RevId baseRevId = new RevId((baseCommitish != null) ? baseCommitish
         : ObjectId.toString(baseCommit.getId()));
@@ -223,8 +239,8 @@ public class RebaseChangeOp extends BatchUpdate.Op {
    * @throws IOException the merge failed for another reason.
    */
   private RevCommit rebaseCommit(RepoContext ctx, RevCommit original,
-      ObjectId base) throws ResourceConflictException, MergeConflictException,
+      ObjectId base, String commitMessage)
-      IOException {
+      throws ResourceConflictException, MergeConflictException, IOException {
     RevCommit parentCommit = original.getParent(0);
 
     if (base.equals(parentCommit)) {
@@ -245,7 +261,7 @@ public class RebaseChangeOp extends BatchUpdate.Op {
     cb.setTreeId(merger.getResultTreeId());
     cb.setParentId(base);
     cb.setAuthor(original.getAuthorIdent());
-    cb.setMessage(original.getFullMessage());
+    cb.setMessage(commitMessage);
     if (committerIdent != null) {
       cb.setCommitter(committerIdent);
     } else {

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

@@ -246,7 +246,24 @@ public class MergeUtil {
     return sb.toString();
   }
 
-  public String createCherryPickCommitMessage(RevCommit n, ChangeControl ctl,
+  /**
+   * Adds footers to existing commit message based on the state of the change.
+   *
+   * This adds the following footers if they are missing:
+   *
+   * <ul>
+   *   <li> Reviewed-on: <i>url</i></li>
+   *   <li> Reviewed-by | Tested-by | <i>Other-Label-Name</i>: <i>reviewer</i>
+   *   </li>
+   *   <li> Change-Id </li>
+   * </ul>
+   *
+   * @param n
+   * @param ctl
+   * @param psId
+   * @return new message
+   */
+  public String createDetailedCommitMessage(RevCommit n, ChangeControl ctl,
       PatchSet.Id psId) {
     Change c = ctl.getChange();
     final List<FooterLine> footers = n.getFooterLines();
@@ -354,8 +371,8 @@ public class MergeUtil {
     return msgbuf.toString();
   }
 
-  public String createCherryPickCommitMessage(final CodeReviewCommit n) {
+  public String createDetailedCommitMessage(final CodeReviewCommit n) {
-    return createCherryPickCommitMessage(n, n.getControl(), n.getPatchsetId());
+    return createDetailedCommitMessage(n, n.getControl(), n.getPatchsetId());
   }
 
   private static boolean isCodeReview(LabelId id) {

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -100,7 +100,7 @@ public class CherryPick extends SubmitStrategy {
       psId = ChangeUtil.nextPatchSetId(
           args.repo, toMerge.change().currentPatchSetId());
       String cherryPickCmtMsg =
-          args.mergeUtil.createCherryPickCommitMessage(toMerge);
+          args.mergeUtil.createDetailedCommitMessage(toMerge);
 
       PersonIdent committer = args.caller.newCommitterIdent(
           ctx.getWhen(), args.serverIdent.getTimeZone());

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java

@@ -139,7 +139,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
             args.repo, toMerge.change().currentPatchSetId());
         // TODO(tandrii): add extension point to customize this commit message.
         String cherryPickCmtMsg =
-            args.mergeUtil.createCherryPickCommitMessage(toMerge);
+            args.mergeUtil.createDetailedCommitMessage(toMerge);
 
         PersonIdent committer = args.caller.newCommitterIdent(ctx.getWhen(),
             args.serverIdent.getTimeZone());
@@ -171,6 +171,9 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
             // later anyway.
             .setCopyApprovals(false)
             .setValidatePolicy(CommitValidators.Policy.NONE)
+            // RebaseAlways should set always modify commit message like
+            // Cherry-Pick strategy.
+            .setDetailedCommitMessage(rebaseAlways)
             // Do not post message after inserting new patchset because there
             // will be one about change being merged already.
             .setPostMessage(false);