opendev/gerrit
Code Issues Proposed changes

Refactor Rebase to use PatchSetInserter

Browse Source 
Refactor the rebase to use the PatchSetInserter class.

The PatchSetInserter takes care of sending email notifications about
new patch sets, so remove the rebase-specific sender.

Change-Id: I1e05db299d03d6b4a9eec4ae5bca4cd62cf01a4f
This commit is contained in:
David Pursehouse
2013-07-08 18:01:55 +09:00
parent c8fe07b8f0
commit fd23ab518f
7 changed files with 43 additions and 242 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/config-mail.txt
View File

@@ -93,13 +93,6 @@ The `NewChange.vm` template will determine the contents of the email related
to a user submitting a new change for review. It is a `ChangeEmail`: see
`ChangeSubject.vm` and `ChangeFooter.vm`.
RebasedPatchSet.vm
~~~~~~~~~~~~~~~~~~
The `RebasedPatchSet.vm` template will determine the contents of the email
related to a user rebasing a patchset for a change through the Gerrit UI.
It is a `ChangeEmail`: see `ChangeSubject.vm` and `ChangeFooter.vm`.
RegisterNewEmail.vm
~~~~~~~~~~~~~~~~~~~

1
gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java
View File

@@ -95,7 +95,6 @@ public class SitePathInitializer {
extractMailExample("Merged.vm");
extractMailExample("MergeFail.vm");
extractMailExample("NewChange.vm");
extractMailExample("RebasedPatchSet.vm");
extractMailExample("RegisterNewEmail.vm");
extractMailExample("ReplacePatchSet.vm");
extractMailExample("Restored.vm");

168
gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
View File

@@ -14,36 +14,25 @@
package com.google.gerrit.server.changedetail;
import com.google.common.collect.Sets;
import com.google.gerrit.common.ChangeHookRunner;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetAncestor;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.mail.RebasedPatchSetSender;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.AtomicUpdate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -53,53 +42,36 @@ import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Collections;
import java.util.List;
import java.util.Set;
public class RebaseChange {
private final ChangeControl.GenericFactory changeControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final ReviewDb db;
private final GitRepositoryManager gitManager;
private final PersonIdent myIdent;
private final GitReferenceUpdated gitRefUpdated;
private final RebasedPatchSetSender.Factory rebasedPatchSetSenderFactory;
private final ChangeHookRunner hooks;
private final MergeUtil.Factory mergeUtilFactory;
private final ProjectCache projectCache;
private final ChangeIndexer indexer;
private final PatchSetInserter.Factory patchSetInserterFactory;
@Inject
RebaseChange(final ChangeControl.GenericFactory changeControlFactory,
final PatchSetInfoFactory patchSetInfoFactory, final ReviewDb db,
final ReviewDb db,
@GerritPersonIdent final PersonIdent myIdent,
final GitRepositoryManager gitManager,
final GitReferenceUpdated gitRefUpdated,
final RebasedPatchSetSender.Factory rebasedPatchSetSenderFactory,
final ChangeHookRunner hooks,
final MergeUtil.Factory mergeUtilFactory,
final ProjectCache projectCache,
final ChangeIndexer changeIndexer) {
final ChangeIndexer changeIndexer,
final PatchSetInserter.Factory patchSetInserterFactory) {
this.changeControlFactory = changeControlFactory;
this.patchSetInfoFactory = patchSetInfoFactory;
this.db = db;
this.gitManager = gitManager;
this.myIdent = myIdent;
this.gitRefUpdated = gitRefUpdated;
this.rebasedPatchSetSenderFactory = rebasedPatchSetSenderFactory;
this.hooks = hooks;
this.mergeUtilFactory = mergeUtilFactory;
this.projectCache = projectCache;
this.indexer = changeIndexer;
this.patchSetInserterFactory = patchSetInserterFactory;
}
/**
@@ -148,9 +120,6 @@ public class RebaseChange {
rw = new RevWalk(git);
inserter = git.newObjectInserter();
final List<PatchSetApproval> oldPatchSetApprovals =
db.patchSetApprovals().byChange(change.getId()).toList();
final String baseRev = findBaseRevision(patchSetId, db,
change.getDest(), git, null, null, null);
final RevCommit baseCommit =
@@ -160,29 +129,10 @@ public class RebaseChange {
uploader.newCommitterIdent(myIdent.getWhen(),
myIdent.getTimeZone());
final PatchSet newPatchSet = rebase(git, rw, inserter, patchSetId, change,
uploader.getAccountId(), baseCommit, mergeUtilFactory.create(
rebase(git, rw, inserter, patchSetId, change,
uploader, baseCommit, mergeUtilFactory.create(
changeControl.getProjectControl().getProjectState(), true),
committerIdent, indexer);
final Set<Account.Id> oldReviewers = Sets.newHashSet();
final Set<Account.Id> oldCC = Sets.newHashSet();
for (PatchSetApproval a : oldPatchSetApprovals) {
if (a.getValue() != 0) {
oldReviewers.add(a.getAccountId());
} else {
oldCC.add(a.getAccountId());
}
}
final ReplacePatchSetSender cm =
rebasedPatchSetSenderFactory.create(change);
cm.setFrom(uploader.getAccountId());
cm.setPatchSet(newPatchSet);
cm.addReviewers(oldReviewers);
cm.addExtraCC(oldCC);
cm.send();
hooks.doPatchsetCreatedHook(change, newPatchSet, db);
committerIdent, true, true);
} catch (PathConflictException e) {
throw new IOException(e.getMessage());
} finally {
@@ -294,18 +244,20 @@ public class RebaseChange {
*
* The rebased commit is added as new patch set to the change.
*
* E-mail notification and triggering of hooks is NOT done for the creation of
* the new patch set.
* E-mail notification and triggering of hooks is only done for the creation of
* the new patch set if `sendEmail` and `runHooks` are set to true.
*
* @param git the repository
* @param revWalk the RevWalk
* @param inserter the object inserter
* @param patchSetId the id of the patch set
* @param chg the change that should be rebased
* @param change the change that should be rebased
* @param uploader the user that creates the rebased patch set
* @param baseCommit the commit that should be the new base
* @param mergeUtil merge utilities for the destination project
* @param indexer helper for indexing the change
* @param committerIdent the committer's identity
* @param sendMail if a mail notification should be sent for the new patch set
* @param runHooks if hooks should be run for the new patch set
* @return the new patch set which is based on the given base commit
* @throws NoSuchChangeException thrown if the change to which the patch set
* belongs does not exist or is not visible to the user
@@ -315,13 +267,13 @@ public class RebaseChange {
*/
public PatchSet rebase(final Repository git, final RevWalk revWalk,
final ObjectInserter inserter, final PatchSet.Id patchSetId,
final Change chg, final Account.Id uploader, final RevCommit baseCommit,
final Change change, final IdentifiedUser uploader, final RevCommit baseCommit,
final MergeUtil mergeUtil, PersonIdent committerIdent,
final ChangeIndexer indexer) throws NoSuchChangeException,
boolean sendMail, boolean runHooks)
throws NoSuchChangeException,
OrmException, IOException, InvalidChangeOperationException,
PathConflictException {
Change change = chg;
if (!chg.currentPatchSetId().equals(patchSetId)) {
if (!change.currentPatchSetId().equals(patchSetId)) {
throw new InvalidChangeOperationException("patch set is not current");
}
final PatchSet originalPatchSet = db.patchSets().get(patchSetId);
@@ -333,81 +285,27 @@ public class RebaseChange {
rebasedCommit = revWalk.parseCommit(newId);
PatchSet.Id id = ChangeUtil.nextPatchSetId(git, change.currentPatchSetId());
final PatchSet newPatchSet = new PatchSet(id);
newPatchSet.setCreatedOn(new Timestamp(System.currentTimeMillis()));
newPatchSet.setUploader(uploader);
newPatchSet.setRevision(new RevId(rebasedCommit.name()));
newPatchSet.setDraft(originalPatchSet.isDraft());
final ChangeControl changeControl =
changeControlFactory.validateFor(change.getId(), uploader);
final PatchSetInfo info =
patchSetInfoFactory.get(rebasedCommit, newPatchSet.getId());
final RefUpdate ru = git.updateRef(newPatchSet.getRefName());
ru.setExpectedOldObjectId(ObjectId.zeroId());
ru.setNewObjectId(rebasedCommit);
ru.disableRefLog();
if (ru.update(revWalk) != RefUpdate.Result.NEW) {
throw new IOException(String.format("Failed to create ref %s in %s: %s",
newPatchSet.getRefName(), change.getDest().getParentKey().get(),
ru.getResult()));
}
gitRefUpdated.fire(change.getProject(), ru);
db.changes().beginTransaction(change.getId());
try {
Change updatedChange = db.changes().get(change.getId());
if (updatedChange != null && change.getStatus().isOpen()) {
change = updatedChange;
} else {
throw new InvalidChangeOperationException(String.format(
"Change %s is closed", change.getId()));
}
ChangeUtil.insertAncestors(db, newPatchSet.getId(), rebasedCommit);
db.patchSets().insert(Collections.singleton(newPatchSet));
updatedChange =
db.changes().atomicUpdate(change.getId(), new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
if (change.getStatus().isClosed()) {
return null;
}
if (!change.currentPatchSetId().equals(patchSetId)) {
return null;
}
if (change.getStatus() != Change.Status.DRAFT) {
change.setStatus(Change.Status.NEW);
}
change.setLastSha1MergeTested(null);
change.setCurrentPatchSet(info);
ChangeUtil.updated(change);
return change;
}
});
if (updatedChange != null) {
change = updatedChange;
} else {
throw new InvalidChangeOperationException(String.format(
"Change %s was modified", change.getId()));
}
indexer.index(change);
ApprovalsUtil.copyLabels(db, projectCache.get(change.getProject())
.getLabelTypes(), patchSetId, change.currentPatchSetId());
PatchSetInserter patchSetinserter = patchSetInserterFactory
.create(git, revWalk, changeControl.getRefControl(), change, rebasedCommit)
.setCopyLabels(true)
.setDraft(originalPatchSet.isDraft())
.setSendMail(sendMail)
.setRunHooks(runHooks);
final ChangeMessage cmsg =
new ChangeMessage(new ChangeMessage.Key(change.getId(),
ChangeUtil.messageUUID(db)), uploader, patchSetId);
ChangeUtil.messageUUID(db)), uploader.getAccountId(), patchSetId);
cmsg.setMessage("Patch Set " + change.currentPatchSetId().get()
+ ": Patch Set " + patchSetId.get() + " was rebased");
db.changeMessages().insert(Collections.singleton(cmsg));
db.commit();
} finally {
db.rollback();
}
return newPatchSet;
Change newChange = patchSetinserter
.setMessage(cmsg)
.insert();
return db.patchSets().get(newChange.currentPatchSetId());
}
/**

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@@ -87,7 +87,6 @@ import com.google.gerrit.server.mail.FromAddressGenerator;
import com.google.gerrit.server.mail.FromAddressGeneratorProvider;
import com.google.gerrit.server.mail.MergeFailSender;
import com.google.gerrit.server.mail.MergedSender;
import com.google.gerrit.server.mail.RebasedPatchSetSender;
import com.google.gerrit.server.mail.RegisterNewEmailSender;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.mail.VelocityRuntimeProvider;
@@ -191,7 +190,6 @@ public class GerritGlobalModule extends FactoryModule {
factory(PluginUser.Factory.class);
factory(ProjectNode.Factory.class);
factory(ProjectState.Factory.class);
factory(RebasedPatchSetSender.Factory.class);
factory(RegisterNewEmailSender.Factory.class);
factory(ReplacePatchSetSender.Factory.class);
factory(PerformCreateProject.Factory.class);

10
gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java
View File

@@ -18,6 +18,7 @@ import com.google.common.collect.Lists;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.changedetail.PathConflictException;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -76,11 +77,14 @@ public class RebaseIfNecessary extends SubmitStrategy {
} else {
try {
final IdentifiedUser uploader =
args.identifiedUserFactory.create(
args.mergeUtil.getSubmitter(n.patchsetId).getAccountId());
final PatchSet newPatchSet =
rebaseChange.rebase(args.repo, args.rw, args.inserter,
n.patchsetId, n.change,
args.mergeUtil.getSubmitter(n.patchsetId).getAccountId(),
newMergeTip, args.mergeUtil, committerIdent, args.indexer);
n.patchsetId, n.change, uploader,
newMergeTip, args.mergeUtil, committerIdent,
false, false);
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.mergeUtil.getApprovalsForCommit(n)) {
approvals.add(new PatchSetApproval(newPatchSet.getId(), a));

37
gerrit-server/src/main/java/com/google/gerrit/server/mail/RebasedPatchSetSender.java
View File

@@ -1,37 +0,0 @@
// Copyright (C) 2012 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.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
/** Send notice to reviewers that a change has been rebased. */
public class RebasedPatchSetSender extends ReplacePatchSetSender {
public static interface Factory {
RebasedPatchSetSender create(Change change);
}
@Inject
public RebasedPatchSetSender(EmailArguments ea, @Assisted Change c) {
super(ea, c);
}
@Override
protected void formatChange() throws EmailException {
appendText(velocifyFile("RebasedPatchSet.vm"));
}
}

