opendev/gerrit
Code Issues Proposed changes

Merge "Add extension point to customize detailed commit message generation."

Browse Source
This commit is contained in:
David Pursehouse 2016-11-30 12:44:21 +00:00 committed by Gerrit Code Review
commit 723b639e93
11 changed files with 277 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
Documentation/dev-plugins.txt
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

42
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
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);

34
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
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 {

45
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
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);

9
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
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());
}
}
}

17
gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
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

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
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);

53
gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeMessageModifier.java Normal file
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);
}

84
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
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) {

5
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
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());

8
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
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)