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

gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java

@@ -214,7 +214,7 @@ public class CherryPickChange {
       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

gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -473,7 +473,7 @@ public class ConsistencyChecker {
       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;

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -50,7 +50,6 @@ import com.google.gerrit.server.patch.PatchSetInfoFactory;
 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 @@ public class PatchSetInserter {
   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 @@ public class PatchSetInserter {
   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 @@ public class PatchSetInserter {
   @AssistedInject
   public PatchSetInserter(ChangeHooks hooks,
       ReviewDb db,
-      ChangeControl.GenericFactory ctlFactory,
       ApprovalsUtil approvalsUtil,
       ApprovalCopier approvalCopier,
       ChangeMessagesUtil cmUtil,
@@ -135,7 +132,6 @@ public class PatchSetInserter {
     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 class PatchSetInserter {
   public PatchSetInserter(ChangeHooks hooks,
       ReviewDb db,
       BatchUpdate.Factory batchUpdateFactory,
-      ChangeControl.GenericFactory ctlFactory,
       ApprovalsUtil approvalsUtil,
       ApprovalCopier approvalCopier,
       ChangeMessagesUtil cmUtil,
@@ -167,7 +162,6 @@ public class PatchSetInserter {
     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 @@ public class PatchSetInserter {
     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 @@ public class PatchSetInserter {
     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 @@ public class PatchSetInserter {
     Op op = new Op();
     try {
       bu.addOp(ctl, op);
-      if (!messageIsForChange()) {
-        commitMessageNotForChange(bu);
-      }
       if (executeBatch) {
         bu.execute();
       }
@@ -303,6 +283,7 @@ public class PatchSetInserter {
 
   private class Op extends BatchUpdate.Op {
     private Change change;
+    private ChangeMessage changeMessage;
     private SetMultimap<ReviewerState, Account.Id> oldReviewers;
 
     @Override
@@ -334,6 +315,14 @@ public class PatchSetInserter {
         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 @@ public class PatchSetInserter {
       }
 
       approvalCopier.copy(db, ctl, patchSet);
-      if (messageIsForChange()) {
+      if (changeMessage != null) {
         cmUtil.addChangeMessage(db, ctx.getChangeUpdate(), changeMessage);
       }
     }
@@ -391,21 +380,6 @@ public class PatchSetInserter {
     }
   }
 
-  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 @@ public class PatchSetInserter {
       throw new InvalidChangeOperationException(e.getMessage());
     }
   }
-
-  private boolean messageIsForChange() {
-    return changeMessage != null && changeMessage.getKey().getParentKey()
-        .equals(patchSet.getId().getParentKey());
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java

@@ -21,11 +21,9 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 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 @@ public class RebaseChange {
         .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());

gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java

@@ -215,7 +215,7 @@ public class ChangeEditUtil {
   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)));