54
gerrit-server/src/main/resources/com/google/gerrit/server/mail/RebasedPatchSet.vm
View File

@@ -1,54 +0,0 @@
## Copyright (C) 2012 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.
##
##
## Template Type:
## -------------
## This is a velocity mail template, see: http://velocity.apache.org and the
## gerrit-docs:config-mail.txt for more info on modifying gerrit mail templates.
##
## Template File Names and extensions:
## ----------------------------------
## Gerrit will use templates ending in ".vm" but will ignore templates ending
## in ".vm.example". If a .vm template does not exist, the default internal
## gerrit template which is the same as the .vm.example will be used. If you
## want to override the default template, copy the .vm.example file to a .vm
## file and edit it appropriately.
##
## This Template:
## --------------
## The RebasedPatchSet.vm template will determine the contents of the email
## related to a user rebasing a patchset for a change through the Gerrit UI.
## It is a ChangeEmail: see ChangeSubject.vm and ChangeFooter.vm.
##
#if($email.reviewerNames)
Hello $email.joinStrings($email.reviewerNames, ', '),
I'd like you to reexamine a rebased change.#if($email.changeUrl) Please visit
$email.changeUrl
to look at the new rebased patch set (#$patchSet.patchSetId).
#end
#else
$fromName has created a new patch set by issuing a rebase in Gerrit (#$patchSet.patchSetId).
#end
Change subject: $change.subject
......................................................................
$email.changeDetail
#if($email.sshHost)
git pull ssh://$email.sshHost/$projectName $patchSet.refName
#end