Merge changes from topic 'already-merged-message'

* changes:
  Insert ChangeMessage when repairing ALREADY_MERGED change
  Don't reuse ALREADY_MERGED for an identical tree

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

@@ -24,7 +24,6 @@ import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -153,7 +152,6 @@ public abstract class AbstractSubmitByMerge extends AbstractSubmit {
     ChangeInfo info = gApi.changes().id(id2.get()).get();
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
-    ChangeMessageInfo lastMessage = Iterables.getLast(info.messages);
 
     RevCommit tip;
     try (Repository repo = repoManager.openRepository(project);
@@ -177,7 +175,7 @@ public abstract class AbstractSubmitByMerge extends AbstractSubmit {
     assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
     assertThat(Iterables.getLast(info.messages).message)
-        .isEqualTo(lastMessage.message);
+        .isEqualTo("Change has been successfully merged by Administrator");
 
     try (Repository repo = repoManager.openRepository(project)) {
       assertThat(repo.exactRef("refs/heads/master").getObjectId())

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

@@ -25,11 +25,11 @@ import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.change.Submit.TestSubmitInput;
+import com.google.gerrit.server.git.strategy.CommitMergeStatus;
 
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
@@ -268,6 +268,31 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     assertNew(change3.getChangeId());
   }
 
+  @Test
+  @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
+  public void submitIdenticalTree() throws Exception {
+    RevCommit initialHead = getRemoteHead();
+
+    PushOneCommit.Result change1 = createChange("Change 1", "a.txt", "a");
+
+    testRepo.reset(initialHead);
+    PushOneCommit.Result change2 = createChange("Change 2", "a.txt", "a");
+
+    submit(change1.getChangeId());
+    RevCommit oldHead = getRemoteHead();
+    assertThat(oldHead.getShortMessage()).isEqualTo("Change 1");
+
+    // Don't check merge result, since ref isn't updated.
+    submit(change2.getChangeId(), new SubmitInput(), null, null, false);
+
+    assertThat(getRemoteHead()).isEqualTo(oldHead);
+
+    ChangeInfo info2 = get(change2.getChangeId());
+    assertThat(info2.status).isEqualTo(ChangeStatus.MERGED);
+    assertThat(Iterables.getLast(info2.messages).message)
+        .isEqualTo(CommitMergeStatus.SKIPPED_IDENTICAL_TREE.getMessage());
+  }
+
   @Test
   public void repairChangeStateAfterFailure() throws Exception {
     RevCommit initialHead = getRemoteHead();
@@ -292,7 +317,6 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
     assertThat(getPatchSet(psId2)).isNull();
-    ChangeMessageInfo lastMessage = Iterables.getLast(info.messages);
 
     ObjectId rev2;
     try (Repository repo = repoManager.openRepository(project);
@@ -320,7 +344,8 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     assertThat(ps2).isNotNull();
     assertThat(ps2.getRevision().get()).isEqualTo(rev2.name());
     assertThat(Iterables.getLast(info.messages).message)
-        .isEqualTo(lastMessage.message);
+        .isEqualTo("Change has been successfully cherry-picked as "
+            + rev2.name() + " by Administrator");
 
     try (Repository repo = repoManager.openRepository(project)) {
       assertThat(repo.exactRef("refs/heads/master").getObjectId())

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

@@ -23,7 +23,6 @@ import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -133,7 +132,6 @@ public class SubmitByFastForwardIT extends AbstractSubmit {
     ChangeInfo info = gApi.changes().id(id.get()).get();
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
-    ChangeMessageInfo lastMessage = Iterables.getLast(info.messages);
 
     ObjectId rev;
     try (Repository repo = repoManager.openRepository(project);
@@ -151,7 +149,7 @@ public class SubmitByFastForwardIT extends AbstractSubmit {
     assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
     assertThat(Iterables.getLast(info.messages).message)
-        .isEqualTo(lastMessage.message);
+        .isEqualTo("Change has been successfully merged by Administrator");
 
     try (Repository repo = repoManager.openRepository(project)) {
       assertThat(repo.exactRef("refs/heads/master").getObjectId())

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

@@ -24,7 +24,6 @@ import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -187,7 +186,6 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
     assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1);
     assertThat(getPatchSet(psId2)).isNull();
-    ChangeMessageInfo lastMessage = Iterables.getLast(info.messages);
 
     ObjectId rev2;
     try (Repository repo = repoManager.openRepository(project);
@@ -215,7 +213,8 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
     assertThat(ps2).isNotNull();
     assertThat(ps2.getRevision().get()).isEqualTo(rev2.name());
     assertThat(Iterables.getLast(info.messages).message)
-        .isEqualTo(lastMessage.message);
+        .isEqualTo("Change has been successfully rebased as "
+            + rev2.name() + " by Administrator");
 
     try (Repository repo = repoManager.openRepository(project)) {
       assertThat(repo.exactRef("refs/heads/master").getObjectId())

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.git.strategy;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.SKIPPED_IDENTICAL_TREE;
 
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -126,7 +127,7 @@ public class CherryPick extends SubmitStrategy {
         toMerge.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
         return;
       } catch (MergeIdenticalTreeException mie) {
-        toMerge.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
+        toMerge.setStatusCode(SKIPPED_IDENTICAL_TREE);
         return;
       }
       // Initial copy doesn't have new patch set ID since change hasn't been
@@ -146,6 +147,10 @@ public class CherryPick extends SubmitStrategy {
     @Override
     public PatchSet updateChangeImpl(ChangeContext ctx) throws OrmException,
          NoSuchChangeException, IOException {
+      if (newCommit == null
+          && toMerge.getStatusCode() == SKIPPED_IDENTICAL_TREE) {
+        return null;
+      }
       checkState(newCommit != null,
           "no new commit produced by CherryPick of %s, expected to fail fast",
           toMerge.change().getId());

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

@@ -36,6 +36,9 @@ public enum CommitMergeStatus {
           + "\n"
           + "Please rebase the change locally and upload the rebased commit for review."),
 
+  SKIPPED_IDENTICAL_TREE(
+      "Marking change merged without cherry-picking to branch, as the resulting commit would be empty."),
+
   MISSING_DEPENDENCY(""),
 
   MANUAL_RECURSIVE_MERGE("The change requires a local merge to resolve.\n"

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

@@ -78,6 +78,7 @@ public abstract class SubmitStrategy {
   static class Arguments {
     interface Factory {
       Arguments create(
+          SubmitType submitType,
           Branch.NameKey destBranch,
           CommitStatus commits,
           CodeReviewRevWalk rw,
@@ -118,6 +119,7 @@ public abstract class SubmitStrategy {
     final ReviewDb db;
     final Set<RevCommit> alreadyAccepted;
     final String submissionId;
+    final SubmitType submitType;
 
     final ProjectState project;
     final MergeSorter mergeSorter;
@@ -151,7 +153,8 @@ public abstract class SubmitStrategy {
         @Assisted RevFlag canMergeFlag,
         @Assisted ReviewDb db,
         @Assisted Set<RevCommit> alreadyAccepted,
-        @Assisted String submissionId) {
+        @Assisted String submissionId,
+        @Assisted SubmitType submitType) {
       this.accountCache = accountCache;
       this.approvalsUtil = approvalsUtil;
       this.batchUpdateFactory = batchUpdateFactory;
@@ -179,6 +182,7 @@ public abstract class SubmitStrategy {
       this.db = db;
       this.alreadyAccepted = alreadyAccepted;
       this.submissionId = submissionId;
+      this.submitType = submitType;
 
       this.project = checkNotNull(projectCache.get(destBranch.getParentKey()),
             "project not found: %s", destBranch.getParentKey());

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

@@ -53,9 +53,9 @@ public class SubmitStrategyFactory {
       Branch.NameKey destBranch, IdentifiedUser caller, MergeTip mergeTip,
       CommitStatus commits, String submissionId)
       throws IntegrationException {
-    SubmitStrategy.Arguments args = argsFactory.create(destBranch, commits, rw,
+    SubmitStrategy.Arguments args = argsFactory.create(submitType, destBranch,
-        caller, mergeTip, inserter, repo, canMergeFlag, db, alreadyAccepted,
+        commits, rw, caller, mergeTip, inserter, repo, canMergeFlag, db,
-        submissionId);
+        alreadyAccepted, submissionId);
     switch (submitType) {
       case CHERRY_PICK:
         return new CherryPick(args);

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

@@ -106,6 +106,7 @@ public class SubmitStrategyListener extends BatchUpdate.Listener {
         case CLEAN_MERGE:
         case CLEAN_REBASE:
         case CLEAN_PICK:
+        case SKIPPED_IDENTICAL_TREE:
           break; // Merge strategy accepted this change.
 
         case ALREADY_MERGED:

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

@@ -265,22 +265,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
           // corresponding to the merge result. This results in a different
           // ChangeMergedEvent in the fixup case, but we'll just live with that.
           : alreadyMerged;
-      String txt = s.getMessage();
+      setMerged(ctx, message(ctx, commit, s));
-
-      ChangeMessage msg;
-      if (s == CommitMergeStatus.CLEAN_MERGE) {
-        msg = message(ctx, commit.getPatchsetId(), txt + getByAccountName());
-      } else if (s == CommitMergeStatus.CLEAN_REBASE
-          || s == CommitMergeStatus.CLEAN_PICK) {
-        msg = message(ctx, commit.getPatchsetId(),
-            txt + " as " + commit.name() + getByAccountName());
-      } else if (s == CommitMergeStatus.ALREADY_MERGED) {
-        msg = null;
-      } else {
-        throw new IllegalStateException("unexpected status " + s +
-            " for change " + c.getId() + "; expected to previously fail fast");
-      }
-      setMerged(ctx, msg);
     } catch (OrmException err) {
       String msg = "Error updating change status for " + id;
       log.error(msg, err);
@@ -425,6 +410,39 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
     return "";
   }
 
+  private ChangeMessage message(ChangeContext ctx, CodeReviewCommit commit,
+      CommitMergeStatus s) {
+    String txt = s.getMessage();
+    if (s == CommitMergeStatus.CLEAN_MERGE) {
+      return message(ctx, commit.getPatchsetId(), txt + getByAccountName());
+    } else if (s == CommitMergeStatus.CLEAN_REBASE
+        || s == CommitMergeStatus.CLEAN_PICK) {
+      return message(ctx, commit.getPatchsetId(),
+          txt + " as " + commit.name() + getByAccountName());
+    } else if (s == CommitMergeStatus.SKIPPED_IDENTICAL_TREE) {
+      return message(ctx, commit.getPatchsetId(), txt);
+    } else if (s == CommitMergeStatus.ALREADY_MERGED) {
+      // Best effort to mimic the message that would have happened had this
+      // succeeded the first time around.
+      switch (args.submitType) {
+        case FAST_FORWARD_ONLY:
+        case MERGE_ALWAYS:
+        case MERGE_IF_NECESSARY:
+          return message(ctx, commit, CommitMergeStatus.CLEAN_MERGE);
+        case CHERRY_PICK:
+          return message(ctx, commit, CommitMergeStatus.CLEAN_PICK);
+        case REBASE_IF_NECESSARY:
+          return message(ctx, commit, CommitMergeStatus.CLEAN_REBASE);
+        default:
+          return message(ctx, commit, null);
+      }
+    } else {
+      throw new IllegalStateException("unexpected status " + s
+          + " for change " + commit.change().getId()
+          + "; expected to previously fail fast");
+    }
+  }
+
   private ChangeMessage message(ChangeContext ctx, PatchSet.Id psId,
       String body) {
     checkNotNull(psId);