Merge changes If23c598d,I5fd1a074

* changes:
  MergeUtil: Add debug logging to PluggableCommitMessageGenerator
  Expand acceptance tests for ChangeMessageModifiers

java/com/google/gerrit/server/git/MergeUtil.java

@@ -32,6 +32,7 @@ import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
@@ -125,20 +126,31 @@ public class MergeUtil {
     }
 
     public String generate(
-        RevCommit original, RevCommit mergeTip, Branch.NameKey dest, String current) {
+        RevCommit original, RevCommit mergeTip, Branch.NameKey dest, String originalMessage) {
       requireNonNull(original.getRawBuffer());
       if (mergeTip != null) {
         requireNonNull(mergeTip.getRawBuffer());
       }
-      for (ChangeMessageModifier changeMessageModifier : changeMessageModifiers) {
+
+      int count = 0;
+      String current = originalMessage;
+      for (Extension<ChangeMessageModifier> ext : changeMessageModifiers.entries()) {
+        ChangeMessageModifier changeMessageModifier = ext.get();
+        String className = changeMessageModifier.getClass().getName();
         current = changeMessageModifier.onSubmit(current, original, mergeTip, dest);
-        requireNonNull(
-            current,
-            () ->
-                String.format(
-                    "%s.OnSubmit returned null instead of new commit message",
-                    changeMessageModifier.getClass().getName()));
+        checkState(
+            current != null,
+            "%s.onSubmit from plugin %s returned null instead of new commit message",
+            className,
+            ext.getPluginName());
+        count++;
+        logger.atFine().log(
+            "Invoked %s from plugin %s, message length now %d",
+            className, ext.getPluginName(), current.length());
       }
+      logger.atFine().log(
+          "Invoked %d ChangeMessageModifiers on message with original length %d",
+          count, originalMessage.length());
       return current;
     }
   }

javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java

@@ -70,7 +70,8 @@ public abstract class AbstractSubmitByRebase extends AbstractSubmit {
     submitWithRebase(user);
   }
 
