PatchSetInserter: Simplify message setup
The commitMessageNotForChange branch was patterned after
ChangeInserter. However, the only caller that adds a message on a
different change was CherryPickChange, which adds the message in a
separate update in a different codepath. So, eliminate this ugly path.
Since we no longer allow callers to specify messages for another
change, the only field in the ChangeMessage that they need control
over is the message text itself. Simplify the setup by eliminating the
setMessage(ChangeMessage) method entirely and only storing the message
string.
Change-Id: Ifb4037bcb2962b4aba2b0d3fe5e660295aecf0a0
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
index e6ecb52..5b7c95d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
@@ -214,7 +214,7 @@
CodeReviewCommit cherryPickCommit, RefControl refControl,
IdentifiedUser identifiedUser)
throws InvalidChangeOperationException, IOException, OrmException,
- NoSuchChangeException, UpdateException, RestApiException {
+ UpdateException, RestApiException {
final ChangeControl changeControl =
refControl.getProjectControl().controlFor(change);
final PatchSetInserter inserter = patchSetInserterFactory
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
index 9247b9f..39d2ff0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -473,7 +473,7 @@
p.status = Status.FIXED;
p.outcome = "Inserted as patch set " + change.currentPatchSetId().get();
return inserter.getPatchSet();
- } catch (InvalidChangeOperationException | OrmException | IOException
+ } catch (InvalidChangeOperationException | IOException
| NoSuchChangeException | UpdateException | RestApiException e) {
warn(e);
p.status = Status.FIX_FAILED;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 5b93f38..a760c9b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -50,7 +50,6 @@
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ChangeModifiedException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.ssh.SshInfo;
import com.google.gwtorm.server.AtomicUpdate;
@@ -94,7 +93,6 @@
private final PatchSetInfoFactory patchSetInfoFactory;
private final ReviewDb db;
private final BatchUpdate.Factory batchUpdateFactory;
- private final ChangeControl.GenericFactory ctlFactory;
private final CommitValidators.Factory commitValidatorsFactory;
private final ReplacePatchSetSender.Factory replacePatchSetFactory;
private final ApprovalsUtil approvalsUtil;
@@ -109,7 +107,7 @@
private Repository git;
private RevWalk revWalk;
private PatchSet patchSet;
- private ChangeMessage changeMessage;
+ private String message;
private SshInfo sshInfo;
private ValidatePolicy validatePolicy = ValidatePolicy.GERRIT;
private boolean draft;
@@ -122,7 +120,6 @@
@AssistedInject
public PatchSetInserter(ChangeHooks hooks,
ReviewDb db,
- ChangeControl.GenericFactory ctlFactory,
ApprovalsUtil approvalsUtil,
ApprovalCopier approvalCopier,
ChangeMessagesUtil cmUtil,
@@ -135,7 +132,6 @@
this.hooks = hooks;
this.db = db;
this.batchUpdateFactory = null;
- this.ctlFactory = ctlFactory;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil;
@@ -153,7 +149,6 @@
public PatchSetInserter(ChangeHooks hooks,
ReviewDb db,
BatchUpdate.Factory batchUpdateFactory,
- ChangeControl.GenericFactory ctlFactory,
ApprovalsUtil approvalsUtil,
ApprovalCopier approvalCopier,
ChangeMessagesUtil cmUtil,
@@ -167,7 +162,6 @@
this.hooks = hooks;
this.db = db;
this.batchUpdateFactory = batchUpdateFactory;
- this.ctlFactory = ctlFactory;
this.approvalsUtil = approvalsUtil;
this.approvalCopier = approvalCopier;
this.cmUtil = cmUtil;
@@ -213,19 +207,8 @@
return patchSet;
}
- public PatchSetInserter setMessage(String message)
- throws OrmException, IOException {
- init();
- changeMessage = new ChangeMessage(
- new ChangeMessage.Key(
- ctl.getChange().getId(), ChangeUtil.messageUUID(db)),
- user.getAccountId(), TimeUtil.nowTs(), patchSet.getId());
- changeMessage.setMessage(message);
- return this;
- }
-
- public PatchSetInserter setMessage(ChangeMessage changeMessage) {
- this.changeMessage = changeMessage;
+ public PatchSetInserter setMessage(String message) {
+ this.message = message;
return this;
}
@@ -269,8 +252,8 @@
return this;
}
- public Change insert() throws InvalidChangeOperationException, OrmException,
- IOException, NoSuchChangeException, UpdateException, RestApiException {
+ public Change insert() throws InvalidChangeOperationException, IOException,
+ UpdateException, RestApiException {
init();
validate();
@@ -287,9 +270,6 @@
Op op = new Op();
try {
bu.addOp(ctl, op);
- if (!messageIsForChange()) {
- commitMessageNotForChange(bu);
- }
if (executeBatch) {
bu.execute();
}
@@ -303,6 +283,7 @@
private class Op extends BatchUpdate.Op {
private Change change;
+ private ChangeMessage changeMessage;
private SetMultimap<ReviewerState, Account.Id> oldReviewers;
@Override
@@ -334,6 +315,14 @@
oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes());
}
+ if (message != null) {
+ changeMessage = new ChangeMessage(
+ new ChangeMessage.Key(
+ ctl.getChange().getId(), ChangeUtil.messageUUID(db)),
+ user.getAccountId(), ctx.getWhen(), patchSet.getId());
+ changeMessage.setMessage(message);
+ }
+
// TODO(dborowitz): Throw ResourceConflictException instead of using
// AtomicUpdate.
change = db.changes().atomicUpdate(id, new AtomicUpdate<Change>() {
@@ -360,7 +349,7 @@
}
approvalCopier.copy(db, ctl, patchSet);
- if (messageIsForChange()) {
+ if (changeMessage != null) {
cmUtil.addChangeMessage(db, ctx.getChangeUpdate(), changeMessage);
}
}
@@ -391,21 +380,6 @@
}
}
- private void commitMessageNotForChange(BatchUpdate bu)
- throws NoSuchChangeException, OrmException {
- if (changeMessage == null) {
- return;
- }
- bu.addOp(ctlFactory.controlFor(
- changeMessage.getPatchSetId().getParentKey(), user), new Op() {
- @Override
- public void updateChange(ChangeContext ctx) throws OrmException {
- cmUtil.addChangeMessage(
- ctx.getDb(), ctx.getChangeUpdate(), changeMessage);
- }
- });
- }
-
private void init() throws IOException {
if (git == null) {
git = batchUpdate.getRepository();
@@ -456,9 +430,4 @@
throw new InvalidChangeOperationException(e.getMessage());
}
}
-
- private boolean messageIsForChange() {
- return changeMessage != null && changeMessage.getKey().getParentKey()
- .equals(patchSet.getId().getParentKey());
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java
index b5ef0a5..865ec39 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java
@@ -21,11 +21,9 @@
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.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
@@ -284,17 +282,9 @@
.setSendMail(false)
.setRunHooks(runHooks);
- PatchSet.Id newPatchSetId = patchSetInserter.getPatchSetId();
- ChangeMessage cmsg = new ChangeMessage(
- new ChangeMessage.Key(change.getId(),
- ChangeUtil.messageUUID(db.get())), uploader.getAccountId(),
- TimeUtil.nowTs(), patchSetId);
-
- cmsg.setMessage("Patch Set " + newPatchSetId.get()
- + ": Patch Set " + patchSetId.get() + " was rebased");
-
Change newChange = patchSetInserter
- .setMessage(cmsg)
+ .setMessage("Patch Set " + patchSetInserter.getPatchSetId().get()
+ + ": Patch Set " + patchSetId.get() + " was rebased")
.insert();
return db.get().patchSets().get(newChange.currentPatchSetId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
index bc093c2..c465aad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -215,7 +215,7 @@
private Change insertPatchSet(ChangeEdit edit, Change change,
Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed)
throws NoSuchChangeException, RestApiException, UpdateException,
- InvalidChangeOperationException, OrmException, IOException {
+ InvalidChangeOperationException, IOException {
PatchSet ps = new PatchSet(
ChangeUtil.nextPatchSetId(change.currentPatchSetId()));
ps.setRevision(new RevId(ObjectId.toString(squashed)));
