Refactor Rebase to use PatchSetInserter
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
diff --git a/Documentation/config-mail.txt b/Documentation/config-mail.txt
index 53c9750..6e333db 100644
--- a/Documentation/config-mail.txt
+++ b/Documentation/config-mail.txt
@@ -93,13 +93,6 @@
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
~~~~~~~~~~~~~~~~~~~
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java
index 415c55a..e71f3d1 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java
@@ -95,7 +95,6 @@
extractMailExample("Merged.vm");
extractMailExample("MergeFail.vm");
extractMailExample("NewChange.vm");
- extractMailExample("RebasedPatchSet.vm");
extractMailExample("RegisterNewEmail.vm");
extractMailExample("ReplacePatchSet.vm");
extractMailExample("Restored.vm");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
index abba89f..ccbe365 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
@@ -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.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 @@
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 @@
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 @@
*
* 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 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 @@
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());
+ PatchSetInserter patchSetinserter = patchSetInserterFactory
+ .create(git, revWalk, changeControl.getRefControl(), change, rebasedCommit)
+ .setCopyLabels(true)
+ .setDraft(originalPatchSet.isDraft())
+ .setSendMail(sendMail)
+ .setRunHooks(runHooks);
- 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);
+ final ChangeMessage cmsg =
+ new ChangeMessage(new ChangeMessage.Key(change.getId(),
+ ChangeUtil.messageUUID(db)), uploader.getAccountId(), patchSetId);
+ cmsg.setMessage("Patch Set " + change.currentPatchSetId().get()
+ + ": Patch Set " + patchSetId.get() + " was rebased");
- 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()));
- }
+ Change newChange = patchSetinserter
+ .setMessage(cmsg)
+ .insert();
- 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());
-
- final ChangeMessage cmsg =
- new ChangeMessage(new ChangeMessage.Key(change.getId(),
- ChangeUtil.messageUUID(db)), uploader, 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;
+ return db.patchSets().get(newChange.currentPatchSetId());
}
/**
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 2e69ff0..56f13be 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -87,7 +87,6 @@
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 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java
index 0518349..d237f49 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseIfNecessary.java
@@ -18,6 +18,7 @@
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 @@
} 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));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RebasedPatchSetSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RebasedPatchSetSender.java
deleted file mode 100644
index f9a85b9..0000000
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RebasedPatchSetSender.java
+++ /dev/null
@@ -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"));
- }
-}
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/RebasedPatchSet.vm b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/RebasedPatchSet.vm
deleted file mode 100644
index e761627..0000000
--- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/RebasedPatchSet.vm
+++ /dev/null
@@ -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