-  private void submitWithRebase(TestAccount submitter) throws Exception {
+  protected ImmutableList<PushOneCommit.Result> submitWithRebase(TestAccount submitter)
+      throws Exception {
     requestScopeOperations.setApiUser(submitter.getId());
     RevCommit initialHead = getRemoteHead();
     PushOneCommit.Result change = createChange("Change 1", "a.txt", "content");
@@ -97,6 +98,7 @@ public abstract class AbstractSubmitByRebase extends AbstractSubmit {
         headAfterFirstSubmit.name(),
         change2.getChangeId(),
         headAfterSecondSubmit.name());
+    return ImmutableList.of(change, change2);
   }
 
   @Test

javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java

@@ -15,25 +15,35 @@
 package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
 import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
 
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
 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 com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.RegistrationHandle;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.config.UrlFormatter;
 import com.google.gerrit.server.git.ChangeMessageModifier;
+import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
-import java.util.List;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Test;
 
 public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
   @Inject private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
+  @Inject private DynamicItem<UrlFormatter> urlFormatter;
 
   @Override
   protected SubmitType getSubmitType() {
@@ -79,36 +89,86 @@ public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
   }
 
   @Test
-  @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
-  public void changeMessageOnSubmit() throws Exception {
-    PushOneCommit.Result change1 = createChange();
-    PushOneCommit.Result change2 = createChange();
+  public void rebaseInvokesChangeMessageModifiers() throws Exception {
+    ChangeMessageModifier modifier1 =
+        (msg, orig, tip, dest) -> msg + "This-change-before-rebase: " + orig.name() + "\n";
+    ChangeMessageModifier modifier2 =
+        (msg, orig, tip, dest) -> msg + "Previous-step-tip: " + tip.name() + "\n";
+    ChangeMessageModifier modifier3 =
+        (msg, orig, tip, dest) -> msg + "Dest: " + dest.getShortName() + "\n";
 
-    RegistrationHandle handle =
-        changeMessageModifiers.add(
-            "gerrit",
-            (newCommitMessage, original, mergeTip, destination) -> {
-              List<String> custom = mergeTip.getFooterLines("Custom");
-              if (!custom.isEmpty()) {
-                newCommitMessage += "Custom-Parent: " + custom.get(0) + "\n";
-              }
-              return newCommitMessage + "Custom: " + destination.get();
-            });
-    try {
-      // 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());
-    } finally {
-      handle.remove();
+    try (AutoCloseable ignored = installChangeMessageModifiers(modifier1, modifier2, modifier3)) {
+      ImmutableList<PushOneCommit.Result> changes = submitWithRebase(admin);
+      ChangeData cd1 = changes.get(0).getChange();
+      ChangeData cd2 = changes.get(1).getChange();
+      assertThat(cd2.patchSets()).hasSize(2);
+      String change1CurrentCommit = cd1.currentPatchSet().getRevision().get();
+      String change2Ps1Commit = cd2.patchSet(new PatchSet.Id(cd2.getId(), 1)).getRevision().get();
+
+      assertThat(gApi.changes().id(cd2.getId().get()).revision(2).commit(false).message)
+          .isEqualTo(
+              "Change 2\n\n"
+                  + ("Change-Id: " + cd2.change().getKey() + "\n")
+                  + ("Reviewed-on: "
+                      + urlFormatter.get().getChangeViewUrl(project, cd2.getId()).get()
+                      + "\n")
+                  + "Reviewed-by: Administrator <admin@example.com>\n"
+                  + ("This-change-before-rebase: " + change2Ps1Commit + "\n")
+                  + ("Previous-step-tip: " + change1CurrentCommit + "\n")
+                  + "Dest: master\n");
     }
-    // ... but both changes should get custom footers.
-    assertThat(getCurrentCommit(change1).getFooterLines("Custom"))
-        .containsExactly("refs/heads/master");
-    assertThat(getCurrentCommit(change2).getFooterLines("Custom"))
-        .containsExactly("refs/heads/master");
-    assertThat(getCurrentCommit(change2).getFooterLines("Custom-Parent"))
-        .containsExactly("refs/heads/master");
+  }
+
+  @Test
+  public void failingChangeMessageModifierShortCircuits() throws Exception {
+    ChangeMessageModifier modifier1 =
+        (msg, orig, tip, dest) -> {
+          throw new IllegalStateException("boom");
+        };
+    ChangeMessageModifier modifier2 = (msg, orig, tip, dest) -> msg + "A-footer: value\n";
+    try (AutoCloseable ignored = installChangeMessageModifiers(modifier1, modifier2)) {
+      try {
+        submitWithRebase();
+        assert_().fail("expected ResourceConflictException");
+      } catch (ResourceConflictException e) {
+        Throwable cause = Throwables.getRootCause(e);
+        assertThat(cause).isInstanceOf(RuntimeException.class);
+        assertThat(cause).hasMessageThat().isEqualTo("boom");
+      }
+    }
+  }
+
+  @Test
+  public void changeMessageModifierReturningNullShortCircuits() throws Exception {
+    ChangeMessageModifier modifier1 = (msg, orig, tip, dest) -> null;
+    ChangeMessageModifier modifier2 = (msg, orig, tip, dest) -> msg + "A-footer: value\n";
+    try (AutoCloseable ignored = installChangeMessageModifiers(modifier1, modifier2)) {
+      try {
+        submitWithRebase();
+        assert_().fail("expected ResourceConflictException");
+      } catch (ResourceConflictException e) {
+        Throwable cause = Throwables.getRootCause(e);
+        assertThat(cause).isInstanceOf(RuntimeException.class);
+        assertThat(cause)
+            .hasMessageThat()
+            .isEqualTo(
+                modifier1.getClass().getName()
+                    + ".onSubmit from plugin modifier-1 returned null instead of new commit"
+                    + " message");
+      }
+    }
+  }
+
+  private AutoCloseable installChangeMessageModifiers(ChangeMessageModifier... modifiers) {
+    Deque<RegistrationHandle> handles = new ArrayDeque<>(modifiers.length);
+    for (int i = 0; i < modifiers.length; i++) {
+      handles.push(changeMessageModifiers.add("modifier-" + (i + 1), modifiers[i]));
+    }
+    return () -> {
+      while (!handles.isEmpty()) {
+        handles.pop().remove();
+      }
+    };
   }
 
   private void assertLatestRevisionHasFooters(PushOneCommit.Result change) throws Exception {