Add extension point to customize detailed commit message generation.

Plugins implementing new ChangeMessageModifier interface can modify
commit message when change is being submitted by Rebase Always and
Cherry Pick submit strategies as well as when change is queried with
COMMIT_FOOTERS option.

Intended usage is insertion of custom footer lines depending on the
state of the change and the destination branch.

Bug: Issue 5003
Change-Id: Ica56762381996db5d6f72b9e04f4588089039b44
This commit is contained in:
Andrii Shyshkalov 2016-11-29 17:45:01 +01:00
parent f739ffb777
commit 6fdc8ebf3d
11 changed files with 277 additions and 30 deletions

View File

@ -482,6 +482,14 @@ a DynamicItem, so Gerrit may only have one copy.
Certain operations in Gerrit can be validated by plugins by
implementing the corresponding link:config-validation.html[listeners].
[[change-message-modifier]]
== Change Message Modifier
`com.google.gerrit.server.git.ChangeMessageModifier`:
plugins implementing this can modify commit message of the change being
submitted by Rebase Always and Cherry Pick submit strategies as well as
change being queried with COMMIT_FOOTERS option.
[[receive-pack]]
== Receive Pack Initializers

View File

@ -73,6 +73,8 @@ import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.MergeInput;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@ -91,6 +93,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.ChangeControl;
@ -129,6 +132,9 @@ public class ChangeIT extends AbstractDaemonTest {
@Inject
private BatchUpdate.Factory updateFactory;
@Inject
private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Before
public void setTimeForTesting() {
systemTimeZone = System.setProperty("user.timezone", "US/Eastern");
@ -1809,6 +1815,42 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(footers).containsExactlyElementsIn(expectedFooters);
}
@Test
public void customCommitFooters() throws Exception {
PushOneCommit.Result change = createChange();
RegistrationHandle handle =
changeMessageModifiers.add(new ChangeMessageModifier() {
@Override
public String onSubmit(String newCommitMessage, RevCommit original,
RevCommit mergeTip, ChangeControl ctl) {
return newCommitMessage + "Custom: "
+ ctl.getChange().getDest().get();
}
});
ChangeInfo actual;
try {
EnumSet<ListChangesOption> options = EnumSet.of(
ListChangesOption.ALL_REVISIONS, ListChangesOption.COMMIT_FOOTERS);
actual = gApi.changes().id(change.getChangeId()).get(options);
} finally {
handle.remove();
}
List<String> footers = new ArrayList<>(Arrays.asList(
actual.revisions.get(change.getCommit().getName()).commitWithFooters
.split("\\n")));
// remove subject + blank line
footers.remove(0);
footers.remove(0);
List<String> expectedFooters =
Arrays.asList(
"Change-Id: " + change.getChangeId(), "Reviewed-on: "
+ canonicalWebUrl.get() + change.getChange().getId(),
"Custom: refs/heads/master");
assertThat(footers).containsExactlyElementsIn(expectedFooters);
}
@Test
public void defaultSearchDoesNotTouchDatabase() throws Exception {
setApiUser(admin);

View File

@ -19,17 +19,23 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.FooterConstants;
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.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
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.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.change.Submit.TestSubmitInput;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.strategy.CommitMergeStatus;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@ -40,6 +46,8 @@ import org.junit.Test;
import java.util.List;
public class SubmitByCherryPickIT extends AbstractSubmit {
@Inject
private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Override
protected SubmitType getSubmitType() {
@ -88,6 +96,32 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
change2.getChangeId(), newHead.name());
}
@Test
public void changeMessageOnSubmit() throws Exception {
PushOneCommit.Result change = createChange();
RegistrationHandle handle =
changeMessageModifiers.add(new ChangeMessageModifier() {
@Override
public String onSubmit(String newCommitMessage, RevCommit original,
RevCommit mergeTip, ChangeControl ctl) {
return newCommitMessage + "Custom: "
+ ctl.getChange().getDest().get();
}
});
try {
submit(change.getChangeId());
} finally {
handle.remove();
}
testRepo.git().fetch().setRemote("origin").call();
ChangeInfo info = get(change.getChangeId());
RevCommit c = testRepo.getRevWalk()
.parseCommit(ObjectId.fromString(info.currentRevision));
testRepo.getRevWalk().parseBody(c);
assertThat(c.getFooterLines("Custom")).containsExactly("refs/heads/master");
assertThat(c.getFooterLines(FooterConstants.REVIEWED_ON)).hasSize(1);
}
@Test
@TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
public void submitWithContentMerge() throws Exception {

View File

@ -22,12 +22,21 @@ 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.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
import java.util.List;
public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
@Inject
private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Override
protected SubmitType getSubmitType() {
@ -76,6 +85,42 @@ public class SubmitByRebaseAlwaysIT extends AbstractSubmitByRebase {
assertLatestRevisionHasFooters(change2);
}
@Test
@TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
public void changeMessageOnSubmit() throws Exception {
PushOneCommit.Result change1 = createChange();
PushOneCommit.Result change2 = createChange();
RegistrationHandle handle =
changeMessageModifiers.add(new ChangeMessageModifier() {
@Override
public String onSubmit(String newCommitMessage, RevCommit original,
RevCommit mergeTip, ChangeControl ctl) {
List<String> custom = mergeTip.getFooterLines("Custom");
if (!custom.isEmpty()) {
newCommitMessage += "Custom-Parent: " + custom.get(0) + "\n";
}
return newCommitMessage + "Custom: "
+ ctl.getChange().getDest().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();
}
// ... 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");
}
private void assertLatestRevisionHasFooters(PushOneCommit.Result change)
throws Exception {
RevCommit c = getCurrentCommit(change);

View File

@ -121,6 +121,7 @@ import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@ -1111,9 +1112,15 @@ public class ChangeJson {
out.commit = toCommit(ctl, rw, commit, has(WEB_LINKS), fillCommit);
}
if (addFooters) {
Ref ref = repo.exactRef(in.getRefName());
RevCommit mergeTip = null;
if (ref != null){
mergeTip = rw.parseCommit(ref.getObjectId());
rw.parseBody(mergeTip);
}
out.commitWithFooters = mergeUtilFactory
.create(projectCache.get(project))
.createDetailedCommitMessage(commit, ctl, in.getId());
.createCommitMessageOnSubmit(commit, mergeTip, ctl, in.getId());
}
}
}

View File

@ -142,14 +142,6 @@ public class RebaseChangeOp extends BatchUpdate.Op {
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));
@ -159,6 +151,15 @@ public class RebaseChangeOp extends BatchUpdate.Op {
ctx.getRepository(), ctx.getRevWalk()));
}
String newCommitMessage;
if (detailedCommitMessage) {
rw.parseBody(baseCommit);
newCommitMessage = newMergeUtil().createCommitMessageOnSubmit(original,
baseCommit, ctl, originalPatchSet.getId());
} else {
newCommitMessage = original.getFullMessage();
}
rebasedCommit = rebaseCommit(ctx, original, baseCommit, newCommitMessage);
RevId baseRevId = new RevId((baseCommitish != null) ? baseCommitish

View File

@ -107,6 +107,7 @@ import com.google.gerrit.server.events.EventsMetrics;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.AbandonOp;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.EmailMerge;
import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitModules;
@ -342,6 +343,7 @@ public class GerritGlobalModule extends FactoryModule {
DynamicSet.bind(binder(), EventListener.class).to(EventsMetrics.class);
DynamicSet.setOf(binder(), UserScopedEventListener.class);
DynamicSet.setOf(binder(), CommitValidationListener.class);
DynamicSet.setOf(binder(), ChangeMessageModifier.class);
DynamicSet.setOf(binder(), RefOperationValidationListener.class);
DynamicSet.setOf(binder(), MergeValidationListener.class);
DynamicSet.setOf(binder(), ProjectCreationValidationListener.class);

View File

@ -0,0 +1,53 @@
// Copyright (C) 2016 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import com.google.gerrit.server.project.ChangeControl;
import org.eclipse.jgit.revwalk.RevCommit;
/**
* Allows to modify the commit message for new commits generated by Rebase
* Always submit strategy.
*
* Invoked by Gerrit when all information about new commit is already known such
* as parent(s), tree hash, etc, but commit's message can still be modified.
*/
@ExtensionPoint
public interface ChangeMessageModifier {
/**
* Implementation must return non-Null commit message.
*
* mergeTip and original commit are guaranteed to have their body parsed,
* meaning that their commit messages and footers can be accessed.
*
* @param newCommitMessage the new commit message that was result of either
* <ul>
* <li>{@link MergeUtil#createDetailedCommitMessage} called before</li>
* <li>other extensions or plugins implementing the same point and
* called before.</li>
* </ul>
* @param original the commit of the change being submitted. <b>Note that its
* commit message may be different than newCommitMessage argument.</b>
* @param mergeTip the current HEAD of the destination branch, which will be a
* parent of a new commit being generated
* @param ctl
* @return a new not null commit message.
*/
String onSubmit(String newCommitMessage, RevCommit original,
RevCommit mergeTip, ChangeControl ctl);
}

View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
@ -25,6 +26,7 @@ import com.google.common.collect.Sets;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@ -33,6 +35,7 @@ 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.PatchSet.Id;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
@ -44,6 +47,7 @@ import com.google.gerrit.server.git.strategy.CommitMergeStatus;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
@ -98,6 +102,32 @@ import java.util.Set;
*/
public class MergeUtil {
private static final Logger log = LoggerFactory.getLogger(MergeUtil.class);
static class PluggableCommitMessageGenerator {
private final DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Inject
public PluggableCommitMessageGenerator(
DynamicSet<ChangeMessageModifier> changeMessageModifiers) {
this.changeMessageModifiers = changeMessageModifiers;
}
public String generate(RevCommit original, RevCommit mergeTip,
ChangeControl ctl, String current) {
checkNotNull(original.getRawBuffer());
if (mergeTip != null) {
checkNotNull(mergeTip.getRawBuffer());
}
for (ChangeMessageModifier changeMessageModifier : changeMessageModifiers) {
current = changeMessageModifier.onSubmit(current, original,
mergeTip, ctl);
checkNotNull(current, changeMessageModifier.getClass().getName()
+ ".OnSubmit returned null instead of new commit message");
}
return current;
}
}
private static final String R_HEADS_MASTER =
Constants.R_HEADS + Constants.MASTER;
@ -123,25 +153,28 @@ public class MergeUtil {
private final ProjectState project;
private final boolean useContentMerge;
private final boolean useRecursiveMerge;
private final PluggableCommitMessageGenerator commitMessageGenerator;
@AssistedInject
MergeUtil(@GerritServerConfig Config serverConfig,
final Provider<ReviewDb> db,
final IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable final Provider<String> urlProvider,
final ApprovalsUtil approvalsUtil,
@Assisted final ProjectState project) {
Provider<ReviewDb> db,
IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable Provider<String> urlProvider,
ApprovalsUtil approvalsUtil,
PluggableCommitMessageGenerator commitMessageGenerator,
@Assisted ProjectState project) {
this(serverConfig, db, identifiedUserFactory, urlProvider, approvalsUtil,
project, project.isUseContentMerge());
project, commitMessageGenerator, project.isUseContentMerge());
}
@AssistedInject
MergeUtil(@GerritServerConfig Config serverConfig,
final Provider<ReviewDb> db,
final IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable final Provider<String> urlProvider,
final ApprovalsUtil approvalsUtil,
@Assisted final ProjectState project,
Provider<ReviewDb> db,
IdentifiedUser.GenericFactory identifiedUserFactory,
@CanonicalWebUrl @Nullable Provider<String> urlProvider,
ApprovalsUtil approvalsUtil,
@Assisted ProjectState project,
PluggableCommitMessageGenerator commitMessageGenerator,
@Assisted boolean useContentMerge) {
this.db = db;
this.identifiedUserFactory = identifiedUserFactory;
@ -150,6 +183,7 @@ public class MergeUtil {
this.project = project;
this.useContentMerge = useContentMerge;
this.useRecursiveMerge = useRecursiveMerge(serverConfig);
this.commitMessageGenerator = commitMessageGenerator;
}
public CodeReviewCommit getFirstFastForward(
@ -264,7 +298,7 @@ public class MergeUtil {
* @param psId
* @return new message
*/
public String createDetailedCommitMessage(RevCommit n, ChangeControl ctl,
private String createDetailedCommitMessage(RevCommit n, ChangeControl ctl,
PatchSet.Id psId) {
Change c = ctl.getChange();
final List<FooterLine> footers = n.getFooterLines();
@ -368,12 +402,32 @@ public class MergeUtil {
msgbuf.append('\n');
}
}
return msgbuf.toString();
}
public String createDetailedCommitMessage(final CodeReviewCommit n) {
return createDetailedCommitMessage(n, n.getControl(), n.getPatchsetId());
public String createCommitMessageOnSubmit(CodeReviewCommit n,
RevCommit mergeTip) {
return createCommitMessageOnSubmit(n, mergeTip, n.getControl(),
n.getPatchsetId());
}
/**
* Creates a commit message for a change, which can be customized by plugins.
*
* By default, adds footers to existing commit message based on the state of
* the change. Plugins implementing {@link ChangeMessageModifier} can modify
* the resulting commit message arbitrarily.
*
* @param n
* @param mergeTip
* @param ctl
* @param id
* @return new message
*/
public String createCommitMessageOnSubmit(RevCommit n, RevCommit mergeTip,
ChangeControl ctl, Id id) {
return commitMessageGenerator.generate(n, mergeTip, ctl,
createDetailedCommitMessage(n, ctl, id));
}
private static boolean isCodeReview(LabelId id) {

View File

@ -33,6 +33,7 @@ import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.io.IOException;
@ -99,8 +100,10 @@ public class CherryPick extends SubmitStrategy {
args.rw.parseBody(toMerge);
psId = ChangeUtil.nextPatchSetId(
args.repo, toMerge.change().currentPatchSetId());
RevCommit mergeTip = args.mergeTip.getCurrentTip();
args.rw.parseBody(mergeTip);
String cherryPickCmtMsg =
args.mergeUtil.createDetailedCommitMessage(toMerge);
args.mergeUtil.createCommitMessageOnSubmit(toMerge, mergeTip);
PersonIdent committer = args.caller.newCommitterIdent(
ctx.getWhen(), args.serverIdent.getTimeZone());

View File

@ -137,10 +137,10 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
args.rw.parseBody(toMerge);
newPatchSetId = ChangeUtil.nextPatchSetId(
args.repo, toMerge.change().currentPatchSetId());
// TODO(tandrii): add extension point to customize this commit message.
RevCommit mergeTip = args.mergeTip.getCurrentTip();
args.rw.parseBody(mergeTip);
String cherryPickCmtMsg =
args.mergeUtil.createDetailedCommitMessage(toMerge);
args.mergeUtil.createCommitMessageOnSubmit(toMerge, mergeTip);
PersonIdent committer = args.caller.newCommitterIdent(ctx.getWhen(),
args.serverIdent.getTimeZone());
try {
@ -162,8 +162,6 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
// Stale read of patch set is ok; see comments in RebaseChangeOp.
PatchSet origPs = args.psUtil.get(ctx.getDb(),
toMerge.getControl().getNotes(), toMerge.getPatchsetId());
// TODO(tandrii): add extension point to customize commit message while
// rebasing.
rebaseOp = args.rebaseFactory.create(
toMerge.getControl(), origPs, args.mergeTip.getCurrentTip().name())
.setFireRevisionCreated(false)