PatchSetInserter: Don't set or expose PatchSet before insertion

Callers only really ever needed the patch set ID, so expose that
directly. There is no need to set the PatchSet externally, as we
already have setters for the appropriate subfields, like uploader.

Change-Id: Ib0f8182ccf520d399f12730568e3b4be59cc952c

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

@@ -345,17 +345,17 @@ public class ConsistencyChecker {
             + " is merged");
         return;
       }
-      checkMergedBitMatchesStatus(currPs, currPsCommit, merged);
+      checkMergedBitMatchesStatus(currPs.getId(), currPsCommit, merged);
     }
   }
 
-  private void checkMergedBitMatchesStatus(PatchSet ps, RevCommit commit,
+  private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit,
       boolean merged) {
     String refName = change.getDest().get();
     if (merged && change.getStatus() != Change.Status.MERGED) {
       ProblemInfo p = problem(String.format(
           "Patch set %d (%s) is merged into destination ref %s (%s), but change"
-          + " status is %s", ps.getId().get(), commit.name(),
+          + " status is %s", psId.get(), commit.name(),
           refName, tip.name(), change.getStatus()));
       if (fix != null) {
         fixMerged(p);
@@ -416,9 +416,9 @@ public class ConsistencyChecker {
                   commit.name(), changeId, change.getKey().get()));
             return;
           }
-          PatchSet ps = insertPatchSet(commit);
+          PatchSet.Id psId = insertPatchSet(commit);
-          if (ps != null) {
+          if (psId != null) {
-            checkMergedBitMatchesStatus(ps, commit, true);
+            checkMergedBitMatchesStatus(psId, commit, true);
           }
           break;
 
@@ -447,7 +447,7 @@ public class ConsistencyChecker {
     }
   }
 
-  private PatchSet insertPatchSet(RevCommit commit) {
+  private PatchSet.Id insertPatchSet(RevCommit commit) {
     ProblemInfo p =
         problem("No patch set found for merged commit " + commit.name());
     if (!user.get().isIdentifiedUser()) {
@@ -470,9 +470,10 @@ public class ConsistencyChecker {
           .setMessage(
               "Patch set for merged commit inserted by consistency checker")
           .insert();
+      PatchSet.Id psId = change.currentPatchSetId();
       p.status = Status.FIXED;
-      p.outcome = "Inserted as patch set " + change.currentPatchSetId().get();
+      p.outcome = "Inserted as patch set " + psId.get();
-      return inserter.getPatchSet();
+      return psId;
     } catch (InvalidChangeOperationException | IOException
         | NoSuchChangeException | UpdateException | RestApiException e) {
       warn(e);

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

@@ -14,9 +14,9 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.ChangeHooks;
@@ -37,10 +37,10 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.GroupCollector;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.BatchUpdate.RepoContext;
+import com.google.gerrit.server.git.GroupCollector;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
@@ -103,7 +103,7 @@ public class PatchSetInserter {
   private final Repository git;
   private final RevWalk revWalk;
 
-  private PatchSet patchSet;
+  private PatchSet.Id psId;
   private String message;
   private SshInfo sshInfo;
   private ValidatePolicy validatePolicy = ValidatePolicy.GERRIT;
@@ -152,27 +152,12 @@ public class PatchSetInserter {
     return (IdentifiedUser) ctl.getCurrentUser();
   }
 
-  public PatchSetInserter setPatchSet(PatchSet patchSet) {
-    Change c = ctl.getChange();
-    PatchSet.Id psid = patchSet.getId();
-    checkArgument(psid.getParentKey().equals(c.getId()),
-        "patch set %s not for change %s", psid, c.getId());
-    checkArgument(psid.get() > c.currentPatchSetId().get(),
-        "new patch set ID %s is not greater than current patch set ID %s",
-        psid.get(), c.currentPatchSetId().get());
-    this.patchSet = patchSet;
-    return this;
-  }
-
   public PatchSet.Id getPatchSetId() throws IOException {
-    init();
+    if (psId == null) {
-    return patchSet.getId();
+      psId =
-  }
+          ChangeUtil.nextPatchSetId(git, ctl.getChange().currentPatchSetId());
-
+    }
-  public PatchSet getPatchSet() {
+    return psId;
-    checkState(patchSet != null,
-        "getPatchSet() only valid after patch set is created");
-    return patchSet;
   }
 
   public PatchSetInserter setMessage(String message) {
@@ -227,7 +212,7 @@ public class PatchSetInserter {
 
     Op op = new Op();
     try (BatchUpdate bu = batchUpdateFactory.create(
-          db, ctl.getChange().getProject(), patchSet.getCreatedOn())) {
+          db, ctl.getChange().getProject(), TimeUtil.nowTs())) {
       bu.addOp(ctl, op);
       bu.execute();
     }
@@ -236,13 +221,14 @@ public class PatchSetInserter {
 
   private class Op extends BatchUpdate.Op {
     private Change change;
+    private PatchSet patchSet;
     private ChangeMessage changeMessage;
     private SetMultimap<ReviewerState, Account.Id> oldReviewers;
 
     @Override
     public void updateRepo(RepoContext ctx) throws IOException {
       ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(),
-          commit, patchSet.getRefName(), ReceiveCommand.Type.CREATE));
+          commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
     }
 
     @Override
@@ -256,6 +242,12 @@ public class PatchSetInserter {
             "Change %s is closed", change.getId()));
       }
 
+      patchSet = new PatchSet(psId);
+      patchSet.setCreatedOn(ctx.getWhen());
+      patchSet.setUploader(firstNonNull(uploader, ctl.getChange().getOwner()));
+      patchSet.setRevision(new RevId(commit.name()));
+      patchSet.setDraft(draft);
+
       ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
       if (groups != null) {
         patchSet.setGroups(groups);
@@ -290,8 +282,7 @@ public class PatchSetInserter {
           if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
             change.setStatus(Change.Status.NEW);
           }
-          change.setCurrentPatchSet(patchSetInfoFactory.get(commit,
+          change.setCurrentPatchSet(patchSetInfoFactory.get(commit, psId));
-              patchSet.getId()));
           ChangeUtil.updated(change);
           return change;
         }
@@ -311,8 +302,7 @@ public class PatchSetInserter {
     public void postUpdate(Context ctx) throws OrmException {
       if (sendMail) {
         try {
-          PatchSetInfo info =
+          PatchSetInfo info = patchSetInfoFactory.get(commit, psId);
-              patchSetInfoFactory.get(commit, patchSet.getId());
           ReplacePatchSetSender cm = replacePatchSetFactory.create(
               change.getId());
           cm.setFrom(user.getAccountId());
@@ -333,28 +323,17 @@ public class PatchSetInserter {
     }
   }
 
-  private void init() throws IOException {
+  private void init() {
     if (sshInfo == null) {
       sshInfo = new NoSshInfo();
     }
-    if (patchSet == null) {
-      patchSet = new PatchSet(
-          ChangeUtil.nextPatchSetId(git, ctl.getChange().currentPatchSetId()));
-      patchSet.setCreatedOn(TimeUtil.nowTs());
-      patchSet.setUploader(ctl.getChange().getOwner());
-      patchSet.setRevision(new RevId(commit.name()));
-    }
-    patchSet.setDraft(draft);
-    if (uploader != null) {
-      patchSet.setUploader(uploader);
-    }
   }
 
   private void validate() throws InvalidChangeOperationException, IOException {
     CommitValidators cv =
         commitValidatorsFactory.create(ctl.getRefControl(), sshInfo, git);
 
-    String refName = patchSet.getRefName();
+    String refName = getPatchSetId().toRefName();
     CommitReceivedEvent event = new CommitReceivedEvent(
         new ReceiveCommand(
             ObjectId.zeroId(),

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

@@ -17,7 +17,6 @@ package com.google.gerrit.server.edit;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.Optional;
-import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -25,9 +24,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Status;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.RefNames;
-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.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.ChangeKind;
@@ -39,6 +36,8 @@ import com.google.gerrit.server.index.ChangeIndexer;
 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.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -67,6 +66,7 @@ public class ChangeEditUtil {
   private final PatchSetInserter.Factory patchSetInserterFactory;
   private final ChangeControl.GenericFactory changeControlFactory;
   private final ChangeIndexer indexer;
+  private final ProjectCache projectCache;
   private final Provider<ReviewDb> db;
   private final Provider<CurrentUser> user;
   private final ChangeKindCache changeKindCache;
@@ -76,6 +76,7 @@ public class ChangeEditUtil {
       PatchSetInserter.Factory patchSetInserterFactory,
       ChangeControl.GenericFactory changeControlFactory,
       ChangeIndexer indexer,
+      ProjectCache projectCache,
       Provider<ReviewDb> db,
       Provider<CurrentUser> user,
       ChangeKindCache changeKindCache) {
@@ -83,6 +84,7 @@ public class ChangeEditUtil {
     this.patchSetInserterFactory = patchSetInserterFactory;
     this.changeControlFactory = changeControlFactory;
     this.indexer = indexer;
+    this.projectCache = projectCache;
     this.db = db;
     this.user = user;
     this.changeKindCache = changeKindCache;
@@ -216,17 +218,19 @@ public class ChangeEditUtil {
       Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed)
       throws NoSuchChangeException, RestApiException, UpdateException,
       InvalidChangeOperationException, IOException {
-    PatchSet ps = new PatchSet(
+    PatchSetInserter inserter =
-        ChangeUtil.nextPatchSetId(change.currentPatchSetId()));
+        patchSetInserterFactory.create(repo, rw,
-    ps.setRevision(new RevId(ObjectId.toString(squashed)));
+            changeControlFactory.controlFor(change, edit.getUser()),
-    ps.setUploader(edit.getUser().getAccountId());
+            squashed);
-    ps.setCreatedOn(TimeUtil.nowTs());
 
     StringBuilder message = new StringBuilder("Patch set ")
-      .append(ps.getPatchSetId())
+      .append(inserter.getPatchSetId().get())
       .append(": ");
 
-    ChangeKind kind = changeKindCache.getChangeKind(db.get(), change, ps);
+    ProjectState project = projectCache.get(change.getDest().getParentKey());
+    // Previously checked that the base patch set is the current patch set.
+    ObjectId prior = ObjectId.fromString(basePatchSet.getRevision().get());
+    ChangeKind kind = changeKindCache.getChangeKind(project, repo, prior, squashed);
     if (kind == ChangeKind.NO_CODE_CHANGE) {
       message.append("Commit message was updated.");
     } else {
@@ -235,11 +239,7 @@ public class ChangeEditUtil {
         .append(".");
     }
 
-    PatchSetInserter inserter =
+    return inserter
-        patchSetInserterFactory.create(repo, rw,
-            changeControlFactory.controlFor(change, edit.getUser()),
-            squashed);
-    return inserter.setPatchSet(ps)
         .setDraft(change.getStatus() == Status.DRAFT ||
             basePatchSet.isDraft())
         .setMessage(message.toString())