diff --git a/java/com/google/gerrit/server/git/MergeUtil.java b/java/com/google/gerrit/server/git/MergeUtil.java index 0d427e676b..208dafb743 100644 --- a/java/com/google/gerrit/server/git/MergeUtil.java +++ b/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 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; } } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java index 31813e336a..36595cec97 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java +++ b/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 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 diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java index 7eec8548d3..d9a26aa03b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java +++ b/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 changeMessageModifiers; + @Inject private DynamicItem 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 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 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 \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 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 {